Re: [PATCH v3 1/3] mm/truncate: fix data loss when splitting straddling large folios fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mm: data loss in hole punch due to ABA in speculative folio lookup
- [High] mm: memory leak in hole punch due to skipped successfully split order-0 folios
--

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

mm/truncate: fix data loss when splitting straddling large folios fails

This commit modifies truncate_inode_partial_folio to return out-parameters
pstart and pend indicating the fully covered page range safe to discard
after any split. This is intended to fix a data loss issue when splitting
a straddling large folio fails.

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

> diff --git a/mm/truncate.c b/mm/truncate.c
> index b58ba940be474..bec6d881d0225 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> 
> [ ... ]
> 
> @@ -259,32 +271,62 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
>  		 * for shmem truncate
>  		 */
>  		struct folio *folio2;
> +		pgoff_t end, aligned_end = (pos + offset + length) >>
> +					   PAGE_SHIFT;
>  
> -		if (offset + length == size)
> -			goto no_split;
> +		if (pstart)
> +			*pstart = round_up(pos + offset, PAGE_SIZE) >>
> +				  PAGE_SHIFT;
> +
> +		if (offset + length == size) {
> +			end = aligned_end;
> +			goto out;
> +		}
>  
>  		split_at2 = folio_page(folio,
>  				PAGE_ALIGN_DOWN(offset + length) / PAGE_SIZE);
>  		folio2 = page_folio(split_at2);
>  
> +		/*
> +		 * folio2 may become stale due to a concurrent split or
> +		 * freeing, so validate it before and after taking its lock.
> +		 * If it fails, we can't get an accurate end position and fall
> +		 * back to folio->index, which may leave sub-folios split off
> +		 * at the offset edge in the page cache this round.
> +		 */
> +		end = folio->index;
>  		if (!folio_try_get(folio2))
> -			goto no_split;
> -
> -		if (!folio_test_large(folio2))
>  			goto out;
>  
> +		if (folio2->mapping != folio->mapping ||
> +		    !folio_test_large(folio2))
> +			goto out_put;

[Severity: High]
Could this lead to a memory leak if the first split successfully shatters the
folio to order-0 pages?

If the hole punch starts in the middle of a large folio and the first split
successfully shatters it into order-0 folios (which happens when min_order
is 0), the tail page folio2 becomes an order-0 folio.

The speculative pre-lock check validates folio2 here:

    if (folio2->mapping != folio->mapping ||
        !folio_test_large(folio2))
        goto out_put;

Because folio2 is order-0, !folio_test_large(folio2) evaluates to true.
Execution jumps to out_put without updating end to aligned_end, leaving end
initialized to folio->index.

When pend is returned as folio->index to the caller
(truncate_inode_pages_range), it is used as the upper bound for the
truncation loop. Because end is less than or equal to start, the loop
terminates immediately, skipping the pages within the hole punch range
and leaving them permanently leaked in the page cache.

> +
> +		if (!folio_trylock(folio2))
> -			goto out;
> +			goto out_put;
> +
> +		if (page_folio(split_at2) != folio2) {
> +			folio_unlock(folio2);
> +			goto out_put;
> +		}
> +		if (!folio_test_large(folio2)) {
> +			end = aligned_end;
> +			folio_unlock(folio2);
> +			goto out_put;
> +		}
>  
> -		/* 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);
> +		/* Split failed: back off to the head of the straddler */
> +		if (folio_split_or_unmap(folio2, split_at2, min_order))
> +			end = folio2->index;
> +		else
> +			end = aligned_end;

[Severity: Critical]
Does this speculative lookup handle the reallocation race correctly if the
folio is reallocated to a different file offset?

After the first split, the tail pages are added to the page cache and
unlocked. During speculative lookup, the tail page could be concurrently
reclaimed, freed, and reallocated as a new large folio in the same mapping
but at a completely different file offset.

The validation checks earlier and the page_folio check here will all pass
because the physical page split_at2 is part of the new allocation.

However, the file index of the folio is not verified. If this unrelated large
folio fails to split, the fallback path assigns its index to end here:

    if (folio_split_or_unmap(folio2, split_at2, min_order))
        end = folio2->index;

This unrelated file offset is then returned to the caller via pend, which
can cause the hole punch loop to truncate an unrelated segment of the file.

Should we verify that the file index of folio2 matches the expected index
(e.g. folio2->index + folio_page_idx(folio2, split_at2) == expected_index)
before using it?

>  
>  		folio_unlock(folio2);
> -out:
> +out_put:
>  		folio_put(folio2);
> -no_split:
> +out:
> +		if (pend)
> +			*pend = end;
>  		return true;
>  	}
>  	if (folio_test_dirty(folio))

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