Re: [PATCH] mm: thp: pin the inode across a file folio split

[Zi Yan]

On Mon Jul 13, 2026 at 1:09 PM EDT, Kiryl Shutsemau wrote:
> From: "Kiryl Shutsemau (Meta)" <kirill@xxxxxxxxxxxxx>
>
> __folio_split() looks up mapping = folio->mapping for a file-backed
> folio and keeps dereferencing it after the split completes:
> shmem_uncharge(mapping->host) for folios dropped beyond EOF and
> i_mmap_unlock_read(mapping) on the way out.
>
> Nothing holds an inode reference for that duration. The split relies on
> the folio the caller keeps locked (@lock_at) to pin the inode through
> the page cache: while it is locked and present,
> truncate_inode_pages_final() in evict() cannot make progress. But the
> split drops @lock_at from the page cache when it falls beyond EOF (the
> @end handling in __folio_freeze_and_split_unmapped()), while keeping it
> locked for the caller. That removes the last pin, and a concurrent final
> iput() can then evict and RCU-free the inode before __folio_split() is
> done touching mapping.
>
> This is reachable from memory_failure(): poisoning a tail page of a
> shmem THP that straddles EOF makes try_to_split_thp_page() split at that
> page, so the dropped @lock_at is the folio returned locked. The result
> is a use-after-free, e.g.:
>
>   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
>    i_callback+0x4c/0xa0 fs/inode.c:326
>    destroy_inode+0x144/0x1e0 fs/inode.c:402
>    evict+0x57f/0xac0 fs/inode.c:870
>
> Pin the inode with igrab() before the split and drop the reference with
> iput() after the last mapping dereference. igrab() returns NULL only if
> the inode is already being evicted (i_count 0 and I_FREEING set), which
> a split racing eviction can observe; there is nothing safe to split
> then, so return -EBUSY, which callers already handle.

I was thinking maybe we could move the EOF folio drop code in the unlock
loop to avoid the shmem_uncharge() issue you mentioned in the Closes.
But that hides this implicit dependency (I did not know about this inode
lifetime issue until this patch comes out). So I agree that an explicit
inode pinning is a much better solution.

>
> 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) <kirill@xxxxxxxxxxxxx>
> ---
>  mm/huge_memory.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 2bccb0a53a0a..9bfa3a879453 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3982,6 +3982,7 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  	bool is_anon = folio_test_anon(folio);
>  	struct address_space *mapping = NULL;
>  	struct anon_vma *anon_vma = NULL;
> +	struct inode *inode = NULL;
>  	int old_order = folio_order(folio);
>  	struct folio *new_folio, *next;
>  	int nr_shmem_dropped = 0;
> @@ -4053,6 +4054,20 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  		}
>  
>  		anon_vma = NULL;
> +
> +		/*
> +		 * The locked @lock_at folio keeps the inode alive: eviction
> +		 * cannot remove it from the page cache while it is locked. But
> +		 * the split drops it if it lies beyond EOF, after which we
> +		 * still touch @mapping (shmem_uncharge(), i_mmap_unlock_read()).
> +		 * Hold an inode reference across the split to be safe.
> +		 */
> +		inode = igrab(mapping->host);
> +		if (!inode) {
> +			/* Inode is being evicted; nothing to split. */
> +			ret = -EBUSY;
> +			goto out;
> +		}
>  		i_mmap_lock_read(mapping);
>  
>  		/*
> @@ -4135,6 +4150,8 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  	}
>  	if (mapping)
>  		i_mmap_unlock_read(mapping);
> +	if (inode)
> +		iput(inode);
>  out:
>  	xas_destroy(&xas);
>  	if (is_pmd_order(old_order))
>
> base-commit: 0e35b9b6ec0ffcc5e23cbdec09f5c622ad532b53

The change makes sense to me. Thank you.

Acked-by: Zi Yan <ziy@xxxxxxxxxx>

-- 
Best Regards,
Yan, Zi