[PATCH] memory_hotplug: fix kernel_panic on offline page processing

[PATCH] memory_hotplug: fix kernel_panic on offline page processing

[Mikhail Zaslonko]

Within show_valid_zones() the function test_pages_in_a_zone() should be
called for online memory blocks only. Otherwise it might lead to the
VM_BUG_ON due to uninitialized struct pages (when CONFIG_DEBUG_VM_PGFLAGS
kernel option is set):

 page dumped because: VM_BUG_ON_PAGE(PagePoisoned(p))
 ------------[ cut here ]------------
 Call Trace:
 ([<000000000038f91e>] test_pages_in_a_zone+0xe6/0x168)
  [<0000000000923472>] show_valid_zones+0x5a/0x1a8
  [<0000000000900284>] dev_attr_show+0x3c/0x78
  [<000000000046f6f0>] sysfs_kf_seq_show+0xd0/0x150
  [<00000000003ef662>] seq_read+0x212/0x4b8
  [<00000000003bf202>] __vfs_read+0x3a/0x178
  [<00000000003bf3ca>] vfs_read+0x8a/0x148
  [<00000000003bfa3a>] ksys_read+0x62/0xb8
  [<0000000000bc2220>] system_call+0xdc/0x2d8

That VM_BUG_ON was triggered by the page poisoning introduced in
mm/sparse.c with the git commit d0dc12e86b31 ("mm/memory_hotplug: optimize
memory hotplug")
With the same commit the new 'nid' field has been added to the struct
memory_block in order to store and later on derive the node id for offline
pages (instead of accessing struct page which might be uninitialized). But
one reference to nid in show_valid_zones() function has been overlooked.
Fixed with current commit.
Also, nr_pages will not be used any more after test_pages_in_a_zone() call,
do not update it.

Fixes: d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
Cc: <stable@vger.kernel.org> # v4.17+
Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
---
 drivers/base/memory.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index f5e560188a18..622ab8edc035 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -416,26 +416,24 @@ static ssize_t show_valid_zones(struct device *dev,
 	struct zone *default_zone;
 	int nid;
 
-	/*
-	 * The block contains more than one zone can not be offlined.
-	 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
-	 */
-	if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages, &valid_start_pfn, &valid_end_pfn))
-		return sprintf(buf, "none\n");
-
-	start_pfn = valid_start_pfn;
-	nr_pages = valid_end_pfn - start_pfn;
-
 	/*
 	 * Check the existing zone. Make sure that we do that only on the
 	 * online nodes otherwise the page_zone is not reliable
 	 */
 	if (mem->state == MEM_ONLINE) {
+		/*
+		 * The block contains more than one zone can not be offlined.
+		 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
+		 */
+		if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages,
+					  &valid_start_pfn, &valid_end_pfn))
+			return sprintf(buf, "none\n");
+		start_pfn = valid_start_pfn;
 		strcat(buf, page_zone(pfn_to_page(start_pfn))->name);
 		goto out;
 	}
 
-	nid = pfn_to_nid(start_pfn);
+	nid = mem->nid;
 	default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
 	strcat(buf, default_zone->name);
 
-- 
2.16.4

Re: [PATCH] memory_hotplug: fix kernel_panic on offline page processing

[Michal Hocko]

On Tue 28-08-18 11:05:39, Mikhail Zaslonko wrote:
> Within show_valid_zones() the function test_pages_in_a_zone() should be
> called for online memory blocks only. Otherwise it might lead to the
> VM_BUG_ON due to uninitialized struct pages (when CONFIG_DEBUG_VM_PGFLAGS
> kernel option is set):
> 
>  page dumped because: VM_BUG_ON_PAGE(PagePoisoned(p))
>  ------------[ cut here ]------------
>  Call Trace:
>  ([<000000000038f91e>] test_pages_in_a_zone+0xe6/0x168)
>   [<0000000000923472>] show_valid_zones+0x5a/0x1a8
>   [<0000000000900284>] dev_attr_show+0x3c/0x78
>   [<000000000046f6f0>] sysfs_kf_seq_show+0xd0/0x150
>   [<00000000003ef662>] seq_read+0x212/0x4b8
>   [<00000000003bf202>] __vfs_read+0x3a/0x178
>   [<00000000003bf3ca>] vfs_read+0x8a/0x148
>   [<00000000003bfa3a>] ksys_read+0x62/0xb8
>   [<0000000000bc2220>] system_call+0xdc/0x2d8
> 
> That VM_BUG_ON was triggered by the page poisoning introduced in
> mm/sparse.c with the git commit d0dc12e86b31 ("mm/memory_hotplug: optimize
> memory hotplug")
> With the same commit the new 'nid' field has been added to the struct
> memory_block in order to store and later on derive the node id for offline
> pages (instead of accessing struct page which might be uninitialized). But
> one reference to nid in show_valid_zones() function has been overlooked.
> Fixed with current commit.
> Also, nr_pages will not be used any more after test_pages_in_a_zone() call,
> do not update it.
> 
> Fixes: d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
> Cc: <stable@vger.kernel.org> # v4.17+
> Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>

Btw. this land mines which are basically impossible to find during the
review are the reason why I was not all that happy about d0dc12e86b31.
It added a margninal improvement but opened a can of warms. On the other
hand maybe we just had to open that can one day...

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  drivers/base/memory.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index f5e560188a18..622ab8edc035 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -416,26 +416,24 @@ static ssize_t show_valid_zones(struct device *dev,
>  	struct zone *default_zone;
>  	int nid;
>  
> -	/*
> -	 * The block contains more than one zone can not be offlined.
> -	 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
> -	 */
> -	if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages, &valid_start_pfn, &valid_end_pfn))
> -		return sprintf(buf, "none\n");
> -
> -	start_pfn = valid_start_pfn;
> -	nr_pages = valid_end_pfn - start_pfn;
> -
>  	/*
>  	 * Check the existing zone. Make sure that we do that only on the
>  	 * online nodes otherwise the page_zone is not reliable
>  	 */
>  	if (mem->state == MEM_ONLINE) {
> +		/*
> +		 * The block contains more than one zone can not be offlined.
> +		 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
> +		 */
> +		if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages,
> +					  &valid_start_pfn, &valid_end_pfn))
> +			return sprintf(buf, "none\n");
> +		start_pfn = valid_start_pfn;
>  		strcat(buf, page_zone(pfn_to_page(start_pfn))->name);
>  		goto out;
>  	}
>  
> -	nid = pfn_to_nid(start_pfn);
> +	nid = mem->nid;
>  	default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
>  	strcat(buf, default_zone->name);
>  
> -- 
> 2.16.4

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memory_hotplug: fix kernel_panic on offline page processing

[Michal Hocko]

[Fixup Pavel's email - the patch is here for your reference
http://lkml.kernel.org/r/20180828090539.41491-1-zaslonko@linux.ibm.com]

On Tue 28-08-18 13:25:43, Michal Hocko wrote:
> On Tue 28-08-18 11:05:39, Mikhail Zaslonko wrote:
> > Within show_valid_zones() the function test_pages_in_a_zone() should be
> > called for online memory blocks only. Otherwise it might lead to the
> > VM_BUG_ON due to uninitialized struct pages (when CONFIG_DEBUG_VM_PGFLAGS
> > kernel option is set):
> > 
> >  page dumped because: VM_BUG_ON_PAGE(PagePoisoned(p))
> >  ------------[ cut here ]------------
> >  Call Trace:
> >  ([<000000000038f91e>] test_pages_in_a_zone+0xe6/0x168)
> >   [<0000000000923472>] show_valid_zones+0x5a/0x1a8
> >   [<0000000000900284>] dev_attr_show+0x3c/0x78
> >   [<000000000046f6f0>] sysfs_kf_seq_show+0xd0/0x150
> >   [<00000000003ef662>] seq_read+0x212/0x4b8
> >   [<00000000003bf202>] __vfs_read+0x3a/0x178
> >   [<00000000003bf3ca>] vfs_read+0x8a/0x148
> >   [<00000000003bfa3a>] ksys_read+0x62/0xb8
> >   [<0000000000bc2220>] system_call+0xdc/0x2d8
> > 
> > That VM_BUG_ON was triggered by the page poisoning introduced in
> > mm/sparse.c with the git commit d0dc12e86b31 ("mm/memory_hotplug: optimize
> > memory hotplug")
> > With the same commit the new 'nid' field has been added to the struct
> > memory_block in order to store and later on derive the node id for offline
> > pages (instead of accessing struct page which might be uninitialized). But
> > one reference to nid in show_valid_zones() function has been overlooked.
> > Fixed with current commit.
> > Also, nr_pages will not be used any more after test_pages_in_a_zone() call,
> > do not update it.
> > 
> > Fixes: d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
> > Cc: <stable@vger.kernel.org> # v4.17+
> > Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
> > Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
> 
> Btw. this land mines which are basically impossible to find during the
> review are the reason why I was not all that happy about d0dc12e86b31.
> It added a margninal improvement but opened a can of warms. On the other
> hand maybe we just had to open that can one day...
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> 
> > ---
> >  drivers/base/memory.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> > index f5e560188a18..622ab8edc035 100644
> > --- a/drivers/base/memory.c
> > +++ b/drivers/base/memory.c
> > @@ -416,26 +416,24 @@ static ssize_t show_valid_zones(struct device *dev,
> >  	struct zone *default_zone;
> >  	int nid;
> >  
> > -	/*
> > -	 * The block contains more than one zone can not be offlined.
> > -	 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
> > -	 */
> > -	if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages, &valid_start_pfn, &valid_end_pfn))
> > -		return sprintf(buf, "none\n");
> > -
> > -	start_pfn = valid_start_pfn;
> > -	nr_pages = valid_end_pfn - start_pfn;
> > -
> >  	/*
> >  	 * Check the existing zone. Make sure that we do that only on the
> >  	 * online nodes otherwise the page_zone is not reliable
> >  	 */
> >  	if (mem->state == MEM_ONLINE) {
> > +		/*
> > +		 * The block contains more than one zone can not be offlined.
> > +		 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
> > +		 */
> > +		if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages,
> > +					  &valid_start_pfn, &valid_end_pfn))
> > +			return sprintf(buf, "none\n");
> > +		start_pfn = valid_start_pfn;
> >  		strcat(buf, page_zone(pfn_to_page(start_pfn))->name);
> >  		goto out;
> >  	}
> >  
> > -	nid = pfn_to_nid(start_pfn);
> > +	nid = mem->nid;
> >  	default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
> >  	strcat(buf, default_zone->name);
> >  
> > -- 
> > 2.16.4
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] memory_hotplug: fix kernel_panic on offline page processing

[Pasha Tatashin]

On 8/28/18 7:25 AM, Michal Hocko wrote:
> On Tue 28-08-18 11:05:39, Mikhail Zaslonko wrote:
>> Within show_valid_zones() the function test_pages_in_a_zone() should be
>> called for online memory blocks only. Otherwise it might lead to the
>> VM_BUG_ON due to uninitialized struct pages (when CONFIG_DEBUG_VM_PGFLAGS
>> kernel option is set):
>>
>>  page dumped because: VM_BUG_ON_PAGE(PagePoisoned(p))
>>  ------------[ cut here ]------------
>>  Call Trace:
>>  ([<000000000038f91e>] test_pages_in_a_zone+0xe6/0x168)
>>   [<0000000000923472>] show_valid_zones+0x5a/0x1a8
>>   [<0000000000900284>] dev_attr_show+0x3c/0x78
>>   [<000000000046f6f0>] sysfs_kf_seq_show+0xd0/0x150
>>   [<00000000003ef662>] seq_read+0x212/0x4b8
>>   [<00000000003bf202>] __vfs_read+0x3a/0x178
>>   [<00000000003bf3ca>] vfs_read+0x8a/0x148
>>   [<00000000003bfa3a>] ksys_read+0x62/0xb8
>>   [<0000000000bc2220>] system_call+0xdc/0x2d8
>>
>> That VM_BUG_ON was triggered by the page poisoning introduced in
>> mm/sparse.c with the git commit d0dc12e86b31 ("mm/memory_hotplug: optimize
>> memory hotplug")
>> With the same commit the new 'nid' field has been added to the struct
>> memory_block in order to store and later on derive the node id for offline
>> pages (instead of accessing struct page which might be uninitialized). But
>> one reference to nid in show_valid_zones() function has been overlooked.
>> Fixed with current commit.
>> Also, nr_pages will not be used any more after test_pages_in_a_zone() call,
>> do not update it.
>>
>> Fixes: d0dc12e86b31 ("mm/memory_hotplug: optimize memory hotplug")
>> Cc: <stable@vger.kernel.org> # v4.17+
>> Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
>> Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
> 
> Btw. this land mines which are basically impossible to find during the
> review are the reason why I was not all that happy about d0dc12e86b31.
> It added a margninal improvement but opened a can of warms. On the other

Hi Michal,

I agree, the hotplug code is very fragile. But, it only means that it
requires improvements.

I specifically added PagePoisoned() check with hotplug optimizations
changes in order to catch these kind of bugs, and I am glad we are
catching them.

Reviewed-by: Pavel Tatashin <pavel.tatashin@microsoft.com>

Thank you,
Pavel

> hand maybe we just had to open that can one day...
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> 
>> ---
>>  drivers/base/memory.c | 20 +++++++++-----------
>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index f5e560188a18..622ab8edc035 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -416,26 +416,24 @@ static ssize_t show_valid_zones(struct device *dev,
>>  	struct zone *default_zone;
>>  	int nid;
>>  
>> -	/*
>> -	 * The block contains more than one zone can not be offlined.
>> -	 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
>> -	 */
>> -	if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages, &valid_start_pfn, &valid_end_pfn))
>> -		return sprintf(buf, "none\n");
>> -
>> -	start_pfn = valid_start_pfn;
>> -	nr_pages = valid_end_pfn - start_pfn;
>> -
>>  	/*
>>  	 * Check the existing zone. Make sure that we do that only on the
>>  	 * online nodes otherwise the page_zone is not reliable
>>  	 */
>>  	if (mem->state == MEM_ONLINE) {
>> +		/*
>> +		 * The block contains more than one zone can not be offlined.
>> +		 * This can happen e.g. for ZONE_DMA and ZONE_DMA32
>> +		 */
>> +		if (!test_pages_in_a_zone(start_pfn, start_pfn + nr_pages,
>> +					  &valid_start_pfn, &valid_end_pfn))
>> +			return sprintf(buf, "none\n");
>> +		start_pfn = valid_start_pfn;
>>  		strcat(buf, page_zone(pfn_to_page(start_pfn))->name);
>>  		goto out;
>>  	}
>>  
>> -	nid = pfn_to_nid(start_pfn);
>> +	nid = mem->nid;
>>  	default_zone = zone_for_pfn_range(MMOP_ONLINE_KEEP, nid, start_pfn, nr_pages);
>>  	strcat(buf, default_zone->name);
>>  
>> -- 
>> 2.16.4
>