Re: [PATCH] mm/util: don't read __page_2 for order-1 folios in snapshot_page()

[David Hildenbrand (Arm)]

On 7/8/26 03:52, Aboorva Devarajan wrote:
> snapshot_page() reconstructs a folio from a struct page.  After copying
> the head and __page_1 it reads __page_2 whenever the folio has more than
> one page:
> 
> 	if (nr_pages > 1)
> 		memcpy(&ps->folio_snapshot.__page_2, &foliop->__page_2,
> 		       sizeof(struct page));
> 
> __page_2 is the folio's third struct page, so it is part of the folio
> only for order >= 2 (nr_pages > 2).  For an order-1 folio (exactly two
> pages) __page_2 is not part of the folio at all, it is the struct page
> of the following pfn.
> 
> When such an order-1 head sits in the last struct page slots of a
> populated section whose neighbouring section is absent (a memory hole),
> __page_2 falls into the next section's unpopulated vmemmap and the
> read oopses.
> 
> Observed on a 22 TB ppc64le LPAR during DLPAR memory remove, on the page
> isolation dump path:
> 
> 	offline_pages -> start_isolate_page_range -> isolate_single_pageblock
> 	  -> set_migratetype_isolate -> dump_page -> __dump_page -> snapshot_page
> 
> 	NIP   = snapshot_page+264  (ld of __page_2)
> 	r4    = foliop = head = 0xc00c0005a03fff80
> 	DAR   = r4 + 0x88     = 0xc00c0005a0400008   (unmapped)
> 	DSISR = 0x40000000                           (no translation)
> 
> The faulting head was a free page that still carried PG_head with
> _nr_pages == 2; its __page_2 is the first entry of the absent section.
> 
> It is also reproducible deterministically in a VM by placing an order-1
> folio in the last slots of a populated section adjacent to a hole
> (memmap=nnM$ssM) and calling dump_page() on it.
> 
> Only read __page_2 for order >= 2 folios (nr_pages > 2).

Hi!

Can you shorten that a bit? It's rather trivial, really.

"snapshot_page() currently reads __page_2 after checking nr_pages > 1, whereby
we really should only do so for nr_pages > 2. Let's fix that to avoid reading
memmap that doesn't exist (e.g., vmemmap hole)


Observed on a 22 TB ppc64le LPAR during DLPAR memory remove ...
"

> 
> Fixes: 31a31da8a618 ("mm: move _pincount in folio to page[2] on 32bit")
> Cc: stable@xxxxxxxxxxxxxxx # v6.15+
> Reported-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
> Signed-off-by: Aboorva Devarajan <aboorvad@xxxxxxxxxxxxx>
> ---
>  mm/util.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index af2c2103f0d95..b3d48a05e6d82 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -1353,7 +1353,13 @@ void snapshot_page(struct page_snapshot *ps, const struct page *page)
>  	if (ps->idx < MAX_FOLIO_NR_PAGES) {
>  		memcpy(&ps->folio_snapshot, foliop, 2 * sizeof(struct page));
>  		nr_pages = folio_nr_pages(&ps->folio_snapshot);
> -		if (nr_pages > 1)
> +		/*
> +		 * __page_2 is the folio's third struct page and is part of the
> +		 * folio only for order >= 2 (nr_pages > 2).  For an order-1
> +		 * folio it is not part of the folio and may fall into an
> +		 * adjacent, possibly absent, section.
> +		 */

No need for the comment, really, this is rather trivial.

> +		if (nr_pages > 2)
>  			memcpy(&ps->folio_snapshot.__page_2, &foliop->__page_2,
>  			       sizeof(struct page));
>  		set_ps_flags(ps, foliop, page);


With a condensed patch description and the comment dropped

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

Thanks!

-- 
Cheers,

David