Re: [PATCH v2] mm/memory-failure: fix concurrent access issue in min_order_for_split()

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

On Wed, Aug 05, 2026 at 05:43:40PM +0800, 李佑鸿  wrote:
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> At 2026-08-05 17:07:18, "David Hildenbrand (Arm)" <david@kernel.org> wrote:
> >On 8/5/26 09:26, dayou5941@163.com wrote:
> >> From: liyouhong <liyouhong@kylinos.cn>
> >>
> >> min_order_for_split() accesses folio->mapping without proper
> >> synchronization. While the compiler typically caches the value
> >> in a register making a NULL deref unlikely in practice, the
> >> real issue is that the callers in memory-failure.c do not hold
> >> the folio lock at the time of the call:
> >>
> >> - memory_failure() explicitly drops the folio lock before calling
> >>   min_order_for_split().
> >> - soft_offline_in_use_page() has not yet acquired the folio lock
> >>   when calling min_order_for_split().
> >>
> >> This means the value of folio->mapping may be modified by a
> >> truncate or invalidate operation while min_order_for_split() is
> >> executing, leading to a torn read or use of a stale mapping value.
> >
> >I recall that a mapping can get freed after truncating the last folio.
> >
> >But Lorenzo had some cocnerns about the validity of the report, so I'll let him reply.
> >
> >>
> >> Fixes: 689b8986776c ("mm/memory-failure: improve large block size folio handling")
> >> Reported-by: Sashiko <sashiko-bot@kernel.org>
> >> Closes: https://sashiko.dev/#/patchset/20260803060001.800638-1-dayou5941@163.com
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: liyouhong <liyouhong@kylinos.cn>
> >> ---
> >> v2:
> >> - Dropped the approach of caching folio->mapping inside min_order_for_split() in favor of adding the folio lock at the callers.
> >> - Added VM_WARN_ON_ONCE_FOLIO() in min_order_for_split().
> >> - Updated the commit message to clarify that the real issue is the callers not holding the folio lock, rather than a TOCTOU race.
> >> v1: https://lore.kernel.org/all/20260804035828.2684059-1-dayou5941@163.com/
> >>
> >> ---
> >>  mm/huge_memory.c    |  2 ++
> >>  mm/memory-failure.c | 12 ++++++++++--
> >>  2 files changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> index 58cabe6af33d..e3f16dadc1d4 100644
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -4300,6 +4300,8 @@ int folio_split(struct folio *folio, unsigned int new_order,
> >>   */
> >>  unsigned int min_order_for_split(struct folio *folio)
> >>  {
> >> +	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> >> +
> >>  	if (folio_test_anon(folio))
> >>  		return 0;
> >>
> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >> index 3b1e6946821b..7391524b5046 100644
> >> --- a/mm/memory-failure.c
> >> +++ b/mm/memory-failure.c
> >> @@ -2437,12 +2437,17 @@ int memory_failure(unsigned long pfn, int flags)
> >>  		res = -EOPNOTSUPP;
> >>  		goto unlock_mutex;
> >>  	}
> >> +
> >>  	folio_unlock(folio);
> >>
> >>  	if (folio_test_large(folio)) {
> >> -		const int new_order = min_order_for_split(folio);
> >> +		const int new_order;
> >>  		int err;
> >>
> >> +		folio_lock(folio);
> >> +		new_order = min_order_for_split(folio);
> >> +		folio_unlock(folio);
> >> +
> >>  		/*
> >>  		 * The flag must be set after the refcount is bumped
> >>  		 * otherwise it may race with THP split.
> >> @@ -2796,8 +2801,11 @@ static int soft_offline_in_use_page(struct page *page)
> >>  	};
> >>
> >>  	if (!huge && folio_test_large(folio)) {
> >> -		const int new_order = min_order_for_split(folio);
> >> +		const int new_order;
> >>
> >> +		folio_lock(folio);
> >> +		new_order = min_order_for_split(folio);
> >> +		folio_unlock(folio);
> >>  		/*
> >>  		 * If new_order (target split order) is not 0, do not split the
> >>  		 * folio at all to retain the still accessible large folio.
> >
> >We already grab the page lock in try_to_split_thp_page(). I think these
> >needs a serious cleanup.
> >
> >While at it, we can fix the missing put_page() in case new_order != 0 in
> >soft_offline_in_use_page() ...
> >
> >So i think we really should do the following:
> >
> >
> >From 58c4ccbb04e13c4c14ef7a94f4b44f8901183081 Mon Sep 17 00:00:00 2001
> >From: "David Hildenbrand (Arm)" <david@kernel.org>
> >Date: Wed, 5 Aug 2026 11:01:46 +0200
> >Subject: [PATCH] tmp
> >
> >Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
> >---
> > mm/memory-failure.c | 46 +++++++++++++++------------------------------
> > 1 file changed, 15 insertions(+), 31 deletions(-)
> >
> >diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >index aaf14608b30e2..de97a7a36764d 100644
> >--- a/mm/memory-failure.c
> >+++ b/mm/memory-failure.c
> >@@ -1705,26 +1705,6 @@ static int identify_page_state(unsigned long pfn, struct page *p,
> > 	return page_action(ps, p, pfn);
> > }
> >
> >-/*
> >- * When 'release' is 'false', it means that if thp split has failed,
> >- * there is still more to do, hence the page refcount we took earlier
> >- * is still needed.
> >- */
> >-static int try_to_split_thp_page(struct page *page, unsigned int new_order,
> >-		bool release)
> >-{
> >-	int ret;
> >-
> >-	lock_page(page);
> >-	ret = split_huge_page_to_order(page, new_order);
> >-	unlock_page(page);
> >-
> >-	if (ret && release)
> >-		put_page(page);
> >-
> >-	return ret;
> >-}
> >-
> > static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
> > 		struct address_space *mapping, pgoff_t index, int flags)
> > {
> >@@ -2509,7 +2489,6 @@ int memory_failure(unsigned long pfn, int flags)
> > 	folio_unlock(folio);
> >
> > 	if (folio_test_large(folio)) {
> >-		const int new_order = min_order_for_split(folio);
> > 		int err;
> >
> > 		/*
> >@@ -2526,24 +2505,24 @@ int memory_failure(unsigned long pfn, int flags)
> > 		 * page is a valid handlable page.
> > 		 */
> > 		folio_set_has_hwpoisoned(folio);
> >-		err = try_to_split_thp_page(p, new_order, /* release= */ false);
> >+
> >+		lock_page(p);
> >+		err = split_huge_page_to_order(p, min_order_for_split(folio));
> >+		unlock_page(p);
> > 		/*
> > 		 * If splitting a folio to order-0 fails, kill the process.
> > 		 * Split the folio regardless to minimize unusable pages.
> > 		 * Because the memory failure code cannot handle large
> > 		 * folios, this split is always treated as if it failed.
> > 		 */
> >-		if (err || new_order) {
> >-			/* get folio again in case the original one is split */
> >-			folio = page_folio(p);
> >+		folio = page_folio(p);
> >+		if (folio_test_large(folio)) {
> > 			res = -EHWPOISON;
> > 			kill_procs_now(p, pfn, flags, folio);
> > 			put_page(p);
> > 			action_result(pfn, MF_MSG_UNSPLIT_THP, MF_FAILED);
> > 			goto unlock_mutex;
> > 		}
> >-		VM_BUG_ON_PAGE(!page_count(p), p);
> >-		folio = page_folio(p);
> > 	}
> >
> > 	/*
> >@@ -2862,8 +2841,7 @@ static int soft_offline_in_use_page(struct page *page)
> > 	};
> >
> > 	if (!huge && folio_test_large(folio)) {
> >-		const int new_order = min_order_for_split(folio);
> >-
> >+		lock_page(page);
> > 		/*
> > 		 * If new_order (target split order) is not 0, do not split the
> > 		 * folio at all to retain the still accessible large folio.
> >@@ -2871,8 +2849,14 @@ static int soft_offline_in_use_page(struct page *page)
> > 		 * preferred, split it to non-zero new_order like it is done in
> > 		 * memory_failure().
> > 		 */
> >-		if (new_order || try_to_split_thp_page(page, /* new_order= */ 0,
> >-						       /* release= */ true)) {
> >+		if (!min_order_for_split(folio))
> >+			ret = split_huge_page_to_order(page, 0);
> >+		else
> >+			ret = -EBUSY;
> >+		unlock_page(page);
> >+
> >+		if (ret) {
> >+			put_page(page);
> > 			pr_info("%#lx: thp split failed\n", pfn);
> > 			return -EBUSY;
> > 		}
> >--
> >2.43.0
> >
> >
> >Likely some more cleanups on top are possible.
>
> >
>
>
> Thanks David and Lorenzo for the detailed feedback.
>
> For v3 I plan to:

v4 you mean :) you already sent a v3 way too quick.

Please send this tomorrow, not today.

>
> 1. Take David's patch as the base, with his Signed-off-by preserved.

That's only if you added a Co-developed-by, David can indicate what he wants.

> 2. Add my VM_WARN_ON_ONCE_FOLIO() in min_order_for_split() as a
>    separate patch.

Yup.

> 3. Address Lorenzo's comment about returning ret directly in
>    soft_offline_in_use_page().

You should respond to review inline.

Just take take David's patch + the VM_WARN_ON_ONCE().

> 4. Fold my earlier standalone put_page() fix into this series
>    and mark it as superseded.
>    link：https://lore.kernel.org/all/20260804035356.2615408-1-dayou5941@163.com/

Not sure what folded means?

Again as above, just take David's patch + the VM_WARN_ON_ONCE().

> 5. Split the series logically if needed (e.g., one patch for
>    removing try_to_split_thp_page(), one for the lock fix, one
>    for the put_page() fix).

No because you'll create bisection hazards. Just 1 patch please.

>
> Please let me know if this plan looks reasonable or if you'd prefer
> a different approach.

As above.

--
Cheers, Lorenzo