* [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages @ 2012-01-10 16:30 Michal Hocko 2012-01-10 21:31 ` David Rientjes 2012-01-11 22:34 ` [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages Andrew Morton 0 siblings, 2 replies; 16+ messages in thread From: Michal Hocko @ 2012-01-10 16:30 UTC (permalink / raw) To: linux-mm Cc: linux-kernel, Andrew Morton, Mel Gorman, KAMEZAWA Hiroyuki, Andrea Arcangeli, David Rientjes 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-10 16:30 [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages Michal Hocko @ 2012-01-10 21:31 ` David Rientjes 2012-01-11 8:48 ` Michal Hocko 2012-01-11 22:34 ` [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages Andrew Morton 1 sibling, 1 reply; 16+ messages in thread From: David Rientjes @ 2012-01-10 21:31 UTC (permalink / raw) To: Michal Hocko Cc: linux-mm, linux-kernel, Andrew Morton, Mel Gorman, KAMEZAWA Hiroyuki, Andrea Arcangeli 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-10 21:31 ` David Rientjes @ 2012-01-11 8:48 ` Michal Hocko 2012-01-12 2:17 ` KAMEZAWA Hiroyuki 0 siblings, 1 reply; 16+ messages in thread From: Michal Hocko @ 2012-01-11 8:48 UTC (permalink / raw) To: David Rientjes Cc: linux-mm, linux-kernel, Andrew Morton, Mel Gorman, KAMEZAWA Hiroyuki, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-11 8:48 ` Michal Hocko @ 2012-01-12 2:17 ` KAMEZAWA Hiroyuki 2012-01-12 8:27 ` Michal Hocko 0 siblings, 1 reply; 16+ messages in thread From: KAMEZAWA Hiroyuki @ 2012-01-12 2:17 UTC (permalink / raw) To: Michal Hocko Cc: David Rientjes, linux-mm, linux-kernel, Andrew Morton, Mel Gorman, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-12 2:17 ` KAMEZAWA Hiroyuki @ 2012-01-12 8:27 ` Michal Hocko 2012-01-12 8:35 ` KAMEZAWA Hiroyuki 0 siblings, 1 reply; 16+ messages in thread From: Michal Hocko @ 2012-01-12 8:27 UTC (permalink / raw) To: KAMEZAWA Hiroyuki Cc: David Rientjes, linux-mm, linux-kernel, Andrew Morton, Mel Gorman, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-12 8:27 ` Michal Hocko @ 2012-01-12 8:35 ` KAMEZAWA Hiroyuki 2012-01-12 9:23 ` Michal Hocko 0 siblings, 1 reply; 16+ messages in thread From: KAMEZAWA Hiroyuki @ 2012-01-12 8:35 UTC (permalink / raw) To: Michal Hocko Cc: David Rientjes, linux-mm, linux-kernel, Andrew Morton, Mel Gorman, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-12 8:35 ` KAMEZAWA Hiroyuki @ 2012-01-12 9:23 ` Michal Hocko 2012-01-12 9:33 ` KAMEZAWA Hiroyuki 0 siblings, 1 reply; 16+ messages in thread From: Michal Hocko @ 2012-01-12 9:23 UTC (permalink / raw) To: KAMEZAWA Hiroyuki Cc: David Rientjes, linux-mm, linux-kernel, Andrew Morton, Mel Gorman, Andrea Arcangeli 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-12 9:23 ` Michal Hocko @ 2012-01-12 9:33 ` KAMEZAWA Hiroyuki 2012-01-12 10:05 ` [PATCH] mm: __count_immobile_pages make sure the node is online Michal Hocko 0 siblings, 1 reply; 16+ messages in thread From: KAMEZAWA Hiroyuki @ 2012-01-12 9:33 UTC (permalink / raw) To: Michal Hocko Cc: David Rientjes, linux-mm, linux-kernel, Andrew Morton, Mel Gorman, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] mm: __count_immobile_pages make sure the node is online 2012-01-12 9:33 ` KAMEZAWA Hiroyuki @ 2012-01-12 10:05 ` Michal Hocko 2012-01-12 11:14 ` Mel Gorman 0 siblings, 1 reply; 16+ messages in thread From: Michal Hocko @ 2012-01-12 10:05 UTC (permalink / raw) To: KAMEZAWA Hiroyuki Cc: David Rientjes, linux-mm, linux-kernel, Andrew Morton, Mel Gorman, Andrea Arcangeli 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: __count_immobile_pages make sure the node is online 2012-01-12 10:05 ` [PATCH] mm: __count_immobile_pages make sure the node is online Michal Hocko @ 2012-01-12 11:14 ` Mel Gorman 2012-01-12 12:35 ` Michal Hocko 0 siblings, 1 reply; 16+ messages in thread From: Mel Gorman @ 2012-01-12 11:14 UTC (permalink / raw) To: Michal Hocko Cc: KAMEZAWA Hiroyuki, David Rientjes, linux-mm, linux-kernel, Andrew Morton, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: __count_immobile_pages make sure the node is online 2012-01-12 11:14 ` Mel Gorman @ 2012-01-12 12:35 ` Michal Hocko 2012-01-12 21:26 ` Andrew Morton 0 siblings, 1 reply; 16+ messages in thread From: Michal Hocko @ 2012-01-12 12:35 UTC (permalink / raw) To: Mel Gorman Cc: KAMEZAWA Hiroyuki, David Rientjes, linux-mm, linux-kernel, Andrew Morton, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: __count_immobile_pages make sure the node is online 2012-01-12 12:35 ` Michal Hocko @ 2012-01-12 21:26 ` Andrew Morton 2012-01-12 21:39 ` Michal Hocko 2012-01-13 10:04 ` Mel Gorman 0 siblings, 2 replies; 16+ messages in thread From: Andrew Morton @ 2012-01-12 21:26 UTC (permalink / raw) To: Michal Hocko Cc: Mel Gorman, KAMEZAWA Hiroyuki, David Rientjes, linux-mm, linux-kernel, Andrea Arcangeli 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: __count_immobile_pages make sure the node is online 2012-01-12 21:26 ` Andrew Morton @ 2012-01-12 21:39 ` Michal Hocko 2012-01-13 10:04 ` Mel Gorman 1 sibling, 0 replies; 16+ messages in thread From: Michal Hocko @ 2012-01-12 21:39 UTC (permalink / raw) To: Andrew Morton Cc: Mel Gorman, KAMEZAWA Hiroyuki, David Rientjes, linux-mm, linux-kernel, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: __count_immobile_pages make sure the node is online 2012-01-12 21:26 ` Andrew Morton 2012-01-12 21:39 ` Michal Hocko @ 2012-01-13 10:04 ` Mel Gorman 1 sibling, 0 replies; 16+ messages in thread From: Mel Gorman @ 2012-01-13 10:04 UTC (permalink / raw) To: Andrew Morton Cc: Michal Hocko, KAMEZAWA Hiroyuki, David Rientjes, linux-mm, linux-kernel, Andrea Arcangeli 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-10 16:30 [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages Michal Hocko 2012-01-10 21:31 ` David Rientjes @ 2012-01-11 22:34 ` Andrew Morton 2012-01-12 8:21 ` Michal Hocko 1 sibling, 1 reply; 16+ messages in thread From: Andrew Morton @ 2012-01-11 22:34 UTC (permalink / raw) To: Michal Hocko Cc: linux-mm, linux-kernel, Mel Gorman, KAMEZAWA Hiroyuki, Andrea Arcangeli, David Rientjes 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages 2012-01-11 22:34 ` [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages Andrew Morton @ 2012-01-12 8:21 ` Michal Hocko 0 siblings, 0 replies; 16+ messages in thread From: Michal Hocko @ 2012-01-12 8:21 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm, linux-kernel, Mel Gorman, KAMEZAWA Hiroyuki, Andrea Arcangeli, David Rientjes 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-01-13 10:04 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-01-10 16:30 [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages Michal Hocko 2012-01-10 21:31 ` David Rientjes 2012-01-11 8:48 ` Michal Hocko 2012-01-12 2:17 ` KAMEZAWA Hiroyuki 2012-01-12 8:27 ` Michal Hocko 2012-01-12 8:35 ` KAMEZAWA Hiroyuki 2012-01-12 9:23 ` Michal Hocko 2012-01-12 9:33 ` KAMEZAWA Hiroyuki 2012-01-12 10:05 ` [PATCH] mm: __count_immobile_pages make sure the node is online Michal Hocko 2012-01-12 11:14 ` Mel Gorman 2012-01-12 12:35 ` Michal Hocko 2012-01-12 21:26 ` Andrew Morton 2012-01-12 21:39 ` Michal Hocko 2012-01-13 10:04 ` Mel Gorman 2012-01-11 22:34 ` [PATCH] mm: Fix NULL ptr dereference in __count_immobile_pages Andrew Morton 2012-01-12 8:21 ` Michal Hocko
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).