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

[Ankur Arora <ankur.a.arora@oracle.com>]

Matthew Wilcox <willy@infradead.org> writes:

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

And the "nr_pages > 0" branch is determined by user space (in the
page fault case) so we likely will get a branch-miss there.

> 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?
>
> 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);
>> > +		}
>> >  	}
>> >  }
>> >
>>
>>


--
ankur