[PATCH v4] mm/memory-failure: fix folio refcount leak and locking in soft/hard offline

[PATCH v4] mm/memory-failure: fix folio refcount leak and locking in soft/hard offline

[dayou5941]

From: Li Youhong <liyouhong@kylinos.cn>

The min_order_for_split() function accesses folio->mapping without proper
synchronization. In memory_failure(), the folio lock is dropped before the
call, and in soft_offline_in_use_page(), the lock is not held at all. This
means that while min_order_for_split() is executing, the value of
folio->mapping may be modified by a truncate or invalidate operation,
leading to a torn read or use of a stale mapping value.

Additionally, the soft_offline_in_use_page() path fails to release the
folio reference taken by get_hwpoison_page() when new_order != 0, causing a
reference leak.

Fix these issues by:
- Moving the split operation logic into the callers and holding the folio
  lock around min_order_for_split() and split_huge_page_to_order().
- Removing the try_to_split_thp_page() helper to simplify refcount
  handling.
- Ensuring the folio reference is always dropped before returning on the
  soft-offline error path.

This refactors the code to be more maintainable, fixes the locking issue
reported by Sashiko, and also addresses the folio reference leak on the
soft-offline error path.

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
Co-developed-by: David Hildenbrand <david@kernel.org>
Signed-off-by: David Hildenbrand <david@kernel.org>
Signed-off-by: Li Youhong <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/

v3:
- Fix author name and Signed-off-by format as requested by Greg.
  v2: https://lore.kernel.org/all/20260805072625.2437636-1-dayou5941@163.com/

v4:
- Rebased on David Hildenbrand's refactoring patch.
- Added Co-developed-by for David Hildenbrand.
- Supersedes the standalone refcount fix:
  link: https://lore.kernel.org/all/20260804035356.2615408-1-dayou5941@163.com/
- The VM_WARN_ON_ONCE_FOLIO() check in min_order_for_split() will be sent
  as a separate follow-up patch.
  v3: https://lore.kernel.org/all/20260805084224.2597547-1-dayou5941@163.com/

---
 mm/memory-failure.c | 49 +++++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 51508a55c405..665f5b444fb7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1649,26 +1649,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)
 {
@@ -2440,7 +2420,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;
 
 		/*
@@ -2457,24 +2436,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);
 	}
 
 	/*
@@ -2776,6 +2755,9 @@ EXPORT_SYMBOL(unpoison_memory);
  * soft_offline_in_use_page handles hugetlb-pages and non-hugetlb pages.
  * If the page is a non-dirty unmapped page-cache page, it simply invalidates.
  * If the page is mapped, it migrates the contents over.
+ *
+ * The folio refcount has been incremented before entering this function.
+ * This folio reference must be released before the function returns on all paths.
  */
 static int soft_offline_in_use_page(struct page *page)
 {
@@ -2793,8 +2775,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.
@@ -2802,8 +2783,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.25.1

Re: [PATCH v4] mm/memory-failure: fix folio refcount leak and locking in soft/hard offline

[Andrew Morton]

On Thu,  6 Aug 2026 11:19:58 +0800 dayou5941@163.com wrote:

> From: Li Youhong <liyouhong@kylinos.cn>
> 
> The min_order_for_split() function accesses folio->mapping without proper
> synchronization. In memory_failure(), the folio lock is dropped before the
> call, and in soft_offline_in_use_page(), the lock is not held at all. This
> means that while min_order_for_split() is executing, the value of
> folio->mapping may be modified by a truncate or invalidate operation,
> leading to a torn read or use of a stale mapping value.
> 
> Additionally, the soft_offline_in_use_page() path fails to release the
> folio reference taken by get_hwpoison_page() when new_order != 0, causing a
> reference leak.
> 
> Fix these issues by:
> - Moving the split operation logic into the callers and holding the folio
>   lock around min_order_for_split() and split_huge_page_to_order().
> - Removing the try_to_split_thp_page() helper to simplify refcount
>   handling.
> - Ensuring the folio reference is always dropped before returning on the
>   soft-offline error path.
> 
> This refactors the code to be more maintainable, fixes the locking issue
> reported by Sashiko, and also addresses the folio reference leak on the
> soft-offline error path.
> 
> ...
>
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
>  static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
>  		struct address_space *mapping, pgoff_t index, int flags)
>  {
> @@ -2440,7 +2420,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;
>  
>  		/*
> @@ -2457,24 +2436,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);

AI review asks (effectively) why the try_to_split_thp_page() return value
never gets used

	https://sashiko.dev/#/patchset/20260806031958.677935-1-dayou5941@163.com

> +
> +		lock_page(p);

This code is a maddening mixture of `pages' and `folios'.  I assume
that migrating it over is a work in progress.

> +		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);

We did that 40 lines earlier?

	folio = page_folio(p);

	/* filter pages that are protected from hwpoison test by users */
	folio_lock(folio);

Re: [PATCH v4] mm/memory-failure: fix folio refcount leak and locking in soft/hard offline

[David Hildenbrand (Arm)]

On 8/6/26 06:34, Andrew Morton wrote:
> On Thu,  6 Aug 2026 11:19:58 +0800 dayou5941@163.com wrote:
> 
>> From: Li Youhong <liyouhong@kylinos.cn>
>>
>> The min_order_for_split() function accesses folio->mapping without proper
>> synchronization. In memory_failure(), the folio lock is dropped before the
>> call, and in soft_offline_in_use_page(), the lock is not held at all. This
>> means that while min_order_for_split() is executing, the value of
>> folio->mapping may be modified by a truncate or invalidate operation,
>> leading to a torn read or use of a stale mapping value.
>>
>> Additionally, the soft_offline_in_use_page() path fails to release the
>> folio reference taken by get_hwpoison_page() when new_order != 0, causing a
>> reference leak.
>>
>> Fix these issues by:
>> - Moving the split operation logic into the callers and holding the folio
>>   lock around min_order_for_split() and split_huge_page_to_order().
>> - Removing the try_to_split_thp_page() helper to simplify refcount
>>   handling.
>> - Ensuring the folio reference is always dropped before returning on the
>>   soft-offline error path.
>>
>> This refactors the code to be more maintainable, fixes the locking issue
>> reported by Sashiko, and also addresses the folio reference leak on the
>> soft-offline error path.
>>
>> ...
>>
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>>  static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
>>  		struct address_space *mapping, pgoff_t index, int flags)
>>  {
>> @@ -2440,7 +2420,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;
>>  
>>  		/*
>> @@ -2457,24 +2436,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);
> 
> AI review asks (effectively) why the try_to_split_thp_page() return value
> never gets used
> 
> 	https://sashiko.dev/#/patchset/20260806031958.677935-1-dayou5941@163.com
> 

No need for AI. Just from compiling:

  └ mm/memory-failure.c: In function ‘memory_failure’:
    mm/memory-failure.c:2492:21: warning: variable ‘err’ set but not used
[-Wunused-but-set-variable]
     2492 |                 int err;


I'll send out a patch myself that already addresses everything raised in here
without even me reading any of it.

-- 
Cheers,

David