mm: Avoid calling folio_page() with an out-of-bounds index

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

Posted by Li Zhe 1 month, 2 weeks ago

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

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

Posted by David Hildenbrand (Arm) 1 month, 2 weeks ago

On 2/25/26 10:26, 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>
> ---
>  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);
> +		}
>  	}
>  }
>  

It's all just arithmetic operations. Just like Willy says, I'd assume
that the compiler can just move them around as it pleases. No dependencies.

So I don't see any real benefit with this patch.

-- 
Cheers,

David

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

Posted by Li Zhe 1 month, 2 weeks ago

On Wed, 25 Feb 2026 14:40:41 +0100 david@kernel.org wrote:

> On 2/25/26 10:26, 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>
> > ---
> >  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);
> > +		}
> >  	}
> >  }
> >  
> 
> It's all just arithmetic operations. Just like Willy says, I'd assume
> that the compiler can just move them around as it pleases. No dependencies.
> 
> So I don't see any real benefit with this patch.

Yes, this change does not offer any performance benefits under the
current conditions. I proposed it primarily as a code cleanup effort.

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

This is what I see as the primary benefit of this patch. Should an
out-of-bounds issue surface later, it would still need to be addressed.

Thanks,
Zhe

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

Posted by Dev Jain 1 month, 2 weeks ago


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.

Reviewed-by: Dev Jain <dev.jain@arm.com>


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

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

Posted by Matthew Wilcox 1 month, 2 weeks ago

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?

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

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

Posted by Ankur Arora 1 month, 2 weeks ago

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

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

Posted by Dev Jain 1 month, 2 weeks ago


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