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

On 8/5/26 09:26, [email protected] wrote:
> From: liyouhong <[email protected]>
> 
> 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 <[email protected]>
> Closes: https://sashiko.dev/#/patchset/[email protected]
> Cc: [email protected]
> Signed-off-by: liyouhong <[email protected]>
> ---
> 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/[email protected]/
> 
> ---
>  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)" <[email protected]>
Date: Wed, 5 Aug 2026 11:01:46 +0200
Subject: [PATCH] tmp

Signed-off-by: David Hildenbrand (Arm) <[email protected]>
---
 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.

-- 
Cheers,

David