[PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[Pingfan Liu]

After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"),
when a mem section is fully deactivated, section_mem_map still records the
section's start pfn, which is not used any more and will be reassigned
during re-added.

In analogy with alloc/free pattern, it is better to clear all fields of
section_mem_map.

Beside this, it breaks the user space tool "makedumpfile" [1], which makes
assumption that a hot-removed section has mem_map as NULL, instead of
checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be
better to change the assumption, and need a patch)

The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" ,
trigger a crash, and save vmcore by makedumpfile

[1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version")

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
To: linux-mm@kvack.org
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Qian Cai <cai@lca.pw>
Cc: kexec@lists.infradead.org
Cc: Kazuhito Hagio <k-hagio@ab.jp.nec.com>
---
v1 -> v2:
 make an explicit convertion from NULL to ulong
 improve commit log
 
 mm/sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 3822ecb..3918fc3 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -789,7 +789,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
 			ms->usage = NULL;
 		}
 		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
-		ms->section_mem_map = sparse_encode_mem_map(NULL, section_nr);
+		ms->section_mem_map = (unsigned long)NULL;
 	}
 
 	if (section_is_early && memmap)
-- 
2.7.5

Re: [PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[Michal Hocko]

On Mon 20-01-20 10:33:14, Pingfan Liu wrote:
> After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"),
> when a mem section is fully deactivated, section_mem_map still records the
> section's start pfn, which is not used any more and will be reassigned
> during re-added.
> 
> In analogy with alloc/free pattern, it is better to clear all fields of
> section_mem_map.
> 
> Beside this, it breaks the user space tool "makedumpfile" [1], which makes
> assumption that a hot-removed section has mem_map as NULL, instead of
> checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be
> better to change the assumption, and need a patch)
> 
> The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" ,
> trigger a crash, and save vmcore by makedumpfile

While makedumpfile lives very closely to the kernel and occasional
breakage is to be expected I still believe that Fixes: ba72b4c8cf60
is due.

> [1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version")
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> To: linux-mm@kvack.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Qian Cai <cai@lca.pw>
> Cc: kexec@lists.infradead.org
> Cc: Kazuhito Hagio <k-hagio@ab.jp.nec.com>

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

> ---
> v1 -> v2:
>  make an explicit convertion from NULL to ulong
>  improve commit log
>  
>  mm/sparse.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 3822ecb..3918fc3 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -789,7 +789,7 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages,
>  			ms->usage = NULL;
>  		}
>  		memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
> -		ms->section_mem_map = sparse_encode_mem_map(NULL, section_nr);
> +		ms->section_mem_map = (unsigned long)NULL;
>  	}
>  
>  	if (section_is_early && memmap)
> -- 
> 2.7.5

-- 
Michal Hocko
SUSE Labs

Re: [PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[David Hildenbrand]

On 20.01.20 08:29, Michal Hocko wrote:
> On Mon 20-01-20 10:33:14, Pingfan Liu wrote:
>> After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"),
>> when a mem section is fully deactivated, section_mem_map still records the
>> section's start pfn, which is not used any more and will be reassigned
>> during re-added.
>>
>> In analogy with alloc/free pattern, it is better to clear all fields of
>> section_mem_map.
>>
>> Beside this, it breaks the user space tool "makedumpfile" [1], which makes
>> assumption that a hot-removed section has mem_map as NULL, instead of
>> checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be
>> better to change the assumption, and need a patch)
>>
>> The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" ,
>> trigger a crash, and save vmcore by makedumpfile
> 
> While makedumpfile lives very closely to the kernel and occasional
> breakage is to be expected I still believe that Fixes: ba72b4c8cf60
> is due.

+1 as I expressed earlier.

> 
>> [1]: makedumpfile, commit e73016540293 ("[v1.6.7] Update version")
>>
>> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
>> To: linux-mm@kvack.org
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Baoquan He <bhe@redhat.com>
>> Cc: Qian Cai <cai@lca.pw>
>> Cc: kexec@lists.infradead.org
>> Cc: Kazuhito Hagio <k-hagio@ab.jp.nec.com>
> 
I think you dropped my

Acked-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[Pingfan Liu]

On Mon, Jan 20, 2020 at 5:03 PM David Hildenbrand <david@redhat.com> wrote:
>
[...]
> I think you dropped my
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
Sorry to forget it.
Thanks for your kind review.

Regards,
Pingfan

Re: [PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[Andrew Morton]

On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <mhocko@suse.com> wrote:

> On Mon 20-01-20 10:33:14, Pingfan Liu wrote:
> > After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"),
> > when a mem section is fully deactivated, section_mem_map still records the
> > section's start pfn, which is not used any more and will be reassigned
> > during re-added.
> > 
> > In analogy with alloc/free pattern, it is better to clear all fields of
> > section_mem_map.
> > 
> > Beside this, it breaks the user space tool "makedumpfile" [1], which makes
> > assumption that a hot-removed section has mem_map as NULL, instead of
> > checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be
> > better to change the assumption, and need a patch)
> > 
> > The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" ,
> > trigger a crash, and save vmcore by makedumpfile
> 
> While makedumpfile lives very closely to the kernel and occasional
> breakage is to be expected I still believe that Fixes: ba72b4c8cf60
> is due.

But not a cc:stable?

Re: [PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[Michal Hocko]

On Thu 23-01-20 19:10:47, Andrew Morton wrote:
> On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <mhocko@suse.com> wrote:
> 
> > On Mon 20-01-20 10:33:14, Pingfan Liu wrote:
> > > After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"),
> > > when a mem section is fully deactivated, section_mem_map still records the
> > > section's start pfn, which is not used any more and will be reassigned
> > > during re-added.
> > > 
> > > In analogy with alloc/free pattern, it is better to clear all fields of
> > > section_mem_map.
> > > 
> > > Beside this, it breaks the user space tool "makedumpfile" [1], which makes
> > > assumption that a hot-removed section has mem_map as NULL, instead of
> > > checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be
> > > better to change the assumption, and need a patch)
> > > 
> > > The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" ,
> > > trigger a crash, and save vmcore by makedumpfile
> > 
> > While makedumpfile lives very closely to the kernel and occasional
> > breakage is to be expected I still believe that Fixes: ba72b4c8cf60
> > is due.
> 
> But not a cc:stable?

Well, I wouldn't say this is really critical. makedumpfile will get its
fix... But if people think it would be useful in stable I won't oppose. 
-- 
Michal Hocko
SUSE Labs

Re: [PATCHv2] mm/sparse: reset section's mem_map when fully deactivated

[Pingfan Liu]

On Fri, Jan 24, 2020 at 2:49 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 23-01-20 19:10:47, Andrew Morton wrote:
> > On Mon, 20 Jan 2020 08:29:39 +0100 Michal Hocko <mhocko@suse.com> wrote:
> >
> > > On Mon 20-01-20 10:33:14, Pingfan Liu wrote:
> > > > After commit ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug"),
> > > > when a mem section is fully deactivated, section_mem_map still records the
> > > > section's start pfn, which is not used any more and will be reassigned
> > > > during re-added.
> > > >
> > > > In analogy with alloc/free pattern, it is better to clear all fields of
> > > > section_mem_map.
> > > >
> > > > Beside this, it breaks the user space tool "makedumpfile" [1], which makes
> > > > assumption that a hot-removed section has mem_map as NULL, instead of
> > > > checking directly against SECTION_MARKED_PRESENT bit. (makedumpfile will be
> > > > better to change the assumption, and need a patch)
> > > >
> > > > The bug can be reproduced on IBM POWERVM by "drmgr -c mem -r -q 5" ,
> > > > trigger a crash, and save vmcore by makedumpfile
> > >
> > > While makedumpfile lives very closely to the kernel and occasional
> > > breakage is to be expected I still believe that Fixes: ba72b4c8cf60
> > > is due.
> >
> > But not a cc:stable?
>
> Well, I wouldn't say this is really critical. makedumpfile will get its
> fix... But if people think it would be useful in stable I won't oppose.
Yes, I think this patch is no more than a prototype improvement, and
makedumpfile has better to get its fix.
And I have sent a patch to kexec-list for it.
(http://lists.infradead.org/pipermail/kexec/2020-January/024406.html)

Thanks,
    Pingfan