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

[Lorenzo Stoakes]

On Wed, Jul 08, 2026 at 10:10:33AM +0200, David Hildenbrand (Arm) wrote:
> 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

Agree with everything David said :)

Patch looks good with changes David suggested applied, so feel free to add my
tag to v2 alongside David's:

Reviewed-by: Lorenzo Stoakes <ljs@xxxxxxxxxx>

Cheers, Lorenzo