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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

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