[PATCH] mm/filemap: __filemap_add_folio() restore index before retrying

[PATCH] mm/filemap: __filemap_add_folio() restore index before retrying

[Hugh Dickins]

In __filemap_add_folio()'s split-a-conflict loop, xas_set_order() is
applied repeatedly: each application modifies xas.xa_index, rounding it
down according to the split_order attempted at that stage: and if all
goes as intended, it eventually (or immediately) converges on an
xas_try_split() to the required folio_order, with xas.xa_index now the
same as index: then xas_store() puts the new folio into the xarray there.

But if a new node was needed, and GFP_NOWAIT allocation did not get one,
the lock is dropped, xas_nomem() used to allocate, and sequence retried.
If (that part of) the xarray is unchanged when the lock is reacquired,
no problem. But what if the conflict was meanwhile resolved by another
thread (perhaps even doing the same thing, inserting a folio at that same
index)? Isn't there a danger of now putting our folio into the xarray at
an intermediate rounded-down index? With !folio_contains() bug to follow,
when CONFIG_DEBUG_VM=y is checking for that.

Fix this with an xas_set_order() to restore the original xas.xa_index at
the bottom of the loop, so the retry does a full re-evaluation after
reacquiring the lock, and cannot reach xas_store() with the wrong index.

Production was suffering from rare SIGILLs and SIGSEGVs, executable text
found a page away from where it belonged, !folio_contains() bug hit when
debug enabled: symptoms not seen since this patch went in.

Fixes: 200a89c159a7 ("mm/filemap: use xas_try_split() in __filemap_add_folio()")
Signed-off-by: Hugh Dickins <hughd@google.com>
Cc: stable@vger.kernel.org
---
 mm/filemap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index 58eb9d240643..d721986d5f46 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -931,6 +931,12 @@ noinline int __filemap_add_folio(struct address_space *mapping,
 
 		if (!xas_nomem(&xas, gfp))
 			break;
+
+		/*
+		 * Lock has been dropped: start again with the original index
+		 * and order (but now with the memory reserved by xas_nomem()).
+		 */
+		xas_set_order(&xas, index, forder);
 	}
 
 	if (xas_error(&xas))

Re: [PATCH] mm/filemap: __filemap_add_folio() restore index before retrying

[Kiryl Shutsemau]

On Mon, Jul 27, 2026 at 10:24:14PM -0700, Hugh Dickins wrote:
> In __filemap_add_folio()'s split-a-conflict loop, xas_set_order() is
> applied repeatedly: each application modifies xas.xa_index, rounding it
> down according to the split_order attempted at that stage: and if all
> goes as intended, it eventually (or immediately) converges on an
> xas_try_split() to the required folio_order, with xas.xa_index now the
> same as index: then xas_store() puts the new folio into the xarray there.
> 
> But if a new node was needed, and GFP_NOWAIT allocation did not get one,
> the lock is dropped, xas_nomem() used to allocate, and sequence retried.
> If (that part of) the xarray is unchanged when the lock is reacquired,
> no problem. But what if the conflict was meanwhile resolved by another
> thread (perhaps even doing the same thing, inserting a folio at that same
> index)? Isn't there a danger of now putting our folio into the xarray at
> an intermediate rounded-down index? With !folio_contains() bug to follow,
> when CONFIG_DEBUG_VM=y is checking for that.
> 
> Fix this with an xas_set_order() to restore the original xas.xa_index at
> the bottom of the loop, so the retry does a full re-evaluation after
> reacquiring the lock, and cannot reach xas_store() with the wrong index.
> 
> Production was suffering from rare SIGILLs and SIGSEGVs, executable text
> found a page away from where it belonged, !folio_contains() bug hit when
> debug enabled: symptoms not seen since this patch went in.
> 
> Fixes: 200a89c159a7 ("mm/filemap: use xas_try_split() in __filemap_add_folio()")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: stable@vger.kernel.org

Makes sense to me.

Acked-by: Kiryl Shutsemau (Meta) <kas@kernel.org>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH] mm/filemap: __filemap_add_folio() restore index before retrying

[Matthew Wilcox]

On Mon, Jul 27, 2026 at 10:24:14PM -0700, Hugh Dickins wrote:
> Production was suffering from rare SIGILLs and SIGSEGVs, executable text
> found a page away from where it belonged, !folio_contains() bug hit when
> debug enabled: symptoms not seen since this patch went in.

That can't have been fun to find.  Thanks, Hugh!

> Fixes: 200a89c159a7 ("mm/filemap: use xas_try_split() in __filemap_add_folio()")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH] mm/filemap: __filemap_add_folio() restore index before retrying

[Zi Yan]

On Tue Jul 28, 2026 at 1:24 AM EDT, Hugh Dickins wrote:
> In __filemap_add_folio()'s split-a-conflict loop, xas_set_order() is
> applied repeatedly: each application modifies xas.xa_index, rounding it
> down according to the split_order attempted at that stage: and if all
> goes as intended, it eventually (or immediately) converges on an
> xas_try_split() to the required folio_order, with xas.xa_index now the
> same as index: then xas_store() puts the new folio into the xarray there.
>
> But if a new node was needed, and GFP_NOWAIT allocation did not get one,
> the lock is dropped, xas_nomem() used to allocate, and sequence retried.
> If (that part of) the xarray is unchanged when the lock is reacquired,
> no problem. But what if the conflict was meanwhile resolved by another
> thread (perhaps even doing the same thing, inserting a folio at that same
> index)? Isn't there a danger of now putting our folio into the xarray at
> an intermediate rounded-down index? With !folio_contains() bug to follow,
> when CONFIG_DEBUG_VM=y is checking for that.
>
> Fix this with an xas_set_order() to restore the original xas.xa_index at
> the bottom of the loop, so the retry does a full re-evaluation after
> reacquiring the lock, and cannot reach xas_store() with the wrong index.
>
> Production was suffering from rare SIGILLs and SIGSEGVs, executable text
> found a page away from where it belonged, !folio_contains() bug hit when
> debug enabled: symptoms not seen since this patch went in.
>
> Fixes: 200a89c159a7 ("mm/filemap: use xas_try_split() in __filemap_add_folio()")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: stable@vger.kernel.org
> ---
>  mm/filemap.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>

Thank you fro the fix.

Reviewed-by: Zi Yan <ziy@nvidia.com>


-- 
Best Regards,
Yan, Zi