Re: [PATCH] mm: fix address wraparound when scanning large folio PTEs

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Wed, Oct 14, 2026 at 10:12:06AM +0200, David Hildenbrand (Arm) wrote:
> >> Reported-by: Kyle Zeng <kylebot@openai.com>
> >> Fixes: d2136d749d76 ("mm: support multi-size THP numa balancing")
> >> Fixes: 508758960b8d ("mm: swap: entirely map large folios found in swapcache")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>

Since I'm convinced the logic itself is correct:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

This cycle is brutal enough not to hold up fixes :>)

> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index 1f5d5f7d39cd2..a3823010aa981 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c

> >> +/*
> >> + * Get the address range in which a folio might be mapped, assuming its pages
> >> + * are mapped consecutively and the folio page at the given idx is mapped at
> >> + * the given address.
> >> + */
> >
> > I know it's a static function but kdoc here might be nice given the number of
> > params, including 2 output parameters.
>
> I had that, but decided "not worth it for an internal helper".
>
> Is anything here really that confusing to understand in mm/memory.c context? I
> highly doubt it.

I mean I found it pretty confusing :)

But this is a nice to have it's not a big deal.

> >> +static void folio_pte_scan_range(const struct folio *folio,
> >> +		const struct vm_area_struct *vma, unsigned long addr,
> >> +		unsigned long idx, unsigned long *start, unsigned long *end)
> >
> > I don't love the name - to me it implies to me that you're scanning the range.
> >
> > Maybe clamp_folio_pte_range()? I guess though clamp implies it limits an input
> > range.
>
> Ugly.

Yup, fair enough.

>
> >
> > Maybe get_folio_pte_range()?
>
> I can just call it "folio_pte_range()". Slightly similar (but different) to
> "folio_pte_batch()"

OK that works.

> > OTOH, that's pretty ugly... :))
>
>
> Not a fan of these semantics.

Yeah on reflection not so great, I sort of talked myself out of it a bit as I
typed...

>
> >
> >> +{
> >> +	/* Stay within the page table and VMA boundaries. */
> >> +	const unsigned long pmd_start = addr & PMD_MASK;
> >> +	const unsigned long pmd_end = pmd_start + PMD_SIZE;
> >> +	const unsigned long lower = max(pmd_start, vma->vm_start);
> >> +	const unsigned long upper = min(pmd_end - 1, vma->vm_end - 1) + 1;
> >
> > This is trickier :)
> >
> > I guess you're doing this in case either bound is right at wraparound
> > somehow?
> >
> > But then couldn't the +1 push you over the edge?
> >
> > Oh wait no, because then vma->vm_end would have to equal 0, which would
> > fail the other-place enforced invariant that vma->vm_start < vma->vm_end.
> >
> > So it's pmd_end that could overflow, hence the -1, +1 dance.
> >
>
> Yes. It's fairly common, though, to handle clamping of exclusive ends that way.

Right, it took me a while to figure this out though.

It involves an implicit assumption in there too, that _one_ of these values is
known not to overflow.

> > #define pmd_addr_end(addr, end)                                         \
> > ({      unsigned long __boundary = ((addr) + PMD_SIZE) & PMD_MASK;      \
> >         (__boundary - 1 < (end) - 1)? __boundary: (end);                \
> > })
>
> As replied elsewhere, I had that, but decided to open-code as pmd_addr_end() has
> very weird semantics with folded page tables, where we just don't clamp at all.

Hmm how so?

The logic seems, at a glance, equivalent?

>
> I'll sleep over it, but to me using pmd_addr_end() is not particularly better.

I'm not going to block things on that basis as we have a bug to fix here, but I
really had to sit down and take time to understand any of this.

> > 	*end = addr + min(max_remaining_bytes, folio_size(folio) - offset);
> >
>
> Sorry, to push back, but are we overthinking this helper here?
>
> I much rather get this fixed quickly then fine tuning some details in a helper
> with clear semantics that anybody can understand when looking at it for 1 minute?

As above, I tagged it, for the purposes of a fix let's just get this in.

But no, there's not overthinking IMO, it took me a long time to understand what
you were doing here, and there's a lot of implicit understanding in there.

And while I don't claim to be a great genius, I am fairly familiar with mm code,
so that's always to me a gauge on complexity.

Anyway, let's get this in, and we can debate the details upstream at our leisure
:)

--
Cheers, Lorenzo