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

[Zi Yan]

On 14 Jul 2026, at 10:58, David Hildenbrand (Arm) wrote:

> 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@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>> Closes: https://lore.kernel.org/linux-mm/20260710071344.GA106129@zh-pc
>>>> Fixes: baa355fd3314 ("thp: file pages support for split_huge_page()")
>>>> Cc: <stable@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Kiryl Shutsemau (Meta) <kas@xxxxxxxxxx>
>>>> ---
>>>>  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.

There is an alternative, only igrab() when @lock_at is at or beyond the EOF,
as I was bouncing ideas with Codex.

Some subtlety exists in these two “@lock_at at or beyond EOF” approaches.
When @lock_at can be a tail page of an after-split folio, Patch 2 returns
-EBUSY unnecessarily, since the after-split folio containing @lock_at will
be still in xarray, preventing inode going away. To make a precise decision,
we will need to determine the index of the after-split folio containing @lock_at
when checking against end. It is easy for uniform split, namely

lock_at_folio_index =
	folio->index + round_down(lock_at - &folio->page, 1UL << new_order);

but for non-uniform split, the calculation is more involved and I have not
figured it out yet.

If we go “return -EBUSY if @lock_at index is at or beyond EOF”, are we OK
with not being able to split a folio when it is actually splittable?


>>
>> 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.

This approach works without changing existing behavior, as long as we document
the requirement well.

I admit that this is another implicit synchronization in folio split process,
in addition to
1) keeping original folio frozen until xarray is updated and
2) do not update xarray with after-split folios until after-split folios are
unfrozen.

>
>>
>> I lean towards the -EBUSY guard.
>
> The latter approach seems cleaner to me, but let's hear others as well.

I prefer moving i_mmap_unlock_read() before the unlock loop (shmem_uncharge()
is already before the loop), but I am more than happy to be convinced that
“return -EBUSY” is better or something else.

Best Regards,
Yan, Zi