Re: [PATCH] fs/proc/task_mmu.c: assume non-NULL vma in pagemap_hugetlb() (Re: [mmotm:master 97/220] fs/proc/task_mmu.c:1042 pagemap_hugetlb() error: we previously assumed 'vma' could be null (see line 1037))

[Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>]

On Tue, Feb 18, 2014 at 12:44:00PM -0800, Andrew Morton wrote:
> On Fri, 14 Feb 2014 10:09:58 -0500 Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
> 
> > Fengguang reported smatch error about potential NULL pointer access.
> > 
> > In updated page table walker, we never run ->hugetlb_entry() callback
> > on the address without vma. This is because __walk_page_range() checks
> > it in advance. So we can assume non-NULL vma in pagemap_hugetlb().
> > 
> > ...
> >
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1032,9 +1032,9 @@ static int pagemap_hugetlb(pte_t *pte, unsigned long addr, unsigned long end,
> >  	pagemap_entry_t pme;
> >  	unsigned long hmask;
> >  
> > -	WARN_ON_ONCE(!vma);
> > +	BUG_ON(!vma);
> 
> Let's just remove it altogether.
> 
> > -	if (vma && (vma->vm_flags & VM_SOFTDIRTY))
> > +	if (vma->vm_flags & VM_SOFTDIRTY)
> 
> The null deref oops will provide us the same info as the BUG_ON.  It
> will require a *little* more thinking to work out that `vma' was NULL,
> but it will be pretty obvious.

Yes, I agree. pagemap_hugetlb() is callback and never inlined, so we will
have an info like 'NULL pointer access on pagemap_hugetlb()' with call trace,
which is enough to pinpoint the problem.

> 
> This requires knowing offsetof(vm_area_struct, vm_flags).  I use a gdb macro:
> 
> define offsetof
> 	set $off = &(((struct $arg0 *)0)->$arg1)
> 	printf "%d 0x%x\n", $off, $off
> end
> 
> akpm3:/usr/src/25> gdb fs/proc/task_mmu.o
> ...
> (gdb) offsetof vm_area_struct vm_flags
> 80 0x50

Nice note, thank you.

Naoya

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>