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

[Zhang Yi <yizhang089@gmail.com>]

On 9/17/2026 2:03 AM, Zi Yan wrote:
> On Wed Sep 16, 2026 at 8:02 AM EDT, Jan Kara wrote:
>> On Wed 16-09-26 17:24:48, Zhang Yi wrote:
>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>
>>> truncate_inode_partial_folio() splits a large folio so that the caller's
>>> truncate loop can drop the in-range sub-folios while keeping the
>>> out-of-range tail. The first split at the punch start edge is
>>> non-uniform, which leaves the sub-folio at the truncation end edge as
>>> large as possible, this means it may still straddle the range, holding
>>> both zeroed in-range and valid out-of-range data. The function then
>>> attempts a second split at offset + length to isolate that tail.
>>>
>>> If the second split fails the straddling sub-folio stays merged. The
>>> function returned true unconditionally on all exit paths of the success
>>> block, telling the caller it was fully handled. The caller kept its
>>> default end and the truncate loop truncated every sub-folio below it,
>>> including the merged straddler, discarding the valid out-of-range tail.
>>>
>>> For example, a 4-page order-2 folio punched from offset 0 to the middle
>>> of the last page:
>>>
>>>    truncate_inode_pages_range()
>>>      truncate_inode_partial_folio()      # same_folio == true
>>>        1st split at page0 -> [p0, p1, p2-3]   # non-uniform, success
>>>        folio2 = p2-3 # straddles: p2 zeroed, p3 tail valid
>>>        2nd split of folio2 fails / cannot lock
>>>        return true                       # BUG: caller keeps default end
>>>      end = 3
>>>      loop truncates p0, p1, p2-3        # p3's valid tail is lost
>>>
>>> This became reachable after commit 7460b470a131 ("mm/truncate: use
>>> folio_split() in truncate operation") replaced the atomic split_folio()
>>> with folio_split(), whose non-uniform split can partially split a folio
>>> and leave the end edge merged.
>>>
>>> It has gone unnoticed because a dirty large folio normally carries the
>>> filesystem's private data, for example buffer_head, so
>>> filemap_release_folio() -> iomap_release_folio() returns false on a
>>> dirty folio and folio_split() aborts with -EBUSY before any split,
>>> leaving the straddler safely unsplit. The bug is only reachable on paths
>>> that produce dirty large folios without filesystem private data, and it
>>> was caught on the upcoming ext4 iomap buffered I/O path when no ifs is
>>> attached.
>>>
>>> Rework the contract so the caller is told the page range to discard:
>>>
>>>    - Add pgoff_t *pstart and *pend out-parameters that receive the page
>>>      range fully covered by [lstart, lend] after any split (or none),
>>>      i.e. the pages wholly within the range and safe to discard.
>>>
>>>    - Adjust the ordering of the validate check when splitting folio2.
>>>      folio2->index is only reliable after the reference count and lock
>>>      have been successfully acquired, since it may have been split
>>>      concurrently, or freed and recycled to an unrelated mapping. On any
>>>      failure to obtain a reliable end position, fall back to
>>>      folio->index, which is safe but leaves the sub-folios split off at
>>>      the offset edge in the page cache.
>>>
>>>    - Rename the byte-range parameters start/end to lstart/lend to better
>>>      express their semantics.
>>>
>>> Callers in truncate_inode_pages_range() and shmem_undo_range() pass
>>> &pstart for the folio at the start edge and &pend for the folio at the
>>> end edge, so the truncate loop drops exactly the fully covered pages and
>>> never touches a straddling folio that still holds valid out-of-range
>>> data.
>>>
>>> Suggested-by: Brian Foster <bfoster@redhat.com>
>>> Link: https://lore.kernel.org/linux-fsdevel/anH-WKA1coW6wtfG@bfoster/
>>> Fixes: 7460b470a131 ("mm/truncate: use folio_split() in truncate operation")
>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>
>> The changes mostly look good to me but I have some confusion around the
>> folio2 splitting below.
>>
>>> @@ -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;
>>> +
>>>   		if (!folio_trylock(folio2))
>>> -			goto out;
>>> +			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);
>>> +		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;
>>> +		}
>>
>> So I always found this folio2 lookup and revalidation somewhat suspicious
>> and now that we're digging into it I'll ask: As you note in the changelog,
>> by the time we compute split_at2 the page can be already freed and reused
>> because it was split off from the original 'folio'. It can be for example a
>> slab page or anything else. So is it guaranteed that page_folio() actually
> 
> For a slab or unrelated page, folio->mapping check rejects them.
> 
>> returns something sensible? What guarantees we properly detect the "reuse
> 
> No, but folio_try_get() prevents non ref'd folios.
> 
>> for something else" case in all possible cases for which the page can be
>> reused? I understand this is mostly a preexisting issue so maybe these
>> questions are more for MM guys than you...
> 
> I agree with you that trying to split a unlocked and not ref'd folio2 is
> flaky. And it almost does a __filemap_get_folio() like you proposed
> below.
> 

I think this is indeed a pre-existing issue, as Sashiko pointed out:

 > [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?

the current mapping check on the folio2 and the folio !=
page_folio(split_at) check in __folio_split() are not able to catch this
problem, which could then cause folio2 to be split at the wrong
position. Does that sound right?

>>
>> So for me as an filesystem guy I'd appreciate some comment in this code
>> explaining why grabbing folio2 this way is actually safe. The really safe
>> way of getting to folio2 would be to use
>>    __filemap_get_folio(mapping, (offset+length) >> PAGE_SHIFT, FGP_LOCK, 0)
>> but I suppose we don't use the mapping lookup as it is more expensive?
> 
> __filemap_get_folio() is a better version of the existing folio2 finding
> code. When I wrote the code, I never thought about using
> __filemap_get_folio(), but I probably should have done it.
> 
> BTW,
> __filemap_get_folio(mapping, folio->index + (offset+length) >> PAGE_SHIFT,
> 		FGP_LOCK | FGP_NOWAIT, 0)
> might be better:
> 1. folio->index is needed to get the right folio2,
> 2. nowait can us nonblocking.
> 
> 

Following your suggestion, I reworked my existing patch to use
__filemap_get_folio(), and it ends up looking roughly like the
following(not tested yet). What do you think?

Besides, if the issue Sashiko pointed out does indeed exist, I can
extract the __filemap_get_folio() change into a separate patch to fix it
on its own.

Thanks,
Yi.

diff --git a/mm/truncate.c b/mm/truncate.c
index 23c90f00b530..b5e5bdf5f9ef 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -279,51 +279,46 @@ bool truncate_inode_partial_folio(struct folio 
*folio, loff_t lstart,
  			*pstart = round_up(pos + offset,
  					   min_nrbytes) >> PAGE_SHIFT;

-		if (offset + length == size) {
-			end = aligned_end;
+		end = aligned_end;
+		if (offset + length == size)
  			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.
+		 * 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
+		 * aligned_end and returns it locked and ref'd with the
+		 * mapping validated.
  		 */
-		end = folio->index;
-		if (!folio_try_get(folio2))
+		folio2 = __filemap_get_folio(folio->mapping, aligned_end,
+					     FGP_LOCK | FGP_NOWAIT, 0);
+		if (IS_ERR(folio2)) {
+			/*
+			 * No sub-folio straddles the boundary when aligned_end
+			 * is empty so discarding up to it is safe.  Otherwise
+			 * the straddler is locked by someone else and we
+			 * cannot obtain a reliable end position, so we fall
+			 * back to folio->index.  This is safe but may leave
+			 * pages split off at the offset edge lingering in the
+			 * page cache this round.
+			 */
+			if (PTR_ERR(folio2) != -ENOENT)
+				end = folio->index;
  			goto out;
-
-		if (folio2->mapping != folio->mapping ||
-		    !folio_test_large(folio2))
-			goto out_put;
-
-		if (!folio_trylock(folio2))
-			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;
  		}

-		/* Split failed: back off to the head of the straddler */
+		/* Already at the minimum order, nothing to split */
+		if (folio_order(folio2) == min_order)
+			goto out_put;
+
+		split_at2 = folio_page(folio2, (aligned_end - folio2->index));
+
+		/* Split failed, keep the straddler intact */
  		if (folio_split_or_unmap(folio2, split_at2, min_order))
  			end = folio2->index;
-		else
-			end = aligned_end;

-		folio_unlock(folio2);
  out_put:
+		folio_unlock(folio2);
  		folio_put(folio2);
  out:
  		if (pend)