Re: [PATCH v2 1/5] mm/memory-failure: keep the folio, not the poisoned subpage, locked across split

["David Hildenbrand (Arm)" <david@kernel.org>]

On 7/14/26 16:53, Kiryl Shutsemau wrote:
> On Tue, Jul 14, 2026 at 03:01:46PM +0200, David Hildenbrand (Arm) wrote:
>> On 7/14/26 14:23, Kiryl Shutsemau wrote:
>>> From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
>>>
>>> try_to_split_thp_page() locked the poisoned page and passed it to
>>> split_huge_page_to_order(), which returns that very page locked to the
>>> caller.  For a tail page that means __folio_split() runs with @lock_at
>>> pointing into the middle of the folio.
>>>
>>> __folio_split() dereferences the mapping after the split completes
>>> (shmem_uncharge(), i_mmap_unlock_read()).  The only thing keeping the
>>> inode alive across that is the locked @lock_at folio: while it stays in
>>> the page cache, eviction cannot complete.
>>>
>>> But a tail @lock_at can lie beyond EOF -- e.g. part of a shmem THP that
>>> reaches past i_size while the file is being truncated.  The split then
>>> drops it from the page cache yet still returns it locked, so the pin is
>>> gone and a racing final iput() can evict and RCU-free the inode while
>>> __folio_split() is still running:
>>>
>>>   BUG: KASAN: slab-use-after-free in __up_read+0x634/0x790
>>>    i_mmap_unlock_read include/linux/fs.h:537 [inline]
>>>    __folio_split+0x732/0x1640 mm/huge_memory.c:4100
>>>    try_to_split_thp_page+0xab/0x390 mm/memory-failure.c:1675
>>>    memory_failure+0x1394/0x26e0 mm/memory-failure.c:2470
>>>
>>>   Freed by task 4601:
>>>    shmem_free_in_core_inode+0x54/0xb0 mm/shmem.c:5177
>>>    evict+0x57f/0xac0 fs/inode.c:870
>>>
>>> Split the folio as a folio, via split_folio_to_order(), so the head is
>>> the anchor left locked.  The head is piece 0, which the beyond-EOF drop
>>> loop never removes (it starts at folio_next(folio)), so the split always
>>> leaves it in the page cache and the inode stays pinned for the whole of
>>> __folio_split().  memory_failure() and soft offline re-lock the poisoned
>>> subpage's folio themselves after the split, so they do not depend on it
>>> being returned locked.
>>>
>>> Reported-by: Hao Zhang <zhanghao1@kylinos.cn>
>>> Closes: https://lore.kernel.org/linux-mm/20260710071344.GA106129@zh-pc
>>> Fixes: baa355fd3314 ("thp: file pages support for split_huge_page()")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
>>> ---
>>>  mm/memory-failure.c | 13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index 51508a55c405..68d42cbed458 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -1657,11 +1657,18 @@ static int identify_page_state(unsigned long pfn, struct page *p,
>>>  static int try_to_split_thp_page(struct page *page, unsigned int new_order,
>>>  		bool release)
>>>  {
>>> +	struct folio *folio = page_folio(page);
>>>  	int ret;
>>>  
>>> -	lock_page(page);
>>> -	ret = split_huge_page_to_order(page, new_order);
>>> -	unlock_page(page);
>>> +	/*
>>> +	 * Lock and split at the head, not the poisoned subpage: __folio_split()
>>> +	 * keeps the anchor folio locked and needs it to stay in the page cache
>>> +	 * to pin the inode. A tail beyond EOF would be dropped yet returned
>>> +	 * locked, losing that pin. The caller re-locks @page afterwards.
>>> +	 */
>>> +	folio_lock(folio);
>>> +	ret = split_folio_to_order(folio, new_order);
>>> +	folio_unlock(folio);
>>
>> With a non-uniform split it would actually make a difference: we'd want to split
>> such that we the other folio pages minimal.
>>
>>  split_folio_to_order() always seems to end up in
>> __split_huge_page_to_list_to_order() where we do a SPLIT_TYPE_UNIFORM.
>>
>> I recall discussing with Zi and Willy that in the future we'd want to convert
>> more places to do a non-uniform split.
>>
>> So I'm afraid that would just re-introduce the problem then.
> 
> Right. Non-uniform split can be useful.
> 
> But my patch is completely broken because code expects the pin to be on the
> @page, not on the head. put_page() few lines down can explode already.

Yeah.

> 
> So the fix does not belong in memory_failure(). It belongs in
> __folio_split(), and it is really just 2/5: refuse the split with -EBUSY
> when @lock_at is at or beyond the sampled EOF.
> 
> The safety then sits in __folio_split() regardless of caller or split type,
> which should also cover your non-uniform worry.
> 
> The behavioural change is that memory_failure() reports a beyond-EOF
> poisoned tail as unsplit (MF_FAILED) instead of recovered, and kills the
> mappers instead of splitting the page off. What we give up is salvaging
> the folio's healthy pages and the clean unmap -- both low value for a page
> that is beyond EOF and getting truncated away. Containment is unaffected:
> PageHWPoison is set before the split and free_pages_prepare() keeps a
> poisoned page out of the buddy allocator, so the bad page never comes back
> regardless.
> 
> The alternative, if we would rather keep the recovered outcome, is to leave
> @lock_at as the poisoned page and move i_mmap_unlock_read() (and
> shmem_uncharge()) ahead of the after-split unlock loop, so every mapping
> dereference happens while @folio -- the head, within EOF -- still pins the
> inode. That is close to Hao's original patch. It works, but it rests on "no
> mapping dereference after the unlock loop", which is its own fragility.

At least it can be well documented.

> 
> I lean towards the -EBUSY guard.

The latter approach seems cleaner to me, but let's hear others as well.

-- 
Cheers,

David