[PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[Michal Hocko]

This patch fixes the following NULL ptr dereference caused by
cat /sys/devices/system/memory/memory0/removable:

Pid: 13979, comm: sed Not tainted 3.0.13-0.5-default #1 IBM BladeCenter LS21 -[7971PAM]-/Server Blade
RIP: 0010:[<ffffffff810f41f4>]  [<ffffffff810f41f4>] __count_immobile_pages+0x4/0x100
RSP: 0018:ffff880221c37e48  EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffffea0000000000 RCX: ffffea0000000000
RDX: 0000000000000000 RSI: ffffea0000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000001 R09: 00000000000146b0
R10: 0000000000000000 R11: ffffffff81328980 R12: 0000160000000000
R13: 6db6db6db6db6db7 R14: 0000000000000001 R15: ffffffff81658af0
FS:  00007fc4e8091700(0000) GS:ffff88023fc80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000698 CR3: 000000023027a000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process sed (pid: 13979, threadinfo ffff880221c36000, task ffff88022e788480)
Stack:
 ffffea0000000000 ffffea00001c0000 ffffffff810f4324 ffffffff8113e104
 0000000000000001 0000000000000001 ffff880232a33ac0 ffff880230c25000
 ffff880232a33b28 ffffffff813289c1 ffff88022e7d7d40 ffffffffffffffed
Call Trace:
 [<ffffffff810f4324>] is_pageblock_removable_nolock+0x34/0x40
 [<ffffffff8113e104>] is_mem_section_removable+0x74/0xf0
 [<ffffffff813289c1>] show_mem_removable+0x41/0x70
 [<ffffffff811c053e>] sysfs_read_file+0xfe/0x1c0
 [<ffffffff81150be7>] vfs_read+0xc7/0x130
 [<ffffffff81150d53>] sys_read+0x53/0xa0
 [<ffffffff81448392>] system_call_fastpath+0x16/0x1b
 [<00007fc4e7bdbea0>] 0x7fc4e7bdbe9f
Code: 34 00 0f 1f 44 00 00 48 89 ef be 02 00 00 00 e8 f3 f3 ff ff ba 02 00 00 00 48 89 ee 48 89 df e8 53 f0 ff ff eb b9 90 55 89 d5 53
 2b bf 98 06 00 00 48 89 f3 48 81 ef 00 15 00 00 48 81 ff ff

We are crashing because we are trying to dereference NULL zone which
came from pfn=0 (struct page ffffea0000000000). According to the boot
log this page is marked reserved:
e820 update range: 0000000000000000 - 0000000000010000 (usable) ==> (reserved)

and early_node_map confirms that:
early_node_map[3] active PFN ranges
    1: 0x00000010 -> 0x0000009c
    1: 0x00000100 -> 0x000bffa3
    1: 0x00100000 -> 0x00240000

The problem is that memory_present works in PAGE_SECTION_MASK aligned
blocks so the reserved range sneaks into the the section as well. This
also means that free_area_init_node will not take care of those reserved
pages and they stay uninitialized.

When we try to read the removable status we walk through all available
sections and hope that the zone is valid for all pages in the section.
But this is not true in this case as the zone and nid are not
initialized.
We have only one node in this particular case and it is marked as
node=1 (rather than 0) and that made the problem visible because
page_to_nid will return 0 and there are no zones on the node.

Let's check that the zone is valid and that the given pfn falls into its
boundaries and mark the section not removable. This might cause some
false positives, probably, but we do not have any sane way to find out
whether the page is reserved by the platform or it is just not used for
whatever other reasons.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: David Rientjes <rientjes@google.com>
---
 mm/page_alloc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2b8ba3a..485be89 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
 bool is_pageblock_removable_nolock(struct page *page)
 {
 	struct zone *zone = page_zone(page);
+	unsigned long pfn = page_to_pfn(page);
+
+	/*
+	 * We have to be careful here because we are iterating over memory
+	 * sections which are not zone aware so we might end up outside of
+	 * the zone but still within the section.
+	 */
+	if (!zone || zone->zone_start_pfn > pfn ||
+			zone->zone_start_pfn + zone->spanned_pages <= pfn)
+		return false;
+
 	return __count_immobile_pages(zone, page, 0);
 }
 
-- 
1.7.7.3

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[David Rientjes]

On Tue, 10 Jan 2012, Michal Hocko wrote:

> This patch fixes the following NULL ptr dereference caused by
> cat /sys/devices/system/memory/memory0/removable:
> 
> Pid: 13979, comm: sed Not tainted 3.0.13-0.5-default #1 IBM BladeCenter LS21 -[7971PAM]-/Server Blade
> RIP: 0010:[<ffffffff810f41f4>]  [<ffffffff810f41f4>] __count_immobile_pages+0x4/0x100
> RSP: 0018:ffff880221c37e48  EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffffea0000000000 RCX: ffffea0000000000
> RDX: 0000000000000000 RSI: ffffea0000000000 RDI: 0000000000000000
> RBP: 0000000000000000 R08: 0000000000000001 R09: 00000000000146b0
> R10: 0000000000000000 R11: ffffffff81328980 R12: 0000160000000000
> R13: 6db6db6db6db6db7 R14: 0000000000000001 R15: ffffffff81658af0
> FS:  00007fc4e8091700(0000) GS:ffff88023fc80000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000698 CR3: 000000023027a000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process sed (pid: 13979, threadinfo ffff880221c36000, task ffff88022e788480)
> Stack:
>  ffffea0000000000 ffffea00001c0000 ffffffff810f4324 ffffffff8113e104
>  0000000000000001 0000000000000001 ffff880232a33ac0 ffff880230c25000
>  ffff880232a33b28 ffffffff813289c1 ffff88022e7d7d40 ffffffffffffffed
> Call Trace:
>  [<ffffffff810f4324>] is_pageblock_removable_nolock+0x34/0x40
>  [<ffffffff8113e104>] is_mem_section_removable+0x74/0xf0
>  [<ffffffff813289c1>] show_mem_removable+0x41/0x70
>  [<ffffffff811c053e>] sysfs_read_file+0xfe/0x1c0
>  [<ffffffff81150be7>] vfs_read+0xc7/0x130
>  [<ffffffff81150d53>] sys_read+0x53/0xa0
>  [<ffffffff81448392>] system_call_fastpath+0x16/0x1b
>  [<00007fc4e7bdbea0>] 0x7fc4e7bdbe9f
> Code: 34 00 0f 1f 44 00 00 48 89 ef be 02 00 00 00 e8 f3 f3 ff ff ba 02 00 00 00 48 89 ee 48 89 df e8 53 f0 ff ff eb b9 90 55 89 d5 53
>  2b bf 98 06 00 00 48 89 f3 48 81 ef 00 15 00 00 48 81 ff ff
> 
> We are crashing because we are trying to dereference NULL zone which
> came from pfn=0 (struct page ffffea0000000000). According to the boot
> log this page is marked reserved:
> e820 update range: 0000000000000000 - 0000000000010000 (usable) ==> (reserved)
> 
> and early_node_map confirms that:
> early_node_map[3] active PFN ranges
>     1: 0x00000010 -> 0x0000009c
>     1: 0x00000100 -> 0x000bffa3
>     1: 0x00100000 -> 0x00240000
> 
> The problem is that memory_present works in PAGE_SECTION_MASK aligned
> blocks so the reserved range sneaks into the the section as well. This
> also means that free_area_init_node will not take care of those reserved
> pages and they stay uninitialized.
> 
> When we try to read the removable status we walk through all available
> sections and hope that the zone is valid for all pages in the section.
> But this is not true in this case as the zone and nid are not
> initialized.
> We have only one node in this particular case and it is marked as
> node=1 (rather than 0) and that made the problem visible because
> page_to_nid will return 0 and there are no zones on the node.
> 
> Let's check that the zone is valid and that the given pfn falls into its
> boundaries and mark the section not removable. This might cause some
> false positives, probably, but we do not have any sane way to find out
> whether the page is reserved by the platform or it is just not used for
> whatever other reasons.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: David Rientjes <rientjes@google.com>
> ---
>  mm/page_alloc.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 2b8ba3a..485be89 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
>  bool is_pageblock_removable_nolock(struct page *page)
>  {
>  	struct zone *zone = page_zone(page);
> +	unsigned long pfn = page_to_pfn(page);
> +
> +	/*
> +	 * We have to be careful here because we are iterating over memory
> +	 * sections which are not zone aware so we might end up outside of
> +	 * the zone but still within the section.
> +	 */
> +	if (!zone || zone->zone_start_pfn > pfn ||
> +			zone->zone_start_pfn + zone->spanned_pages <= pfn)
> +		return false;
> +
>  	return __count_immobile_pages(zone, page, 0);
>  }
>  

This seems partially bogus, why would

	page_zone(page)->zone_start_pfn > page_to_pfn(page) ||
	page_zone(page)->zone_start_pfn + page_zone(page)->spanned_pages <= page_to_pfn(page)

ever be true?  That would certainly mean that the struct zone is corrupted 
and seems to be unnecessary to fix the problem you're addressing.

I think this should be handled in is_mem_section_removable() on the pfn 
rather than using the struct page in is_pageblock_removable_nolock() and 
converting back and forth.  We should make sure that any page passed to 
is_pageblock_removable_nolock() is valid.

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[Michal Hocko]

On Tue 10-01-12 13:31:08, David Rientjes wrote:
> On Tue, 10 Jan 2012, Michal Hocko wrote:
[...]
> >  mm/page_alloc.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 2b8ba3a..485be89 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> >  bool is_pageblock_removable_nolock(struct page *page)
> >  {
> >  	struct zone *zone = page_zone(page);
> > +	unsigned long pfn = page_to_pfn(page);
> > +
> > +	/*
> > +	 * We have to be careful here because we are iterating over memory
> > +	 * sections which are not zone aware so we might end up outside of
> > +	 * the zone but still within the section.
> > +	 */
> > +	if (!zone || zone->zone_start_pfn > pfn ||
> > +			zone->zone_start_pfn + zone->spanned_pages <= pfn)
> > +		return false;
> > +
> >  	return __count_immobile_pages(zone, page, 0);
> >  }
> >  
> 
> This seems partially bogus, why would
> 
> 	page_zone(page)->zone_start_pfn > page_to_pfn(page) ||
> 	page_zone(page)->zone_start_pfn + page_zone(page)->spanned_pages <= page_to_pfn(page)
> 
> ever be true?  That would certainly mean that the struct zone is corrupted 
> and seems to be unnecessary to fix the problem you're addressing.

Not really. Consider the case when the node 0 is present. Uninitialized
page would lead to node=0, zone=0 and then we have to check for the zone
boundaries.

> I think this should be handled in is_mem_section_removable() on the pfn 
> rather than using the struct page in is_pageblock_removable_nolock() and 
> converting back and forth.  We should make sure that any page passed to 
> is_pageblock_removable_nolock() is valid.

Yes, I do not like pfn->page->pfn dance as well and in fact I do not
have a strong opinion which one is better. I just put it at the place
where we care about zone to be more obvious. If others think that I
should move the check one level higher I'll do that. I just think this
is more obvious.

Thanks for your comments.
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[Andrew Morton]

On Tue, 10 Jan 2012 17:30:22 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> This patch fixes the following NULL ptr dereference caused by
> cat /sys/devices/system/memory/memory0/removable:

Which is world-readable, I assume?

> ...
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
>  bool is_pageblock_removable_nolock(struct page *page)
>  {
>  	struct zone *zone = page_zone(page);
> +	unsigned long pfn = page_to_pfn(page);
> +
> +	/*
> +	 * We have to be careful here because we are iterating over memory
> +	 * sections which are not zone aware so we might end up outside of
> +	 * the zone but still within the section.
> +	 */
> +	if (!zone || zone->zone_start_pfn > pfn ||
> +			zone->zone_start_pfn + zone->spanned_pages <= pfn)
> +		return false;
> +
>  	return __count_immobile_pages(zone, page, 0);
>  }

So I propose that we backport it into -stable?

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[KAMEZAWA Hiroyuki]

On Wed, 11 Jan 2012 09:48:02 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Tue 10-01-12 13:31:08, David Rientjes wrote:
> > On Tue, 10 Jan 2012, Michal Hocko wrote:
> [...]
> > >  mm/page_alloc.c |   11 +++++++++++
> > >  1 files changed, 11 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 2b8ba3a..485be89 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> > >  bool is_pageblock_removable_nolock(struct page *page)
> > >  {
> > >  	struct zone *zone = page_zone(page);
> > > +	unsigned long pfn = page_to_pfn(page);
> > > +

Hmm, I don't like to use page_zone() when we know the page may not be initialized.
Shouldn't we add

	if (!node_online(page_to_nid(page))
		return false;
?

But...hmm. I think we should return 'true' here for removing a section with a hole
finally....(Now, false will be safe.)



> > > +	/*
> > > +	 * We have to be careful here because we are iterating over memory
> > > +	 * sections which are not zone aware so we might end up outside of
> > > +	 * the zone but still within the section.
> > > +	 */
> > > +	if (!zone || zone->zone_start_pfn > pfn ||
> > > +			zone->zone_start_pfn + zone->spanned_pages <= pfn)
> > > +		return false;
> > > +
> > >  	return __count_immobile_pages(zone, page, 0);
> > >  }
> > >  
> > 
> > This seems partially bogus, why would
> > 
> > 	page_zone(page)->zone_start_pfn > page_to_pfn(page) ||
> > 	page_zone(page)->zone_start_pfn + page_zone(page)->spanned_pages <= page_to_pfn(page)
> > 
> > ever be true?  That would certainly mean that the struct zone is corrupted 
> > and seems to be unnecessary to fix the problem you're addressing.
> 
> Not really. Consider the case when the node 0 is present. Uninitialized
> page would lead to node=0, zone=0 and then we have to check for the zone
> boundaries.
> 



> > I think this should be handled in is_mem_section_removable() on the pfn 
> > rather than using the struct page in is_pageblock_removable_nolock() and 
> > converting back and forth.  We should make sure that any page passed to 
> > is_pageblock_removable_nolock() is valid.
> 
> Yes, I do not like pfn->page->pfn dance as well and in fact I do not
> have a strong opinion which one is better. I just put it at the place
> where we care about zone to be more obvious. If others think that I
> should move the check one level higher I'll do that. I just think this
> is more obvious.
> 
Hmm, mem_section and pageblock is a different chunk...
And, IIUC, in some IBM machines, section may includes several zones.
Please taking care of that if you move this to is_mem_section_removable()...

Thanks,
-Kame

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[Michal Hocko]

On Wed 11-01-12 14:34:39, Andrew Morton wrote:
> On Tue, 10 Jan 2012 17:30:22 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > This patch fixes the following NULL ptr dereference caused by
> > cat /sys/devices/system/memory/memory0/removable:
> 
> Which is world-readable, I assume?

Right. But considering that we haven't seen any report like that it
seems that the HW is rather rare

> 
> > ...
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> >  bool is_pageblock_removable_nolock(struct page *page)
> >  {
> >  	struct zone *zone = page_zone(page);
> > +	unsigned long pfn = page_to_pfn(page);
> > +
> > +	/*
> > +	 * We have to be careful here because we are iterating over memory
> > +	 * sections which are not zone aware so we might end up outside of
> > +	 * the zone but still within the section.
> > +	 */
> > +	if (!zone || zone->zone_start_pfn > pfn ||
> > +			zone->zone_start_pfn + zone->spanned_pages <= pfn)
> > +		return false;
> > +
> >  	return __count_immobile_pages(zone, page, 0);
> >  }
> 
> So I propose that we backport it into -stable?

Agreed.

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[Michal Hocko]

On Thu 12-01-12 11:17:02, KAMEZAWA Hiroyuki wrote:
> On Wed, 11 Jan 2012 09:48:02 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Tue 10-01-12 13:31:08, David Rientjes wrote:
> > > On Tue, 10 Jan 2012, Michal Hocko wrote:
> > [...]
> > > >  mm/page_alloc.c |   11 +++++++++++
> > > >  1 files changed, 11 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > index 2b8ba3a..485be89 100644
> > > > --- a/mm/page_alloc.c
> > > > +++ b/mm/page_alloc.c
> > > > @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> > > >  bool is_pageblock_removable_nolock(struct page *page)
> > > >  {
> > > >  	struct zone *zone = page_zone(page);
> > > > +	unsigned long pfn = page_to_pfn(page);
> > > > +
> 
> Hmm, I don't like to use page_zone() when we know the page may not be initialized.
> Shouldn't we add
> 
> 	if (!node_online(page_to_nid(page))
> 		return false;
> ?

How is this different? The node won't be initialized in page flags as
well.

> But...hmm. I think we should return 'true' here for removing a section with a hole
> finally....(Now, false will be safe.)

Those pages are reserved (for BIOS I guess) in this particular case so I
do not think it is safe to claim that the block is removable. Or am I
missing something?

[...]
> > > I think this should be handled in is_mem_section_removable() on the pfn 
> > > rather than using the struct page in is_pageblock_removable_nolock() and 
> > > converting back and forth.  We should make sure that any page passed to 
> > > is_pageblock_removable_nolock() is valid.
> > 
> > Yes, I do not like pfn->page->pfn dance as well and in fact I do not
> > have a strong opinion which one is better. I just put it at the place
> > where we care about zone to be more obvious. If others think that I
> > should move the check one level higher I'll do that. I just think this
> > is more obvious.
> > 
> Hmm, mem_section and pageblock is a different chunk...
> And, IIUC, in some IBM machines, section may includes several zones.
> Please taking care of that if you move this to is_mem_section_removable()...

Thanks for pointing this out. 

> 
> Thanks,
> -Kame

Thanks for comments.
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[KAMEZAWA Hiroyuki]

On Thu, 12 Jan 2012 09:27:22 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 12-01-12 11:17:02, KAMEZAWA Hiroyuki wrote:
> > On Wed, 11 Jan 2012 09:48:02 +0100
> > Michal Hocko <mhocko@suse.cz> wrote:
> > 
> > > On Tue 10-01-12 13:31:08, David Rientjes wrote:
> > > > On Tue, 10 Jan 2012, Michal Hocko wrote:
> > > [...]
> > > > >  mm/page_alloc.c |   11 +++++++++++
> > > > >  1 files changed, 11 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > > index 2b8ba3a..485be89 100644
> > > > > --- a/mm/page_alloc.c
> > > > > +++ b/mm/page_alloc.c
> > > > > @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> > > > >  bool is_pageblock_removable_nolock(struct page *page)
> > > > >  {
> > > > >  	struct zone *zone = page_zone(page);
> > > > > +	unsigned long pfn = page_to_pfn(page);
> > > > > +
> > 
> > Hmm, I don't like to use page_zone() when we know the page may not be initialized.
> > Shouldn't we add
> > 
> > 	if (!node_online(page_to_nid(page))
> > 		return false;
> > ?
> 
> How is this different? The node won't be initialized in page flags as
> well.
> 

page_zone(page) is
==
static inline struct zone *page_zone(const struct page *page)
{
        return &NODE_DATA(page_to_nid(page))->node_zones[page_zonenum(page)];
}
==

Then, if the page is unitialized, 

   &(NODE_DATA(0)->node_zones[0])

If NODE_DATA(0) is NULL, node_zones[0] is NULL just because zone array is placed
on the top of struct pglist_data.

I never think someone may change the layout but...Hmm, just a nitpick.
please do as you like.


> > But...hmm. I think we should return 'true' here for removing a section with a hole
> > finally....(Now, false will be safe.)
> 
> Those pages are reserved (for BIOS I guess) in this particular case so I
> do not think it is safe to claim that the block is removable. Or am I
> missing something?
> 

We can't know it's reserved by BIOS or it's just a memory hole by the fact
the page wasn't initialized.


Thanks,
-Kame

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[Michal Hocko]

On Thu 12-01-12 17:35:36, KAMEZAWA Hiroyuki wrote:
> On Thu, 12 Jan 2012 09:27:22 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Thu 12-01-12 11:17:02, KAMEZAWA Hiroyuki wrote:
> > > On Wed, 11 Jan 2012 09:48:02 +0100
> > > Michal Hocko <mhocko@suse.cz> wrote:
> > > 
> > > > On Tue 10-01-12 13:31:08, David Rientjes wrote:
> > > > > On Tue, 10 Jan 2012, Michal Hocko wrote:
> > > > [...]
> > > > > >  mm/page_alloc.c |   11 +++++++++++
> > > > > >  1 files changed, 11 insertions(+), 0 deletions(-)
> > > > > > 
> > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > > > index 2b8ba3a..485be89 100644
> > > > > > --- a/mm/page_alloc.c
> > > > > > +++ b/mm/page_alloc.c
> > > > > > @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> > > > > >  bool is_pageblock_removable_nolock(struct page *page)
> > > > > >  {
> > > > > >  	struct zone *zone = page_zone(page);
> > > > > > +	unsigned long pfn = page_to_pfn(page);
> > > > > > +
> > > 
> > > Hmm, I don't like to use page_zone() when we know the page may not be initialized.
> > > Shouldn't we add
> > > 
> > > 	if (!node_online(page_to_nid(page))
> > > 		return false;
> > > ?
> > 
> > How is this different? The node won't be initialized in page flags as
> > well.
> > 
> 
> page_zone(page) is
> ==
> static inline struct zone *page_zone(const struct page *page)
> {
>         return &NODE_DATA(page_to_nid(page))->node_zones[page_zonenum(page)];
> }
> ==
> 
> Then, if the page is unitialized, 
> 
>    &(NODE_DATA(0)->node_zones[0])
> 
> If NODE_DATA(0) is NULL, node_zones[0] is NULL just because zone array is placed
> on the top of struct pglist_data.
> 
> I never think someone may change the layout but...Hmm, just a nitpick.
> please do as you like.

Yes, fair point. See the follow up patch bellow.

> > > But...hmm. I think we should return 'true' here for removing a section with a hole
> > > finally....(Now, false will be safe.)
> > 
> > Those pages are reserved (for BIOS I guess) in this particular case so I
> > do not think it is safe to claim that the block is removable. Or am I
> > missing something?
> > 
> 
> We can't know it's reserved by BIOS or it's just a memory hole by the fact
> the page wasn't initialized.

OK, so then we should return false to mark to block non removable,
right?
--- 
>From 04f74e6f0ebf28f61650d63f8884e8855fb21b55 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Thu, 12 Jan 2012 10:19:04 +0100
Subject: [PATCH] mm: __count_immobile_pages make sure the node is online

page_zone requires to have an online node otherwise we are accessing
NULL NODE_DATA. This is not an issue at the moment because node_zones
are located at the structure beginning but this might change in the
future so better be careful about that.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/page_alloc.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 485be89..c6fb8ea 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5607,14 +5607,21 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
 
 bool is_pageblock_removable_nolock(struct page *page)
 {
-	struct zone *zone = page_zone(page);
-	unsigned long pfn = page_to_pfn(page);
+	struct zone *zone;
+	unsigned long pfn;
 
 	/*
 	 * We have to be careful here because we are iterating over memory
 	 * sections which are not zone aware so we might end up outside of
 	 * the zone but still within the section.
+	 * We have to take care about the node as well. If the node is offline
+	 * its NODE_DATA will be NULL - see page_zone.
 	 */
+	if (!node_online(page_to_nid(page)))
+		return false;
+
+	zone = page_zone(page);
+	pfn = page_to_pfn(page);
 	if (!zone || zone->zone_start_pfn > pfn ||
 			zone->zone_start_pfn + zone->spanned_pages <= pfn)
 		return false;
-- 
1.7.7.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages

[KAMEZAWA Hiroyuki]

On Thu, 12 Jan 2012 10:23:14 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 12-01-12 17:35:36, KAMEZAWA Hiroyuki wrote:
> > On Thu, 12 Jan 2012 09:27:22 +0100
> > Michal Hocko <mhocko@suse.cz> wrote:
> > 
> > > On Thu 12-01-12 11:17:02, KAMEZAWA Hiroyuki wrote:
> > > > On Wed, 11 Jan 2012 09:48:02 +0100
> > > > Michal Hocko <mhocko@suse.cz> wrote:
> > > > 
> > > > > On Tue 10-01-12 13:31:08, David Rientjes wrote:
> > > > > > On Tue, 10 Jan 2012, Michal Hocko wrote:
> > > > > [...]
> > > > > > >  mm/page_alloc.c |   11 +++++++++++
> > > > > > >  1 files changed, 11 insertions(+), 0 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > > > > index 2b8ba3a..485be89 100644
> > > > > > > --- a/mm/page_alloc.c
> > > > > > > +++ b/mm/page_alloc.c
> > > > > > > @@ -5608,6 +5608,17 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> > > > > > >  bool is_pageblock_removable_nolock(struct page *page)
> > > > > > >  {
> > > > > > >  	struct zone *zone = page_zone(page);
> > > > > > > +	unsigned long pfn = page_to_pfn(page);
> > > > > > > +
> > > > 
> > > > Hmm, I don't like to use page_zone() when we know the page may not be initialized.
> > > > Shouldn't we add
> > > > 
> > > > 	if (!node_online(page_to_nid(page))
> > > > 		return false;
> > > > ?
> > > 
> > > How is this different? The node won't be initialized in page flags as
> > > well.
> > > 
> > 
> > page_zone(page) is
> > ==
> > static inline struct zone *page_zone(const struct page *page)
> > {
> >         return &NODE_DATA(page_to_nid(page))->node_zones[page_zonenum(page)];
> > }
> > ==
> > 
> > Then, if the page is unitialized, 
> > 
> >    &(NODE_DATA(0)->node_zones[0])
> > 
> > If NODE_DATA(0) is NULL, node_zones[0] is NULL just because zone array is placed
> > on the top of struct pglist_data.
> > 
> > I never think someone may change the layout but...Hmm, just a nitpick.
> > please do as you like.
> 
> Yes, fair point. See the follow up patch bellow.
> 
> > > > But...hmm. I think we should return 'true' here for removing a section with a hole
> > > > finally....(Now, false will be safe.)
> > > 
> > > Those pages are reserved (for BIOS I guess) in this particular case so I
> > > do not think it is safe to claim that the block is removable. Or am I
> > > missing something?
> > > 
> > 
> > We can't know it's reserved by BIOS or it's just a memory hole by the fact
> > the page wasn't initialized.
> 
> OK, so then we should return false to mark to block non removable,
> right?

Ok.

> --- 
> From 04f74e6f0ebf28f61650d63f8884e8855fb21b55 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Thu, 12 Jan 2012 10:19:04 +0100
> Subject: [PATCH] mm: __count_immobile_pages make sure the node is online
> 
> page_zone requires to have an online node otherwise we are accessing
> NULL NODE_DATA. This is not an issue at the moment because node_zones
> are located at the structure beginning but this might change in the
> future so better be careful about that.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  mm/page_alloc.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 485be89..c6fb8ea 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5607,14 +5607,21 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
>  
>  bool is_pageblock_removable_nolock(struct page *page)
>  {
> -	struct zone *zone = page_zone(page);
> -	unsigned long pfn = page_to_pfn(page);
> +	struct zone *zone;
> +	unsigned long pfn;
>  
>  	/*
>  	 * We have to be careful here because we are iterating over memory
>  	 * sections which are not zone aware so we might end up outside of
>  	 * the zone but still within the section.
> +	 * We have to take care about the node as well. If the node is offline
> +	 * its NODE_DATA will be NULL - see page_zone.
>  	 */
> +	if (!node_online(page_to_nid(page)))
> +		return false;
> +
> +	zone = page_zone(page);
> +	pfn = page_to_pfn(page);
>  	if (!zone || zone->zone_start_pfn > pfn ||
>  			zone->zone_start_pfn + zone->spanned_pages <= pfn)
>  		return false;

!zone check can be removed because of node_online() check.

Thanks,
-Kame

[PATCH] mm: __count_immobile_pages make sure the node is online

[Michal Hocko]

On Thu 12-01-12 18:33:23, KAMEZAWA Hiroyuki wrote:
[...]
> > From 04f74e6f0ebf28f61650d63f8884e8855fb21b55 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Thu, 12 Jan 2012 10:19:04 +0100
> > Subject: [PATCH] mm: __count_immobile_pages make sure the node is online
> > 
> > page_zone requires to have an online node otherwise we are accessing
> > NULL NODE_DATA. This is not an issue at the moment because node_zones
> > are located at the structure beginning but this might change in the
> > future so better be careful about that.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  mm/page_alloc.c |   11 +++++++++--
> >  1 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 485be89..c6fb8ea 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5607,14 +5607,21 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
> >  
> >  bool is_pageblock_removable_nolock(struct page *page)
> >  {
> > -	struct zone *zone = page_zone(page);
> > -	unsigned long pfn = page_to_pfn(page);
> > +	struct zone *zone;
> > +	unsigned long pfn;
> >  
> >  	/*
> >  	 * We have to be careful here because we are iterating over memory
> >  	 * sections which are not zone aware so we might end up outside of
> >  	 * the zone but still within the section.
> > +	 * We have to take care about the node as well. If the node is offline
> > +	 * its NODE_DATA will be NULL - see page_zone.
> >  	 */
> > +	if (!node_online(page_to_nid(page)))
> > +		return false;
> > +
> > +	zone = page_zone(page);
> > +	pfn = page_to_pfn(page);
> >  	if (!zone || zone->zone_start_pfn > pfn ||
> >  			zone->zone_start_pfn + zone->spanned_pages <= pfn)
> >  		return false;
> 
> !zone check can be removed because of node_online() check.

Yes you are right. Fixed bellow:
---
>From 39de8df13532150fc4518dad0cb3f6fd88745b8a Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Thu, 12 Jan 2012 10:19:04 +0100
Subject: [PATCH] mm: __count_immobile_pages make sure the node is online

page_zone requires to have an online node otherwise we are accessing
NULL NODE_DATA. This is not an issue at the moment because node_zones
are located at the structure beginning but this might change in the
future so better be careful about that.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/page_alloc.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 485be89..b09e8c2 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5607,15 +5607,22 @@ __count_immobile_pages(struct zone *zone, struct page *page, int count)
 
 bool is_pageblock_removable_nolock(struct page *page)
 {
-	struct zone *zone = page_zone(page);
-	unsigned long pfn = page_to_pfn(page);
+	struct zone *zone;
+	unsigned long pfn;
 
 	/*
 	 * We have to be careful here because we are iterating over memory
 	 * sections which are not zone aware so we might end up outside of
 	 * the zone but still within the section.
+	 * We have to take care about the node as well. If the node is offline
+	 * its NODE_DATA will be NULL - see page_zone.
 	 */
-	if (!zone || zone->zone_start_pfn > pfn ||
+	if (!node_online(page_to_nid(page)))
+		return false;
+
+	zone = page_zone(page);
+	pfn = page_to_pfn(page);
+	if (zone->zone_start_pfn > pfn ||
 			zone->zone_start_pfn + zone->spanned_pages <= pfn)
 		return false;
 
-- 
1.7.7.3


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] mm: __count_immobile_pages make sure the node is online

[Mel Gorman]

On Thu, Jan 12, 2012 at 11:05:21AM +0100, Michal Hocko wrote:
> From 39de8df13532150fc4518dad0cb3f6fd88745b8a Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Thu, 12 Jan 2012 10:19:04 +0100
> Subject: [PATCH] mm: __count_immobile_pages make sure the node is online
> 
> page_zone requires to have an online node otherwise we are accessing
> NULL NODE_DATA. This is not an issue at the moment because node_zones
> are located at the structure beginning but this might change in the
> future so better be careful about that.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Mel Gorman <mgorman@suse.de>

Be aware that this is not the version picked up by Andrew. It would
not hurt to resend as V2 with a changelog and a note saying it replaces
mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch in mmotm.
This is just in case the wrong one gets merged due to this thread
getting lost in the noise of Andrew's inbox.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm: __count_immobile_pages make sure the node is online

[Michal Hocko]

On Thu 12-01-12 11:14:15, Mel Gorman wrote:
> On Thu, Jan 12, 2012 at 11:05:21AM +0100, Michal Hocko wrote:
> > From 39de8df13532150fc4518dad0cb3f6fd88745b8a Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Thu, 12 Jan 2012 10:19:04 +0100
> > Subject: [PATCH] mm: __count_immobile_pages make sure the node is online
> > 
> > page_zone requires to have an online node otherwise we are accessing
> > NULL NODE_DATA. This is not an issue at the moment because node_zones
> > are located at the structure beginning but this might change in the
> > future so better be careful about that.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Acked-by: Mel Gorman <mgorman@suse.de>

Thanks

> Be aware that this is not the version picked up by Andrew. It would
> not hurt to resend as V2 with a changelog and a note saying it replaces
> mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch in mmotm.
> This is just in case the wrong one gets merged due to this thread
> getting lost in the noise of Andrew's inbox.

I guess that both of them should be merged. This one as a follow up fix.
That's why I made it separate and didn't repost the whole patch.
The original patch is much more obvious why it fixes the issue this one
is to have everything clear.
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] mm: __count_immobile_pages make sure the node is online

[Andrew Morton]

On Thu, 12 Jan 2012 13:35:55 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 12-01-12 11:14:15, Mel Gorman wrote:
> > On Thu, Jan 12, 2012 at 11:05:21AM +0100, Michal Hocko wrote:
> 
> > Be aware that this is not the version picked up by Andrew. It would
> > not hurt to resend as V2 with a changelog and a note saying it replaces
> > mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch in mmotm.

They're rather different things? According to the changelogs,
mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch fixes a
known-to-occur oops. 
mm-__count_immobile_pages-make-sure-the-node-is-online.patch fixes a
bug which might happen in the future if we change the node_zones layut?

So I'm thinking that
mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch is 3.3 and
-stable material, whereas this patch
(mm-__count_immobile_pages-make-sure-the-node-is-online.patch) is 3.3
material.  (Actually, it's 3.4 material which I shall stuff into 3.3
because the amount of MM material which we're putting into 3.3 is just
off the charts and I fear that 3.4 will be similar)

> > This is just in case the wrong one gets merged due to this thread
> > getting lost in the noise of Andrew's inbox.

It won't get lost, but there's a higher-than-usual chance of delays if
the patch happens during the merge window: if I see a lengthy patch
thread I'll move it into my to-apply folder for consideration later on.
So I will look at it, but it might be after the merge window.  We can
still apply a fix after the merge window of course, but this all might
end up leaving buggy code in the tree for longer than we'd like.

Re: [PATCH] mm: __count_immobile_pages make sure the node is online

[Michal Hocko]

On Thu 12-01-12 13:26:29, Andrew Morton wrote:
> On Thu, 12 Jan 2012 13:35:55 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Thu 12-01-12 11:14:15, Mel Gorman wrote:
> > > On Thu, Jan 12, 2012 at 11:05:21AM +0100, Michal Hocko wrote:
> > 
> > > Be aware that this is not the version picked up by Andrew. It would
> > > not hurt to resend as V2 with a changelog and a note saying it replaces
> > > mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch in mmotm.
> 
> They're rather different things? According to the changelogs,
> mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch fixes a
> known-to-occur oops. 
> mm-__count_immobile_pages-make-sure-the-node-is-online.patch fixes a
> bug which might happen in the future if we change the node_zones layut?

Yes, and that's the reason why I posted it as a separate patch. I could
also have squashed both patches together and it would work the same way
but I think that the zone issue wouldn't be that obvious.

> 
> So I'm thinking that
> mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch is 3.3 and
> -stable material, whereas this patch
> (mm-__count_immobile_pages-make-sure-the-node-is-online.patch) is 3.3
> material.  (Actually, it's 3.4 material which I shall stuff into 3.3
> because the amount of MM material which we're putting into 3.3 is just
> off the charts and I fear that 3.4 will be similar)

Agreed

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] mm: __count_immobile_pages make sure the node is online

[Mel Gorman]

On Thu, Jan 12, 2012 at 01:26:29PM -0800, Andrew Morton wrote:
> On Thu, 12 Jan 2012 13:35:55 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Thu 12-01-12 11:14:15, Mel Gorman wrote:
> > > On Thu, Jan 12, 2012 at 11:05:21AM +0100, Michal Hocko wrote:
> > 
> > > Be aware that this is not the version picked up by Andrew. It would
> > > not hurt to resend as V2 with a changelog and a note saying it replaces
> > > mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch in mmotm.
> 
> They're rather different things? According to the changelogs,
> mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch fixes a
> known-to-occur oops. 
> mm-__count_immobile_pages-make-sure-the-node-is-online.patch fixes a
> bug which might happen in the future if we change the node_zones layut?
> 

That's fine. I thought the second patch was so closely related that
they would be collapsed together and both sent to stable. There is no
particular advantage to doing this and keeping them separate is fine.

FWIW, my ack applied to both patches in that case.

> So I'm thinking that
> mm-fix-null-ptr-dereference-in-__count_immobile_pages.patch is 3.3 and
> -stable material, whereas this patch
> (mm-__count_immobile_pages-make-sure-the-node-is-online.patch) is 3.3
> material.  (Actually, it's 3.4 material which I shall stuff into 3.3
> because the amount of MM material which we're putting into 3.3 is just
> off the charts and I fear that 3.4 will be similar)
> 

Ok, sounds good. Thanks.

> > > This is just in case the wrong one gets merged due to this thread
> > > getting lost in the noise of Andrew's inbox.
> 
> It won't get lost, but there's a higher-than-usual chance of delays if
> the patch happens during the merge window: if I see a lengthy patch
> thread I'll move it into my to-apply folder for consideration later on.
> So I will look at it, but it might be after the merge window.  We can
> still apply a fix after the merge window of course, but this all might
> end up leaving buggy code in the tree for longer than we'd like.
> 

Understood.

-- 
Mel Gorman
SUSE Labs