Re: [PATCH] mm: Avoid calling folio_page() with an out-of-bounds index

[Dev Jain <dev.jain@arm.com>]

On 25/02/26 7:01 pm, Matthew Wilcox wrote:
> On Wed, Feb 25, 2026 at 03:35:32PM +0530, Dev Jain wrote:
>> On 25/02/26 2:56 pm, Li Zhe wrote:
>>> In folio_zero_user(), the page pointer is calculated via folio_page()
>>> before checking if the number of pages to be cleared is greater than zero.
>>> Furthermore, folio_page() does not verify that the page number lies
>>> within folio.
>>>
>>> When 'addr_hint' is near the end of a large folio, the range 'r[0]'
>>> represents an empty interval. In this scenario, 'nr_pages' will be
>>> calculated as 0 and 'r[0].start' can be an index that is out-of-bounds
>>> for folio_page(). The code unconditionally calls folio_page() on a wrong
>>> index, even though the subsequent clearing logic is correctly skipped.
>>>
>>> While this does not cause a functional bug today, calculating a page
>>> pointer for an out-of-bounds index is logically unsound and fragile. It
>>> could pose a risk for future refactoring or trigger warnings from static
>>> analysis tools.
>>>
>>> To fix this, move the call to folio_page() inside the 'if (nr_pages > 0)'
>>> block. This ensures that the page pointer is only calculated when it is
>>> actually needed for a valid, non-empty range of pages, thus making the code
>>> more robust and logically correct.
>>>
>>> Signed-off-by: Li Zhe <lizhe.67@bytedance.com>
>>> ---
>>
>> Not only the correctness, but even from a perf PoV (folio_zero_user is a
>> hot path) it may make sense to initialize the variable only when required.
> 
> But now calculating 'addr' and 'page' is dependent on calculating
> nr_pages instead of being an independent calculation.  I'd be *VERY*
> wary of saying this is a performance win without actually measuring it.
> CPUs are far more complex than you seem to realise (which is ironic,
> given your employer).
> 
> Now, maybe the compiler is smart enough to realise there isn't a real
> dependency and it can hoist the calculation out of the 'if'.  But then
> what have we achieved with this patch?

I did do a compiler analysis in my head, I should have spelled it out
before R'bing in haste :) My guess was that since the array size is small
(i.e 3) the compiler should unroll the for loop - that should perhaps
generate better code with the patch (the two initializations will be done
inside the branch as opposed to outside).

But your point is that the current code may be CPU friendly (better
instruction parallelism, etc), so we shouldn't speculate without testing
(even my compiler logic may be wrong in reality). Indeed. Thank you for
setting me straight.

> 
> Honestly, I think this patch is worthless and would not include it.
> 
>>
>>
>>>  mm/memory.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 07778814b4a8..6f8c55d604b5 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -7343,12 +7343,14 @@ void folio_zero_user(struct folio *folio, unsigned long addr_hint)
>>>  	r[0] = DEFINE_RANGE(r[2].end + 1, pg.end);
>>>  
>>>  	for (i = 0; i < ARRAY_SIZE(r); i++) {
>>> -		const unsigned long addr = base_addr + r[i].start * PAGE_SIZE;
>>>  		const long nr_pages = (long)range_len(&r[i]);
>>> -		struct page *page = folio_page(folio, r[i].start);
>>>  
>>> -		if (nr_pages > 0)
>>> +		if (nr_pages > 0) {
>>> +			const unsigned long addr = base_addr + r[i].start * PAGE_SIZE;
>>> +			struct page *page = folio_page(folio, r[i].start);
>>> +
>>>  			clear_contig_highpages(page, addr, nr_pages);
>>> +		}
>>>  	}
>>>  }
>>>  
>>
>>