linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [mmotm:master 97/220] fs/proc/task_mmu.c:1042 pagemap_hugetlb() error: we previously assumed 'vma' could be null (see line 1037)
       [not found] <52fdd350.dwn4aII31EyWlDq9%fengguang.wu@intel.com>
@ 2014-02-14 13:04 ` Fengguang Wu
  2014-02-14 15:09   ` [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
       [not found]   ` <52fe31de.89cfe00a.338f.ffff9a19SMTPIN_ADDED_BROKEN@mx.google.com>
  0 siblings, 2 replies; 4+ messages in thread
From: Fengguang Wu @ 2014-02-14 13:04 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: kbuild-all, Johannes Weiner, Andrew Morton,
	Linux Memory Management List


Hi Naoya,

FYI, there are new smatch warnings show up in

tree:   git://git.cmpxchg.org/linux-mmotm.git master
head:   0363f94bc1c9b81f23ee7d2446331eb288568ea7
commit: 81272031cc2831a3d1abb3c681f1188aa36a1454 [97/220] pagewalk: remove argument hmask from hugetlb_entry()

fs/proc/task_mmu.c:1042 pagemap_hugetlb() error: we previously assumed 'vma' could be null (see line 1037)

vim +/vma +1042 fs/proc/task_mmu.c

d9104d1c Cyrill Gorcunov       2013-09-11  1031  	int flags2;
16fbdce6 Konstantin Khlebnikov 2012-05-10  1032  	pagemap_entry_t pme;
81272031 Naoya Horiguchi       2014-02-13  1033  	unsigned long hmask;
5dc37642 Naoya Horiguchi       2009-12-14  1034  
d9104d1c Cyrill Gorcunov       2013-09-11  1035  	WARN_ON_ONCE(!vma);
d9104d1c Cyrill Gorcunov       2013-09-11  1036  
d9104d1c Cyrill Gorcunov       2013-09-11 @1037  	if (vma && (vma->vm_flags & VM_SOFTDIRTY))
d9104d1c Cyrill Gorcunov       2013-09-11  1038  		flags2 = __PM_SOFT_DIRTY;
d9104d1c Cyrill Gorcunov       2013-09-11  1039  	else
d9104d1c Cyrill Gorcunov       2013-09-11  1040  		flags2 = 0;
d9104d1c Cyrill Gorcunov       2013-09-11  1041  
21a2f342 Naoya Horiguchi       2014-02-13 @1042  	hmask = huge_page_mask(hstate_vma(vma));
5dc37642 Naoya Horiguchi       2009-12-14  1043  	for (; addr != end; addr += PAGE_SIZE) {
116354d1 Naoya Horiguchi       2010-04-06  1044  		int offset = (addr & ~hmask) >> PAGE_SHIFT;
d9104d1c Cyrill Gorcunov       2013-09-11  1045  		huge_pte_to_pagemap_entry(&pme, pm, *pte, offset, flags2);



---
0-DAY kernel build testing backend              Open Source Technology Center
http://lists.01.org/mailman/listinfo/kbuild                 Intel Corporation

--
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>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [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))
  2014-02-14 13:04 ` [mmotm:master 97/220] fs/proc/task_mmu.c:1042 pagemap_hugetlb() error: we previously assumed 'vma' could be null (see line 1037) Fengguang Wu
@ 2014-02-14 15:09   ` Naoya Horiguchi
       [not found]   ` <52fe31de.89cfe00a.338f.ffff9a19SMTPIN_ADDED_BROKEN@mx.google.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Naoya Horiguchi @ 2014-02-14 15:09 UTC (permalink / raw)
  To: Fengguang Wu
  Cc: kbuild-all, Johannes Weiner, Andrew Morton,
	Linux Memory Management List

Hi Fengguang,

On Fri, Feb 14, 2014 at 09:04:50PM +0800, Fengguang Wu wrote:
...
> FYI, there are new smatch warnings show up in
> 
> tree:   git://git.cmpxchg.org/linux-mmotm.git master
> head:   0363f94bc1c9b81f23ee7d2446331eb288568ea7
> commit: 81272031cc2831a3d1abb3c681f1188aa36a1454 [97/220] pagewalk: remove argument hmask from hugetlb_entry()
> 
> fs/proc/task_mmu.c:1042 pagemap_hugetlb() error: we previously assumed 'vma' could be null (see line 1037)
> 
> vim +/vma +1042 fs/proc/task_mmu.c
> 
> d9104d1c Cyrill Gorcunov       2013-09-11  1031  	int flags2;
> 16fbdce6 Konstantin Khlebnikov 2012-05-10  1032  	pagemap_entry_t pme;
> 81272031 Naoya Horiguchi       2014-02-13  1033  	unsigned long hmask;
> 5dc37642 Naoya Horiguchi       2009-12-14  1034  
> d9104d1c Cyrill Gorcunov       2013-09-11  1035  	WARN_ON_ONCE(!vma);
> d9104d1c Cyrill Gorcunov       2013-09-11  1036  
> d9104d1c Cyrill Gorcunov       2013-09-11 @1037  	if (vma && (vma->vm_flags & VM_SOFTDIRTY))
> d9104d1c Cyrill Gorcunov       2013-09-11  1038  		flags2 = __PM_SOFT_DIRTY;
> d9104d1c Cyrill Gorcunov       2013-09-11  1039  	else
> d9104d1c Cyrill Gorcunov       2013-09-11  1040  		flags2 = 0;
> d9104d1c Cyrill Gorcunov       2013-09-11  1041  
> 21a2f342 Naoya Horiguchi       2014-02-13 @1042  	hmask = huge_page_mask(hstate_vma(vma));
> 5dc37642 Naoya Horiguchi       2009-12-14  1043  	for (; addr != end; addr += PAGE_SIZE) {
> 116354d1 Naoya Horiguchi       2010-04-06  1044  		int offset = (addr & ~hmask) >> PAGE_SHIFT;
> d9104d1c Cyrill Gorcunov       2013-09-11  1045  		huge_pte_to_pagemap_entry(&pme, pm, *pte, offset, flags2);

Thanks for reporting, here is a patch.
We never have NULL vma in pagemap_hugetlb(), I added the BUG_ON check.

Thanks,
Naoya Horiguchi
---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Fri, 14 Feb 2014 09:35:06 -0500
Subject: [PATCH] fs/proc/task_mmu.c: assume non-NULL vma in pagemap_hugetlb()

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().

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 fs/proc/task_mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index f819d0d4a0e8..69aed7192254 100644
--- 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);
 
-	if (vma && (vma->vm_flags & VM_SOFTDIRTY))
+	if (vma->vm_flags & VM_SOFTDIRTY)
 		flags2 = __PM_SOFT_DIRTY;
 	else
 		flags2 = 0;
-- 
1.8.5.3

--
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>

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* 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))
       [not found]   ` <52fe31de.89cfe00a.338f.ffff9a19SMTPIN_ADDED_BROKEN@mx.google.com>
@ 2014-02-18 20:44     ` Andrew Morton
  2014-02-18 21:01       ` Naoya Horiguchi
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2014-02-18 20:44 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: Fengguang Wu, kbuild-all, Johannes Weiner,
	Linux Memory Management List

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.


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

--
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>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* 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))
  2014-02-18 20:44     ` Andrew Morton
@ 2014-02-18 21:01       ` Naoya Horiguchi
  0 siblings, 0 replies; 4+ messages in thread
From: Naoya Horiguchi @ 2014-02-18 21:01 UTC (permalink / raw)
  To: akpm; +Cc: fengguang.wu, kbuild-all, hannes, linux-mm

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>

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-02-18 21:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <52fdd350.dwn4aII31EyWlDq9%fengguang.wu@intel.com>
2014-02-14 13:04 ` [mmotm:master 97/220] fs/proc/task_mmu.c:1042 pagemap_hugetlb() error: we previously assumed 'vma' could be null (see line 1037) Fengguang Wu
2014-02-14 15:09   ` [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
     [not found]   ` <52fe31de.89cfe00a.338f.ffff9a19SMTPIN_ADDED_BROKEN@mx.google.com>
2014-02-18 20:44     ` Andrew Morton
2014-02-18 21:01       ` Naoya Horiguchi

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).