Re: [PATCH] mm: page_ext: validate section in for_each_page_ext iterator

["David Hildenbrand (Arm)" <david@kernel.org>]

On 6/17/26 17:09, Ketan wrote:
> The page_ext iteration API do not validate if the PFN still
> belongs to a valid section while advancing the iterator.
> 
> When dynamically adding memory in hotplug path, it can lead
> to a NULL pointer dereference during page_ext_lookup at the
> boundary of the last valid section.
> 
> [   14.555124][  T846] Call trace:
> [   14.555125][  T846]  lookup_page_ext+0x6c/0x108 (P)
> [   14.555127][  T846]  page_ext_lookup+0x30/0x3c
> [   14.555129][  T846]   +0x11c/0x260
> [   14.571201][  T846]  __free_pages_ok+0x5e8/0x8e0
> [   14.571204][  T846]  __free_pages_core+0x78/0xf0
> [   14.571206][  T846]  generic_online_page+0x14/0x24
> [   14.597782][  T846]  online_pages+0x178/0x30c
> [   14.597784][  T846]  memory_block_change_state+0x284/0x32c
> [   14.597787][  T846]  memory_subsys_online+0x4c/0x64
> [   14.597789][  T846]  device_online+0x88/0xb0
> [   14.597791][  T846]  online_memory_block+0x30/0x40
> [   14.597793][  T846]  walk_memory_blocks+0xac/0xe8
> [   14.597794][  T846]  add_memory_resource+0x280/0x298
> [   14.656161][  T846]  add_memory+0x60/0x98

So, we do allocate the page_ext in memory_notify(MEM_GOING_ONLINE) -> ...
page_ext_callback() -> online_page_ext().

Which makes sure that all page_ext is actually allocated (for the full section).

So when we later end up in generic_online_page(), the page_ext should be there
and the memory section should be valid.


How can online_pages(), which onlines memory part of present memory sections,
online (free) memory that suddenly does not belong to a present memory section?

Something doesn't make sense here.

> 
> Add a valid-section check before looking up the next page_ext
> so the iterator stops cleanly at section boundaries.
> 
> Fixes: 9039b9096ea2 ("mm: page_owner: use new iteration API")

If the problem is real, we'd want to CC stable.

> Signed-off-by: Ketan Kishore <ketan.kishore@oss.qualcomm.com>
> ---
>  mm/page_ext.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/mm/page_ext.c b/mm/page_ext.c
> index e2e92bd27ebd..74067ea740fe 100644
> --- a/mm/page_ext.c
> +++ b/mm/page_ext.c
> @@ -253,6 +253,18 @@ static struct page_ext *lookup_page_ext(const struct page *page)
>  {
>  	unsigned long pfn = page_to_pfn(page);
>  	struct mem_section *section = __pfn_to_section(pfn);
> +
> +	/*
> +	 * section can be NULL when the page_ext iterator's for-loop increment
> +	 * computes a PFN one step beyond the last registered section. This
> +	 * occurs because pfn_to_page() uses __nr_to_section() which succeeds
> +	 * for unregistered sections that share a root array with registered
> +	 * sections,while __pfn_to_section() returns NULL for them.
> +	 *
> +	 */
> +	if (!section)
> +		return NULL;
> +
>  	struct page_ext *page_ext = READ_ONCE(section->page_ext);

In addition to what Zi says, which will end up as:

	if (!section)
		return NULL;
	page_ext = READ_ONCE(section->page_ext);


drop that comment, not required. And likely best to just add a unlikely(),
because this doesn't usually happen.

-- 
Cheers,

David