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

[Matthew Wilcox]

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

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