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

[David Hildenbrand (Arm)]

>> Reported-by: Kyle Zeng <kylebot@xxxxxxxxxx>
>> Fixes: d2136d749d76 ("mm: support multi-size THP numa balancing")
>> Fixes: 508758960b8d ("mm: swap: entirely map large folios found in swapcache")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> ---
>>  mm/memory.c | 43 +++++++++++++++++++++++++++++--------------
>>  1 file changed, 29 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 1f5d5f7d39cd2..a3823010aa981 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4933,6 +4933,27 @@ static void check_swap_exclusive(struct folio *folio, swp_entry_t entry,
>>  	} while (--nr_pages);
>>  }
>>
>> +/*
>> + * 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.

> 
> Also then can specify exclusive end, etc.
> 
>> +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.

> 
> Maybe get_folio_pte_range()?

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

> 
> Also I think it might be useful to have it return the number of pages contained
> within the range?
> 
> Then this below:
> 
> +		folio_pte_scan_range(folio, vma, address, idx, &folio_start,
> +				     &folio_end);
> +		if (unlikely((folio_end - folio_start) >> PAGE_SHIFT != nr))
> 
> Could be:
> 
> +		if (unlikely(get_folio_pte_range(folio, vma, address, idx,
> +						 &folio_start, &folio_end)) != nr)
> 
> OTOH, that's pretty ugly... :))


Not a fan of these semantics.

> 
>> +{
>> +	/* 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;
> 
> OK so get the range of the PTE page table.
> 
>> +	const unsigned long lower = max(pmd_start, vma->vm_start);
> 
> Then clamp to VMA 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.

> Yeah I kind of hate that, that's a lot of 'just have to know' right there.
> 
> Actually we have pmd_addr_end() already, could we use that?
> 
> #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.

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

> 
> That embeds the trick in the helper and it expresses what you're getting.
> 
> Then it could be:
> 
> 	const unsigned long pmd_start = addr & PMD_MASK;
> 	const unsigned long lower = max(pmd_start, vma->vm_start);
> 	const unsigned long upper = pmd_addr_end(addr, vma->vm_end);
> 
>> +	const unsigned long offset = idx << PAGE_SHIFT;
>> +
>> +	/* Avoid any address wraparounds. */
>> +	*start = addr - min(addr - lower, offset);
> 
> This is also tricky. This is clamping to max(lower, addr - offset) right,
> but being very careful about underflow?
> 
> I don't love the addr - (addr - lower) = lower here, and it feels like it
> embeds a comparison there too. It's all very implicit.
> 
> Actually maybe just naming addr - lower gets us there:
> 
> 	const unsigned long max_offset = addr - lower;
> 	...
> 	*start = addr - min(max_offset, offset);
> 
> There's check_sub_overflow() or you could use a ternary also but both feel
> like they add more complexity than is useful.
> 
>> +	*end = addr + min(upper - addr, folio_size(folio) - offset);
> 
> Same deal here.
> 
> Maybe something like:
> 
> 	const unsigned long max_remaining_bytes = upper - addr;
> 	...
> 	*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?


-- 
Cheers,

David