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

["David Hildenbrand (Arm)" <david@kernel.org>]

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