Re: [PATCH v2] mm/truncate: fix data loss when truncating straddling large folios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Speculative folio access without proper re-validation causes corrupted truncation bounds and massive data loss.
--

commit fca88c7c0a18efec3a5bc4b07763e9cc7f4d03f7
Author: Zhang Yi <yi.zhang@huawei.com>

mm/truncate: fix data loss when truncating straddling large folios
    
This commit fixes a data loss issue where a straddling large folio
might be incorrectly truncated if the second split fails. It modifies
truncate_inode_partial_folio() to clarify return semantics and return
the safe-to-discard page range to the caller via out-parameters.

Link: https://lore.kernel.org/linux-fsdevel/anH-WKA1coW6wtfG@bfoster/

> diff --git a/mm/truncate.c b/mm/truncate.c
> --- a/mm/truncate.c
> +++ b/mm/truncate.c

[ ... ]

> @@ -259,38 +266,67 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)

[ ... ]

>  		if (!folio_try_get(folio2))
> -			goto no_split;
> +			goto split;
>  
>  		if (!folio_test_large(folio2))
>  			goto out;
>  
> -		if (!folio_trylock(folio2))
> +		if (!folio_trylock(folio2)) {
> +			tail_isolated = false;
>  			goto out;
> +		}

[Severity: Critical]
Can jumping to the out label here bypass the validation of the
speculatively looked-up folio?

If folio2 was evicted and the physical page was reallocated to an unrelated
mapping (like an anonymous THP) right before folio_try_get(folio2),
folio_trylock(folio2) could fail because the new owner holds the lock.

> +
> +		/*
> +		 * split_at2 may no longer belong to folio2 due to concurrent
> +		 * split. Retry to find the correct folio in case it's still
> +		 * large.
> +		 */
> +		if (page_folio(split_at2) != folio2) {
> +			folio_unlock(folio2);
> +			folio_put(folio2);
> +			goto retry;
> +		}
>  
>  		/* 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);
> +		    folio2->mapping == folio->mapping &&
> +		    folio_split_or_unmap(folio2, split_at2, min_order))
> +			tail_isolated = false;
>  
>  		folio_unlock(folio2);
>  out:
> +		if (!tail_isolated && pend)
> +			*pend = folio2->index;

[Severity: Critical]
Does this assignment risk reading folio2->index from an unrelated folio?

If the folio_trylock(folio2) failure above jumps here without re-validating
that page_folio(split_at2) equals folio2, we could read the index of a
completely different folio.

If that happens, could truncate_inode_pages_range() receive a garbage,
arbitrarily large index for its truncation end boundary and unintentionally
drop all pages up to that index?

>  		folio_put(folio2);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909062339.473816-1-yi.zhang@huaweicloud.com?part=1