Re: [PATCH v4 2/4] mm/truncate: look up the end-edge straddler by index

[Jan Kara]

On Tue 22-09-26 19:07:01, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> In truncate_inode_partial_folio(), after the first split at the start
> edge, folio_split() unlocks and drops the refcount of the after-split
> sub-folios. The sub-folio straddling the end of the truncation range is
> therefore unlocked and only transiently ref'd in the page cache while
> the code still derives it from a page pointer inside the original folio.
> 
> Between the first split finishing and page_folio() resolving split_at2,
> that tail page can be reclaimed, freed and reallocated as a new large
> folio in the same mapping at a different file offset. folio2 then points
> at a folio that does not cover the end boundary, yet
> folio2->mapping == folio->mapping still holds, so the stale pointer
> passes the mapping check and folio_split_or_unmap() splits a folio at a
> wrong position (or, with a transient refcount, a use-after-free window
> opens between try_get and the split). __folio_split()'s own
> folio != page_folio(split_at) check cannot catch this either since
> split_at2 has been reallocated as part of the new folio, so
> page_folio(split_at2) resolves back to folio2.
> 
> Look the straddler up by its page index instead. __filemap_get_folio()
> returns the folio currently covering the boundary, ref'd and locked,
> with the mapping validated under the lock, so the split target is always
> the real folio at the end edge.
> 
> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> Link: https://sashiko.dev/#/message/20260916094500.C30061F00893%40smtp.kernel.org
> Link: https://lore.kernel.org/linux-mm/DLGXT0ERY79Z.3C5DYVJVX6S9Z@xxxxxxxxxx/
> Fixes: 7460b470a131 ("mm/truncate: use folio_split() in truncate operation")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Suggested-by: Zi Yan <ziy@xxxxxxxxxx>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  mm/truncate.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index f9625bb4916f..a8a179b38252 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -259,30 +259,32 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
>  		 * for shmem truncate
>  		 */
>  		struct folio *folio2;
> +		pgoff_t end_idx;
>  
>  		if (offset + length == size)
>  			goto no_split;
>  
> -		split_at2 = folio_page(folio,
> -				PAGE_ALIGN_DOWN(offset + length) / PAGE_SIZE);
> -		folio2 = page_folio(split_at2);
> -
> -		if (!folio_try_get(folio2))
> +		/*
> +		 * After the first split at the start edge, the folio at the
> +		 * end edge may be freed and reused concurrently.
> +		 * __filemap_get_folio() looks up the straddler at end_idx
> +		 * and returns it locked and ref'd with the mapping
> +		 * validated.
> +		 */
> +		end_idx = (pos + offset + length) >> PAGE_SHIFT;
> +		folio2 = __filemap_get_folio(folio->mapping, end_idx,
> +					     FGP_LOCK | FGP_NOWAIT, 0);
> +		if (IS_ERR(folio2))
>  			goto no_split;
>  
> +		/* make sure folio2 is large */
>  		if (!folio_test_large(folio2))
>  			goto out;
>  
> -		if (!folio_trylock(folio2))
> -			goto out;
> -
> -		/* make sure folio2 is large and does not change its mapping */
> -		if (folio_test_large(folio2) &&
> -		    folio2->mapping == folio->mapping)
> -			folio_split_or_unmap(folio2, split_at2, min_order);
> -
> -		folio_unlock(folio2);
> +		split_at2 = folio_page(folio2, (end_idx - folio2->index));
> +		folio_split_or_unmap(folio2, split_at2, min_order);
>  out:
> +		folio_unlock(folio2);
>  		folio_put(folio2);
>  no_split:
>  		return true;
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR