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

On 8/6/26 06:34, Andrew Morton wrote:
> On Thu,  6 Aug 2026 11:19:58 +0800 [email protected] wrote:
> 
>> From: Li Youhong <[email protected]>
>>
>> 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/[email protected]
> 

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