mm/hotplug: invalid PFNs from pfn_to_online_page()
diff mbox series

Message ID 20190118021650.93222-1-cai@lca.pw
State New
Headers show
Series
  • mm/hotplug: invalid PFNs from pfn_to_online_page()
Related show

Commit Message

Qian Cai Jan. 18, 2019, 2:16 a.m. UTC
On an arm64 ThunderX2 server, the first kmemleak scan would crash [1]
with CONFIG_DEBUG_VM_PGFLAGS=y due to page_to_nid() found a pfn that is
not directly mapped (MEMBLOCK_NOMAP). Hence, the page->flags is
uninitialized.

This is due to the commit 9f1eb38e0e11 ("mm, kmemleak: little
optimization while scanning") starts to use pfn_to_online_page() instead
of pfn_valid(). However, in the CONFIG_MEMORY_HOTPLUG=y case,
pfn_to_online_page() does not call memblock_is_map_memory() while
pfn_valid() does.

[1]
[  102.195320] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000006
[  102.204113] Mem abort info:
[  102.206921]   ESR = 0x96000005
[  102.209997]   Exception class = DABT (current EL), IL = 32 bits
[  102.215926]   SET = 0, FnV = 0
[  102.218993]   EA = 0, S1PTW = 0
[  102.222150] Data abort info:
[  102.225047]   ISV = 0, ISS = 0x00000005
[  102.228887]   CM = 0, WnR = 0
[  102.231866] user pgtable: 64k pages, 48-bit VAs, pgdp = (____ptrval____)
[  102.238572] [0000000000000006] pgd=0000000000000000, pud=0000000000000000
[  102.245448] Internal error: Oops: 96000005 [#1] SMP
[  102.264062] CPU: 60 PID: 1408 Comm: kmemleak Not tainted 5.0.0-rc2+ #8
[  102.280403] pstate: 60400009 (nZCv daif +PAN -UAO)
[  102.280409] pc : page_mapping+0x24/0x144
[  102.280415] lr : __dump_page+0x34/0x3dc
[  102.292923] sp : ffff00003a5cfd10
[  102.296229] x29: ffff00003a5cfd10 x28: 000000000000802f
[  102.301533] x27: 0000000000000000 x26: 0000000000277d00
[  102.306835] x25: ffff000010791f56 x24: ffff7fe000000000
[  102.312138] x23: ffff000010772f8b x22: ffff00001125f670
[  102.317442] x21: ffff000011311000 x20: ffff000010772f8b
[  102.322747] x19: fffffffffffffffe x18: 0000000000000000
[  102.328049] x17: 0000000000000000 x16: 0000000000000000
[  102.333352] x15: 0000000000000000 x14: ffff802698b19600
[  102.338654] x13: ffff802698b1a200 x12: ffff802698b16f00
[  102.343956] x11: ffff802698b1a400 x10: 0000000000001400
[  102.349260] x9 : 0000000000000001 x8 : ffff00001121a000
[  102.354563] x7 : 0000000000000000 x6 : ffff0000102c53b8
[  102.359868] x5 : 0000000000000000 x4 : 0000000000000003
[  102.365173] x3 : 0000000000000100 x2 : 0000000000000000
[  102.370476] x1 : ffff000010772f8b x0 : ffffffffffffffff
[  102.375782] Process kmemleak (pid: 1408, stack limit = 0x(____ptrval____))
[  102.382648] Call trace:
[  102.385091]  page_mapping+0x24/0x144
[  102.388659]  __dump_page+0x34/0x3dc
[  102.392140]  dump_page+0x28/0x4c
[  102.395363]  kmemleak_scan+0x4ac/0x680
[  102.399106]  kmemleak_scan_thread+0xb4/0xdc
[  102.403285]  kthread+0x12c/0x13c
[  102.406509]  ret_from_fork+0x10/0x18
[  102.410080] Code: d503201f f9400660 36000040 d1000413 (f9400661)
[  102.416357] ---[ end trace 4d4bd7f573490c8e ]---
[  102.420966] Kernel panic - not syncing: Fatal exception
[  102.426293] SMP: stopping secondary CPUs
[  102.431830] Kernel Offset: disabled
[  102.435311] CPU features: 0x002,20000c38
[  102.439223] Memory Limit: none
[  102.442384] ---[ end Kernel panic - not syncing: Fatal exception ]---

Fixes: 2d070eab2e82 ("mm: consider zone which is not fully populated to
have holes")
Signed-off-by: Qian Cai <cai@lca.pw>
---
 include/linux/memory_hotplug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michal Hocko Jan. 21, 2019, 9:53 a.m. UTC | #1
On Thu 17-01-19 21:16:50, Qian Cai wrote:
> On an arm64 ThunderX2 server, the first kmemleak scan would crash [1]
> with CONFIG_DEBUG_VM_PGFLAGS=y due to page_to_nid() found a pfn that is
> not directly mapped (MEMBLOCK_NOMAP). Hence, the page->flags is
> uninitialized.
> 
> This is due to the commit 9f1eb38e0e11 ("mm, kmemleak: little
> optimization while scanning") starts to use pfn_to_online_page() instead
> of pfn_valid(). However, in the CONFIG_MEMORY_HOTPLUG=y case,
> pfn_to_online_page() does not call memblock_is_map_memory() while
> pfn_valid() does.

How come there is an online section which has an pfn_valid==F? We do
allocate the full section worth of struct pages so there is a valid
struct page. Is there any hole inside this section?

Isn't this a problem that the particular struct page hasn't been
intialized properly?

> [1]
> [  102.195320] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000006
> [  102.204113] Mem abort info:
> [  102.206921]   ESR = 0x96000005
> [  102.209997]   Exception class = DABT (current EL), IL = 32 bits
> [  102.215926]   SET = 0, FnV = 0
> [  102.218993]   EA = 0, S1PTW = 0
> [  102.222150] Data abort info:
> [  102.225047]   ISV = 0, ISS = 0x00000005
> [  102.228887]   CM = 0, WnR = 0
> [  102.231866] user pgtable: 64k pages, 48-bit VAs, pgdp = (____ptrval____)
> [  102.238572] [0000000000000006] pgd=0000000000000000, pud=0000000000000000
> [  102.245448] Internal error: Oops: 96000005 [#1] SMP
> [  102.264062] CPU: 60 PID: 1408 Comm: kmemleak Not tainted 5.0.0-rc2+ #8
> [  102.280403] pstate: 60400009 (nZCv daif +PAN -UAO)
> [  102.280409] pc : page_mapping+0x24/0x144
> [  102.280415] lr : __dump_page+0x34/0x3dc
> [  102.292923] sp : ffff00003a5cfd10
> [  102.296229] x29: ffff00003a5cfd10 x28: 000000000000802f
> [  102.301533] x27: 0000000000000000 x26: 0000000000277d00
> [  102.306835] x25: ffff000010791f56 x24: ffff7fe000000000
> [  102.312138] x23: ffff000010772f8b x22: ffff00001125f670
> [  102.317442] x21: ffff000011311000 x20: ffff000010772f8b
> [  102.322747] x19: fffffffffffffffe x18: 0000000000000000
> [  102.328049] x17: 0000000000000000 x16: 0000000000000000
> [  102.333352] x15: 0000000000000000 x14: ffff802698b19600
> [  102.338654] x13: ffff802698b1a200 x12: ffff802698b16f00
> [  102.343956] x11: ffff802698b1a400 x10: 0000000000001400
> [  102.349260] x9 : 0000000000000001 x8 : ffff00001121a000
> [  102.354563] x7 : 0000000000000000 x6 : ffff0000102c53b8
> [  102.359868] x5 : 0000000000000000 x4 : 0000000000000003
> [  102.365173] x3 : 0000000000000100 x2 : 0000000000000000
> [  102.370476] x1 : ffff000010772f8b x0 : ffffffffffffffff
> [  102.375782] Process kmemleak (pid: 1408, stack limit = 0x(____ptrval____))
> [  102.382648] Call trace:
> [  102.385091]  page_mapping+0x24/0x144
> [  102.388659]  __dump_page+0x34/0x3dc
> [  102.392140]  dump_page+0x28/0x4c
> [  102.395363]  kmemleak_scan+0x4ac/0x680
> [  102.399106]  kmemleak_scan_thread+0xb4/0xdc
> [  102.403285]  kthread+0x12c/0x13c
> [  102.406509]  ret_from_fork+0x10/0x18
> [  102.410080] Code: d503201f f9400660 36000040 d1000413 (f9400661)
> [  102.416357] ---[ end trace 4d4bd7f573490c8e ]---
> [  102.420966] Kernel panic - not syncing: Fatal exception
> [  102.426293] SMP: stopping secondary CPUs
> [  102.431830] Kernel Offset: disabled
> [  102.435311] CPU features: 0x002,20000c38
> [  102.439223] Memory Limit: none
> [  102.442384] ---[ end Kernel panic - not syncing: Fatal exception ]---
> 
> Fixes: 2d070eab2e82 ("mm: consider zone which is not fully populated to
> have holes")

Did you mean 
Fixes: 9f1eb38e0e11 ("mm, kmemleak: little  optimization while scanning")

> Signed-off-by: Qian Cai <cai@lca.pw>
> ---
>  include/linux/memory_hotplug.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 07da5c6c5ba0..b8b36e6ac43b 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -26,7 +26,7 @@ struct vmem_altmap;
>  	struct page *___page = NULL;			\
>  	unsigned long ___nr = pfn_to_section_nr(pfn);	\
>  							\
> -	if (___nr < NR_MEM_SECTIONS && online_section_nr(___nr))\
> +	if (online_section_nr(___nr) && pfn_valid(pfn))	\
>  		___page = pfn_to_page(pfn);		\

Why have you removed the bound check? Is this safe?
Regarding the fix, I am not really sure TBH. If the secion is online
then we assume all struct pages to be initialized. If anything this
should be limited to werid arches which might have holes so
pfn_valid_within().

>  	___page;					\
>  })
> -- 
> 2.17.2 (Apple Git-113)
Qian Cai Jan. 21, 2019, 4:38 p.m. UTC | #2
On 1/21/19 4:53 AM, Michal Hocko wrote:
> On Thu 17-01-19 21:16:50, Qian Cai wrote:
>> On an arm64 ThunderX2 server, the first kmemleak scan would crash [1]
>> with CONFIG_DEBUG_VM_PGFLAGS=y due to page_to_nid() found a pfn that is
>> not directly mapped (MEMBLOCK_NOMAP). Hence, the page->flags is
>> uninitialized.
>>
>> This is due to the commit 9f1eb38e0e11 ("mm, kmemleak: little
>> optimization while scanning") starts to use pfn_to_online_page() instead
>> of pfn_valid(). However, in the CONFIG_MEMORY_HOTPLUG=y case,
>> pfn_to_online_page() does not call memblock_is_map_memory() while
>> pfn_valid() does.
> 
> How come there is an online section which has an pfn_valid==F? We do
> allocate the full section worth of struct pages so there is a valid
> struct page. Is there any hole inside this section?

It has CONFIG_HOLES_IN_ZONE=y.

> 
> Isn't this a problem that the particular struct page hasn't been
> intialized properly?
> 
>> [1]
>> [  102.195320] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000006
>> [  102.204113] Mem abort info:
>> [  102.206921]   ESR = 0x96000005
>> [  102.209997]   Exception class = DABT (current EL), IL = 32 bits
>> [  102.215926]   SET = 0, FnV = 0
>> [  102.218993]   EA = 0, S1PTW = 0
>> [  102.222150] Data abort info:
>> [  102.225047]   ISV = 0, ISS = 0x00000005
>> [  102.228887]   CM = 0, WnR = 0
>> [  102.231866] user pgtable: 64k pages, 48-bit VAs, pgdp = (____ptrval____)
>> [  102.238572] [0000000000000006] pgd=0000000000000000, pud=0000000000000000
>> [  102.245448] Internal error: Oops: 96000005 [#1] SMP
>> [  102.264062] CPU: 60 PID: 1408 Comm: kmemleak Not tainted 5.0.0-rc2+ #8
>> [  102.280403] pstate: 60400009 (nZCv daif +PAN -UAO)
>> [  102.280409] pc : page_mapping+0x24/0x144
>> [  102.280415] lr : __dump_page+0x34/0x3dc
>> [  102.292923] sp : ffff00003a5cfd10
>> [  102.296229] x29: ffff00003a5cfd10 x28: 000000000000802f
>> [  102.301533] x27: 0000000000000000 x26: 0000000000277d00
>> [  102.306835] x25: ffff000010791f56 x24: ffff7fe000000000
>> [  102.312138] x23: ffff000010772f8b x22: ffff00001125f670
>> [  102.317442] x21: ffff000011311000 x20: ffff000010772f8b
>> [  102.322747] x19: fffffffffffffffe x18: 0000000000000000
>> [  102.328049] x17: 0000000000000000 x16: 0000000000000000
>> [  102.333352] x15: 0000000000000000 x14: ffff802698b19600
>> [  102.338654] x13: ffff802698b1a200 x12: ffff802698b16f00
>> [  102.343956] x11: ffff802698b1a400 x10: 0000000000001400
>> [  102.349260] x9 : 0000000000000001 x8 : ffff00001121a000
>> [  102.354563] x7 : 0000000000000000 x6 : ffff0000102c53b8
>> [  102.359868] x5 : 0000000000000000 x4 : 0000000000000003
>> [  102.365173] x3 : 0000000000000100 x2 : 0000000000000000
>> [  102.370476] x1 : ffff000010772f8b x0 : ffffffffffffffff
>> [  102.375782] Process kmemleak (pid: 1408, stack limit = 0x(____ptrval____))
>> [  102.382648] Call trace:
>> [  102.385091]  page_mapping+0x24/0x144
>> [  102.388659]  __dump_page+0x34/0x3dc
>> [  102.392140]  dump_page+0x28/0x4c
>> [  102.395363]  kmemleak_scan+0x4ac/0x680
>> [  102.399106]  kmemleak_scan_thread+0xb4/0xdc
>> [  102.403285]  kthread+0x12c/0x13c
>> [  102.406509]  ret_from_fork+0x10/0x18
>> [  102.410080] Code: d503201f f9400660 36000040 d1000413 (f9400661)
>> [  102.416357] ---[ end trace 4d4bd7f573490c8e ]---
>> [  102.420966] Kernel panic - not syncing: Fatal exception
>> [  102.426293] SMP: stopping secondary CPUs
>> [  102.431830] Kernel Offset: disabled
>> [  102.435311] CPU features: 0x002,20000c38
>> [  102.439223] Memory Limit: none
>> [  102.442384] ---[ end Kernel panic - not syncing: Fatal exception ]---
>>
>> Fixes: 2d070eab2e82 ("mm: consider zone which is not fully populated to
>> have holes")
> 
> Did you mean 
> Fixes: 9f1eb38e0e11 ("mm, kmemleak: little  optimization while scanning")

No, pfn_to_online_page() missed a few checks compared to pfn_valid() at least on
arm64 where the returned pfn is no longer valid (where pfn_valid() will skip those).

2d070eab2e82 introduced pfn_to_online_page(), so it was targeted to fix it.

> 
>> Signed-off-by: Qian Cai <cai@lca.pw>
>> ---
>>  include/linux/memory_hotplug.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>> index 07da5c6c5ba0..b8b36e6ac43b 100644
>> --- a/include/linux/memory_hotplug.h
>> +++ b/include/linux/memory_hotplug.h
>> @@ -26,7 +26,7 @@ struct vmem_altmap;
>>  	struct page *___page = NULL;			\
>>  	unsigned long ___nr = pfn_to_section_nr(pfn);	\
>>  							\
>> -	if (___nr < NR_MEM_SECTIONS && online_section_nr(___nr))\
>> +	if (online_section_nr(___nr) && pfn_valid(pfn))	\
>>  		___page = pfn_to_page(pfn);		\
> 
> Why have you removed the bound check? Is this safe?
> Regarding the fix, I am not really sure TBH. If the secion is online
> then we assume all struct pages to be initialized. If anything this
> should be limited to werid arches which might have holes so
> pfn_valid_within().

It looks to me at least on arm64 and x86_64, it has done this check in
pfn_valid() already.

if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
		return 0
Qian Cai Jan. 21, 2019, 5:58 p.m. UTC | #3
On 1/21/19 11:38 AM, Qian Cai wrote:
> 
> 
> On 1/21/19 4:53 AM, Michal Hocko wrote:
>> On Thu 17-01-19 21:16:50, Qian Cai wrote:
>>> On an arm64 ThunderX2 server, the first kmemleak scan would crash [1]
>>> with CONFIG_DEBUG_VM_PGFLAGS=y due to page_to_nid() found a pfn that is
>>> not directly mapped (MEMBLOCK_NOMAP). Hence, the page->flags is
>>> uninitialized.
>>>
>>> This is due to the commit 9f1eb38e0e11 ("mm, kmemleak: little
>>> optimization while scanning") starts to use pfn_to_online_page() instead
>>> of pfn_valid(). However, in the CONFIG_MEMORY_HOTPLUG=y case,
>>> pfn_to_online_page() does not call memblock_is_map_memory() while
>>> pfn_valid() does.
>>
>> How come there is an online section which has an pfn_valid==F? We do
>> allocate the full section worth of struct pages so there is a valid
>> struct page. Is there any hole inside this section?
> 
> It has CONFIG_HOLES_IN_ZONE=y.

Actually, this does not seem have anything to do with holes.

68709f45385a arm64: only consider memblocks with NOMAP cleared for linear mapping

This causes pages marked as nomap being no long reassigned to the new zone in
memmap_init_zone() by calling __init_single_page().

There is an old discussion for this topic.
https://lkml.org/lkml/2016/11/30/566
Michal Hocko Jan. 21, 2019, 6:19 p.m. UTC | #4
On Mon 21-01-19 11:38:49, Qian Cai wrote:
> 
> 
> On 1/21/19 4:53 AM, Michal Hocko wrote:
> > On Thu 17-01-19 21:16:50, Qian Cai wrote:
[...]
> >> Fixes: 2d070eab2e82 ("mm: consider zone which is not fully populated to
> >> have holes")
> > 
> > Did you mean 
> > Fixes: 9f1eb38e0e11 ("mm, kmemleak: little  optimization while scanning")
> 
> No, pfn_to_online_page() missed a few checks compared to pfn_valid() at least on
> arm64 where the returned pfn is no longer valid (where pfn_valid() will skip those).
> 
> 2d070eab2e82 introduced pfn_to_online_page(), so it was targeted to fix it.

But it is 9f1eb38e0e11 which has replaced pfn_valid by
pfn_to_online_page.

> 
> > 
> >> Signed-off-by: Qian Cai <cai@lca.pw>
> >> ---
> >>  include/linux/memory_hotplug.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> >> index 07da5c6c5ba0..b8b36e6ac43b 100644
> >> --- a/include/linux/memory_hotplug.h
> >> +++ b/include/linux/memory_hotplug.h
> >> @@ -26,7 +26,7 @@ struct vmem_altmap;
> >>  	struct page *___page = NULL;			\
> >>  	unsigned long ___nr = pfn_to_section_nr(pfn);	\
> >>  							\
> >> -	if (___nr < NR_MEM_SECTIONS && online_section_nr(___nr))\
> >> +	if (online_section_nr(___nr) && pfn_valid(pfn))	\
> >>  		___page = pfn_to_page(pfn);		\
> > 
> > Why have you removed the bound check? Is this safe?
> > Regarding the fix, I am not really sure TBH. If the secion is online
> > then we assume all struct pages to be initialized. If anything this
> > should be limited to werid arches which might have holes so
> > pfn_valid_within().
> 
> It looks to me at least on arm64 and x86_64, it has done this check in
> pfn_valid() already.
> 
> if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
> 		return 0

But an everflow could happen before pfn_valid is evaluated, no?
Michal Hocko Jan. 21, 2019, 6:36 p.m. UTC | #5
On Mon 21-01-19 12:58:46, Qian Cai wrote:
> 
> 
> On 1/21/19 11:38 AM, Qian Cai wrote:
> > 
> > 
> > On 1/21/19 4:53 AM, Michal Hocko wrote:
> >> On Thu 17-01-19 21:16:50, Qian Cai wrote:
> >>> On an arm64 ThunderX2 server, the first kmemleak scan would crash [1]
> >>> with CONFIG_DEBUG_VM_PGFLAGS=y due to page_to_nid() found a pfn that is
> >>> not directly mapped (MEMBLOCK_NOMAP). Hence, the page->flags is
> >>> uninitialized.
> >>>
> >>> This is due to the commit 9f1eb38e0e11 ("mm, kmemleak: little
> >>> optimization while scanning") starts to use pfn_to_online_page() instead
> >>> of pfn_valid(). However, in the CONFIG_MEMORY_HOTPLUG=y case,
> >>> pfn_to_online_page() does not call memblock_is_map_memory() while
> >>> pfn_valid() does.
> >>
> >> How come there is an online section which has an pfn_valid==F? We do
> >> allocate the full section worth of struct pages so there is a valid
> >> struct page. Is there any hole inside this section?
> > 
> > It has CONFIG_HOLES_IN_ZONE=y.
> 
> Actually, this does not seem have anything to do with holes.
> 
> 68709f45385a arm64: only consider memblocks with NOMAP cleared for linear mapping
> 
> This causes pages marked as nomap being no long reassigned to the new zone in
> memmap_init_zone() by calling __init_single_page().

Thanks for the pointer. This sched some light but I cannot say I would
understand all the details.

> There is an old discussion for this topic.
> https://lkml.org/lkml/2016/11/30/566

Hmm, I see. The documentation is not the best (mea culpa)
 * Return page for the valid pfn only if the page is online. All pfn
 * walkers which rely on the fully initialized page->flags and others
 * should use this rather than pfn_valid && pfn_to_page

This suggests that the pfn is _valid_ when using pfn_to_online_page and
some callers indeed do so. Some of them don't though which is probably
because the later part of the documentation suggests that it should
replace pfn_valid & pfn_to_page. Thinking about this more, I guess we do
not want to put an additional burden on callers and require pfn_valid to
be called as well. This is just error prone and can lead to problems
like this one.

So I agree with your change (modulo the range check) but please make
sure to make all this information to the changelog.

Thanks!
Qian Cai Jan. 21, 2019, 6:37 p.m. UTC | #6
On 1/21/19 1:19 PM, Michal Hocko wrote:
> On Mon 21-01-19 11:38:49, Qian Cai wrote:
>>
>>
>> On 1/21/19 4:53 AM, Michal Hocko wrote:
>>> On Thu 17-01-19 21:16:50, Qian Cai wrote:
> [...]
>>>> Fixes: 2d070eab2e82 ("mm: consider zone which is not fully populated to
>>>> have holes")
>>>
>>> Did you mean 
>>> Fixes: 9f1eb38e0e11 ("mm, kmemleak: little  optimization while scanning")
>>
>> No, pfn_to_online_page() missed a few checks compared to pfn_valid() at least on
>> arm64 where the returned pfn is no longer valid (where pfn_valid() will skip those).
>>
>> 2d070eab2e82 introduced pfn_to_online_page(), so it was targeted to fix it.
> 
> But it is 9f1eb38e0e11 which has replaced pfn_valid by
> pfn_to_online_page.

Well, the comment of pfn_to_online_page() said,

/*
 * Return page for the valid pfn only if the page is online.
 * All pfn walkers which rely on the fully initialized
 * page->flags and others should use this rather than
 * pfn_valid && pfn_to_page
 */

That seems incorrect to me in the first place, as it currently not return "fully
initialized page->flags" pages in arm64.

Once this fixed, there is no problem with 9f1eb38e0e11. It seems to me
9f1eb38e0e11 just depends on a broken interface, so it is better to fix the
broken interface.

> 
>>
>>>
>>>> Signed-off-by: Qian Cai <cai@lca.pw>
>>>> ---
>>>>  include/linux/memory_hotplug.h | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>>>> index 07da5c6c5ba0..b8b36e6ac43b 100644
>>>> --- a/include/linux/memory_hotplug.h
>>>> +++ b/include/linux/memory_hotplug.h
>>>> @@ -26,7 +26,7 @@ struct vmem_altmap;
>>>>  	struct page *___page = NULL;			\
>>>>  	unsigned long ___nr = pfn_to_section_nr(pfn);	\
>>>>  							\
>>>> -	if (___nr < NR_MEM_SECTIONS && online_section_nr(___nr))\
>>>> +	if (online_section_nr(___nr) && pfn_valid(pfn))	\
>>>>  		___page = pfn_to_page(pfn);		\
>>>
>>> Why have you removed the bound check? Is this safe?
>>> Regarding the fix, I am not really sure TBH. If the secion is online
>>> then we assume all struct pages to be initialized. If anything this
>>> should be limited to werid arches which might have holes so
>>> pfn_valid_within().
>>
>> It looks to me at least on arm64 and x86_64, it has done this check in
>> pfn_valid() already.
>>
>> if (pfn_to_section_nr(pfn) >= NR_MEM_SECTIONS)
>> 		return 0
> 
> But an everflow could happen before pfn_valid is evaluated, no?
> 

I guess you mean "overflow". I'll probably keep that check and use
pfn_valid_within() anyway, so I could optimize the checking if
CONFIG_HOLES_IN_ZONE=n.

Patch
diff mbox series

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 07da5c6c5ba0..b8b36e6ac43b 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -26,7 +26,7 @@  struct vmem_altmap;
 	struct page *___page = NULL;			\
 	unsigned long ___nr = pfn_to_section_nr(pfn);	\
 							\
-	if (___nr < NR_MEM_SECTIONS && online_section_nr(___nr))\
+	if (online_section_nr(___nr) && pfn_valid(pfn))	\
 		___page = pfn_to_page(pfn);		\
 	___page;					\
 })