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

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

On 10/14/26 15:27, Lorenzo Stoakes (ARM) wrote:
> On Wed, Oct 14, 2026 at 10:12:06AM +0200, David Hildenbrand (Arm) wrote:
> 
> 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 :>)
> 
> 
>>>
>>> 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.
> 
>>>
>>> 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.

Ok, let's do that.

>>>
>>>
>>> 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?

It's the following thing that adds confusion:

include/asm-generic/pgtable-nopmd.h:#define pmd_addr_end(addr, end)
       (end)

I think we have to assume here that with NOPMD we will never end up in this code
because large folios will not apply (each PMD table has exactly one entry as of
today, so we cannot map large folios .... I think).

I can try condensing that in a comment and cross my fingers that that is the
case. I tried that now. and codex review already tells me:

  • [P1] Folded PMDs can reach this helper — memory.c:4948 (mm/memory.c:4948).
The comment’s assumption is false: arm64 with 64 KiB pages and 42-bit VA uses
folded PMDs and supports THP and
    NUMA balancing. Here, pmd_addr_end() returns vma->vm_end without clamping. A
large folio remapped across a PTE-table boundary can therefore make
numa_rebuild_large_mapping() walk beyond
    the mapped PTE table. The previous code explicitly limited this to pt_start
+ PMD_SIZE. Cap the forward distance at PMD_SIZE - (addr & ~PMD_MASK) as well as
the VMA and folio limits.

Let me see if there is any truth to that ...


[...]

> 
> 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.

What I am saying is that I don't agree with adding another two const variables
with names that are less intuitive than just saying the computation. Let me try
to improve the comment:

/* Avoid address wraparounds by not adding/subtracting beyond limits. */

-- 
Cheers,

David