[PATCH] mm/util: fix off-by-one when checking folio index in snapshot_page()

[PATCH] mm/util: fix off-by-one when checking folio index in snapshot_page()

[shao.mingyin]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

mm/util: fix off-by-one when checking folio index in snapshot_page()

snapshot_page() computes the page index before copying the folio state.
If the folio changes between those reads, the index can equal the copied
folio's page count. Valid indices are strictly less than that count,
but the current check accepts equality and marks the snapshot faithful.

Use >= so that this inconsistent snapshot is retried, or falls back to
the unfaithful single-page representation. This also avoids reporting an
out-of-range folio index without the corresponding dump_page() warning.

Fixes: d863a12108f2 ("mm/util: introduce snapshot_page()")
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
Build-tested mm/util.o with W=1 on x86_64. The boundary check was
reviewed statically; the folio-split race has not been reproduced.


 mm/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/util.c b/mm/util.c
index a5140b3b70f3305cd78144a54d3b15a794da9609..6e6a8f8640a75ebb0f81f54c9a5085f698320714 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -1462,7 +1462,7 @@ void snapshot_page(struct page_snapshot *ps, const struct page *page)
 		set_ps_flags(ps, foliop, page);
 	}
 
-	if (ps->idx > nr_pages) {
+	if (ps->idx >= nr_pages) {
 		if (loops-- > 0)
 			goto again;
 		clear_compound_head(&ps->page_snapshot);

base-commit: 84abb955b1015477aa11a96e2f3ac1b938b77f48
-- 
2.43.0

Re: [PATCH] mm/util: fix off-by-one when checking folio index in snapshot_page()

[Lorenzo Stoakes (ARM)]

On Sun, Nov 15, 2026 at 12:46:57AM +0800, shao.mingyin@zte.com.cn wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
>
> mm/util: fix off-by-one when checking folio index in snapshot_page()
>
> snapshot_page() computes the page index before copying the folio state.
> If the folio changes between those reads, the index can equal the copied
> folio's page count. Valid indices are strictly less than that count,
> but the current check accepts equality and marks the snapshot faithful.
>
> Use >= so that this inconsistent snapshot is retried, or falls back to
> the unfaithful single-page representation. This also avoids reporting an
> out-of-range folio index without the corresponding dump_page() warning.
>
> Fixes: d863a12108f2 ("mm/util: introduce snapshot_page()")
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>

NAK.

this is generated by some AI slopper's script, spamming the kernel. The
patches I looked at were wrong/broken, so no to anything from this person.

They've sent 300 mails in under four weeks spanniing: ata, scsi, target,
ufs, nvme/nvmet, ubifs, ublk, zloop, dm, md, mmc, virtio-blk, mfd, gpio,
pwm, spi, PCI, ASoC, ACPI, net, vfs mount, block partitions, and now mm.

and 17 series across 7 subsystems over the past 23 hours.

We need a way to ban emails from people like this...

--
Cheers, Lorenzo

Re: [PATCH] mm/util: fix off-by-one when checking folio index in snapshot_page()

[David Hildenbrand (Arm)]

On 11/14/26 18:33, Lorenzo Stoakes (ARM) wrote:
> On Sun, Nov 15, 2026 at 12:46:57AM +0800, shao.mingyin@zte.com.cn wrote:
>> From: Shao Mingyin <shao.mingyin@zte.com.cn>
>>
>> mm/util: fix off-by-one when checking folio index in snapshot_page()
>>
>> snapshot_page() computes the page index before copying the folio state.
>> If the folio changes between those reads, the index can equal the copied
>> folio's page count. Valid indices are strictly less than that count,
>> but the current check accepts equality and marks the snapshot faithful.
>>
>> Use >= so that this inconsistent snapshot is retried, or falls back to
>> the unfaithful single-page representation. This also avoids reporting an
>> out-of-range folio index without the corresponding dump_page() warning.
>>
>> Fixes: d863a12108f2 ("mm/util: introduce snapshot_page()")
>> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> NAK.

Let's do the following:. Thoughts?

From c7d1a79926e3b3448bb5e84f901c185234f43b52 Mon Sep 17 00:00:00 2001
From: "David Hildenbrand (Arm)" <david@kernel.org>
Date: Sun, 15 Nov 2026 18:11:08 +0100
Subject: [PATCH] mm/util: fix retry detection and handling in snapshot_page()

snapshot_page() detects races with concurrent folio split+construction by
checking whether the given page still falls into the folio. Unfortunately,
we have an off-by-one error in there, and we should be checking for
"ps->idx >= nr_pages", because idx 0 is valid.

For example, we might currently report a tail page at idx 1 as faithful,
although the snapshotted folio is now a small folio.

However, there is one complication: As we don't hold a folio reference,
folio_nr_pages() can return 0 on races (or when called from page freeing
code). Handle that by just setting it to 1 in that case, so we don't
wrongly report a head page as "page does not match folio".

In addition, we should reset ps->flags when retrying, by just resetting
it to PAGE_SNAPSHOT_FAITHFUL. Otherwise, PAGE_SNAPSHOT_PG_BUDDY and
PAGE_SNAPSHOT_PG_IDLE from a failed attempt could stick around.

These races are fairly rare, so this is not really critical, we simply
report something slightly wrong. No need to CC stable.

Fixes: d863a12108f2 ("mm/util: introduce snapshot_page()")
Reported-by: Shao Mingyin <shao.mingyin@zte.com.cn>
Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
---
 mm/util.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/util.c b/mm/util.c
index 80f6c714844f2..3b10d4c2a4fd9 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -1406,9 +1406,9 @@ void snapshot_page(struct page_snapshot *ps, const struct page *page)
 	int loops = 5;
 
 	ps->pfn = page_to_pfn(page);
-	ps->flags = PAGE_SNAPSHOT_FAITHFUL;
 
 again:
+	ps->flags = PAGE_SNAPSHOT_FAITHFUL;
 	memset(&ps->folio_snapshot, 0, sizeof(struct folio));
 	memcpy(&ps->page_snapshot, page, sizeof(*page));
 	info = ps->page_snapshot.compound_info;
@@ -1438,13 +1438,16 @@ void snapshot_page(struct page_snapshot *ps, const struct page *page)
 	if (ps->idx < MAX_FOLIO_NR_PAGES) {
 		memcpy(&ps->folio_snapshot, foliop, 2 * sizeof(struct page));
 		nr_pages = folio_nr_pages(&ps->folio_snapshot);
+		/* On races, or when freeing pages, we can read 0. */
+		if (nr_pages == 0)
+			nr_pages = 1;
 		if (nr_pages > 2)
 			memcpy(&ps->folio_snapshot.__page_2, &foliop->__page_2,
 			       sizeof(struct page));
 		set_ps_flags(ps, foliop, page);
 	}
 
-	if (ps->idx > nr_pages) {
+	if (ps->idx >= nr_pages) {
 		if (loops-- > 0)
 			goto again;
 		clear_compound_head(&ps->page_snapshot);
-- 
2.56.1


-- 
Cheers,

David

Re: [PATCH] mm/util: fix off-by-one when checking folio index in snapshot_page()

[Lorenzo Stoakes (ARM)]

On Sun, Nov 15, 2026 at 06:32:12PM +0100, David Hildenbrand (Arm) wrote:
> On 11/14/26 18:33, Lorenzo Stoakes (ARM) wrote:
> > On Sun, Nov 15, 2026 at 12:46:57AM +0800, shao.mingyin@zte.com.cn wrote:
> >> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> >>
> >> mm/util: fix off-by-one when checking folio index in snapshot_page()
> >>
> >> snapshot_page() computes the page index before copying the folio state.
> >> If the folio changes between those reads, the index can equal the copied
> >> folio's page count. Valid indices are strictly less than that count,
> >> but the current check accepts equality and marks the snapshot faithful.
> >>
> >> Use >= so that this inconsistent snapshot is retried, or falls back to
> >> the unfaithful single-page representation. This also avoids reporting an
> >> out-of-range folio index without the corresponding dump_page() warning.
> >>
> >> Fixes: d863a12108f2 ("mm/util: introduce snapshot_page()")
> >> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> >
> > NAK.
>
> Let's do the following:. Thoughts?
>
> From c7d1a79926e3b3448bb5e84f901c185234f43b52 Mon Sep 17 00:00:00 2001
> From: "David Hildenbrand (Arm)" <david@kernel.org>
> Date: Sun, 15 Nov 2026 18:11:08 +0100
> Subject: [PATCH] mm/util: fix retry detection and handling in snapshot_page()
>
> snapshot_page() detects races with concurrent folio split+construction by
> checking whether the given page still falls into the folio. Unfortunately,
> we have an off-by-one error in there, and we should be checking for
> "ps->idx >= nr_pages", because idx 0 is valid.
>
> For example, we might currently report a tail page at idx 1 as faithful,
> although the snapshotted folio is now a small folio.
>
> However, there is one complication: As we don't hold a folio reference,
> folio_nr_pages() can return 0 on races (or when called from page freeing
> code). Handle that by just setting it to 1 in that case, so we don't
> wrongly report a head page as "page does not match folio".
>
> In addition, we should reset ps->flags when retrying, by just resetting
> it to PAGE_SNAPSHOT_FAITHFUL. Otherwise, PAGE_SNAPSHOT_PG_BUDDY and
> PAGE_SNAPSHOT_PG_IDLE from a failed attempt could stick around.
>
> These races are fairly rare, so this is not really critical, we simply
> report something slightly wrong. No need to CC stable.
>
> Fixes: d863a12108f2 ("mm/util: introduce snapshot_page()")
> Reported-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>

Yeah this is reminisecent of other patches from the same batch I saw in
that the original didn't quite fix the problem correctly :)

Thanks for this! One small nit below but otherwise:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> ---
>  mm/util.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/util.c b/mm/util.c
> index 80f6c714844f2..3b10d4c2a4fd9 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -1406,9 +1406,9 @@ void snapshot_page(struct page_snapshot *ps, const struct page *page)
>  	int loops = 5;
>
>  	ps->pfn = page_to_pfn(page);
> -	ps->flags = PAGE_SNAPSHOT_FAITHFUL;
>
>  again:
> +	ps->flags = PAGE_SNAPSHOT_FAITHFUL;

set_ps_flgs() or's stuff in so it does need this it seems.

>  	memset(&ps->folio_snapshot, 0, sizeof(struct folio));
>  	memcpy(&ps->page_snapshot, page, sizeof(*page));
>  	info = ps->page_snapshot.compound_info;
> @@ -1438,13 +1438,16 @@ void snapshot_page(struct page_snapshot *ps, const struct page *page)
>  	if (ps->idx < MAX_FOLIO_NR_PAGES) {
>  		memcpy(&ps->folio_snapshot, foliop, 2 * sizeof(struct page));
>  		nr_pages = folio_nr_pages(&ps->folio_snapshot);
> +		/* On races, or when freeing pages, we can read 0. */
> +		if (nr_pages == 0)
> +			nr_pages = 1;

Nit but could be:

		/* On races, or when freeing pages, we can read 0. */
  		nr_pages = folio_nr_pages(&ps->folio_snapshot) ?: 1;

>  		if (nr_pages > 2)
>  			memcpy(&ps->folio_snapshot.__page_2, &foliop->__page_2,
>  			       sizeof(struct page));
>  		set_ps_flags(ps, foliop, page);
>  	}
>
> -	if (ps->idx > nr_pages) {
> +	if (ps->idx >= nr_pages) {
>  		if (loops-- > 0)
>  			goto again;
>  		clear_compound_head(&ps->page_snapshot);
> --
> 2.56.1
>
>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo