LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Jesper Juhl <jj@chaosbits.net>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, linux-mm@kvack.org,
	Tejun Heo <tj@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
	Ingo Molnar <mingo@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Rohit Seth <rohit.seth@intel.com>
Subject: Re: [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share()
Date: Thu, 3 Feb 2011 13:36:24 -0800	[thread overview]
Message-ID: <20110203133624.cd353dd6.akpm@linux-foundation.org> (raw)
In-Reply-To: <alpine.LNX.2.00.1102032214350.1369@swampdragon.chaosbits.net>

On Thu, 3 Feb 2011 22:17:53 +0100 (CET)
Jesper Juhl <jj@chaosbits.net> wrote:

> Document that NULL deref will never happen post find_vma() in 
> huge_pmd_share().
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> ---
>  hugetlbpage.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 069ce7c..7dd2d5f 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -61,6 +61,11 @@ static int vma_shareable(struct vm_area_struct *vma, unsigned long addr)
>  static void huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
>  {
>  	struct vm_area_struct *vma = find_vma(mm, addr);
> +	/*
> +	 * There is no potential NULL deref here. mmap_sem is held and
> +	 * caller knows that the virtual address at `addr' is within a
> +	 * vma, so find_vma() will never return NULL here.
> +	 */
>  	struct address_space *mapping = vma->vm_file->f_mapping;
>  	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
>  			vma->vm_pgoff;

Not really.

That mmap_sem is held and that `addr' refers to a known-to-be-present
VMA are part of huge_pmd_share()'s interface.  They are preconditions
which must be satisfied before the function can be used.

So they should be documented as such, in the function's documentation. 
In fact they're the *most important* thing to document about the
function because they are subtle and unobvious from the implementation
and from the function signature and name.

>From an understandability/maintainability POV the code is crap.  It's yet
another example of kernel developers' liking for pointing machine guns
at each other's feet.

Really, some poor schmuck needs to go in and reverse-engineer all the
secret interface requirements and document them.  But we shouldn't let
a chance go by - a nice kerneldoc description for huge_pmd_share()
would be appreciated.  One which documents both the explicit and the
implicit calling conventions.


  reply	other threads:[~2011-02-03 21:36 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-03 20:50 Jesper Juhl
2011-02-03 21:01 ` Andrew Morton
2011-02-03 21:17   ` Jesper Juhl
2011-02-03 21:36     ` Andrew Morton [this message]
2011-02-03 21:43       ` Jesper Juhl

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110203133624.cd353dd6.akpm@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=hpa@zytor.com \
    --cc=jj@chaosbits.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mingo@redhat.com \
    --cc=rohit.seth@intel.com \
    --cc=tglx@linutronix.de \
    --cc=tj@kernel.org \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH] Huge TLB: Potential NULL deref in arch/x86/mm/hugetlbpage.c:huge_pmd_share()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).