[PATCH STABLE V2] xfs: trim IO to found COW extent limit

[PATCH STABLE V2] xfs: trim IO to found COW extent limit

[Eric Sandeen]

A bug existed in the XFS reflink code between v5.1 and v5.5 in which
the mapping for a COW IO was not trimmed to the mapping of the COW
extent that was found.  This resulted in a too-short copy, and
corruption of other files which shared the original extent.

(This happened only when extent size hints were set, which bypasses
delalloc and led to this code path.)

This was (inadvertently) fixed upstream with

36adcbace24e "xfs: fill out the srcmap in iomap_begin"

and related patches which moved lots of this functionality to
the iomap subsystem.

Hence, this is a -stable only patch, targeted to fix this
corruption vector without other major code changes.

Fixes: 78f0cc9d55cb ("xfs: don't use delalloc extents for COW on files with extsize hints")
Cc: <stable@vger.kernel.org> # 5.4.x
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---

V2: Fix typo in subject, add reviewers

I've tested this with a targeted reproducer (in next email) as well as
with xfstests.

There is also now a testcase for xfstests submitted upstream

Stable folk, not sure how to send a "stable only" patch, or if that's even
valid.  Assuming you're willing to accept it, I would still like to have
some formal Reviewed-by's from the xfs developer community before it gets
merged.

Big thanks to Darrick & Dave for letting me whine about this bug and
offering suggestions for testing and ultimately, a patch to test.

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 06b9e0aacf54..3289d0f4bb03 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1002,9 +1002,15 @@ xfs_file_iomap_begin(
 		 * I/O, which must be block aligned, we need to report the
 		 * newly allocated address.  If the data fork has a hole, copy
 		 * the COW fork mapping to avoid allocating to the data fork.
+		 *
+		 * Otherwise, ensure that the imap range does not extend past
+		 * the range allocated/found in cmap.
 		 */
 		if (directio || imap.br_startblock == HOLESTARTBLOCK)
 			imap = cmap;
+		else
+			xfs_trim_extent(&imap, cmap.br_startoff,
+					cmap.br_blockcount);
 
 		end_fsb = imap.br_startoff + imap.br_blockcount;
 		length = XFS_FSB_TO_B(mp, end_fsb) - offset;

Re: [PATCH STABLE V2] xfs: trim IO to found COW extent limit

[Sasha Levin]

On Thu, Oct 01, 2020 at 08:34:48AM -0500, Eric Sandeen wrote:
>A bug existed in the XFS reflink code between v5.1 and v5.5 in which
>the mapping for a COW IO was not trimmed to the mapping of the COW
>extent that was found.  This resulted in a too-short copy, and
>corruption of other files which shared the original extent.
>
>(This happened only when extent size hints were set, which bypasses
>delalloc and led to this code path.)
>
>This was (inadvertently) fixed upstream with
>
>36adcbace24e "xfs: fill out the srcmap in iomap_begin"
>
>and related patches which moved lots of this functionality to
>the iomap subsystem.
>
>Hence, this is a -stable only patch, targeted to fix this
>corruption vector without other major code changes.
>
>Fixes: 78f0cc9d55cb ("xfs: don't use delalloc extents for COW on files with extsize hints")
>Cc: <stable@vger.kernel.org> # 5.4.x
>Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>Reviewed-by: Christoph Hellwig <hch@lst.de>
>---
>
>V2: Fix typo in subject, add reviewers
>
>I've tested this with a targeted reproducer (in next email) as well as
>with xfstests.
>
>There is also now a testcase for xfstests submitted upstream
>
>Stable folk, not sure how to send a "stable only" patch, or if that's even
>valid.  Assuming you're willing to accept it, I would still like to have
>some formal Reviewed-by's from the xfs developer community before it gets
>merged.

This is perfect stable-process-wise :) Will wait for reviews/acks before
merging.

-- 
Thanks,
Sasha

Re: [PATCH STABLE V2] xfs: trim IO to found COW extent limit

[Eric Sandeen]

On 10/2/20 8:07 AM, Sasha Levin wrote:
> On Thu, Oct 01, 2020 at 08:34:48AM -0500, Eric Sandeen wrote:
>> A bug existed in the XFS reflink code between v5.1 and v5.5 in which
>> the mapping for a COW IO was not trimmed to the mapping of the COW
>> extent that was found.  This resulted in a too-short copy, and
>> corruption of other files which shared the original extent.
>>
>> (This happened only when extent size hints were set, which bypasses
>> delalloc and led to this code path.)
>>
>> This was (inadvertently) fixed upstream with
>>
>> 36adcbace24e "xfs: fill out the srcmap in iomap_begin"
>>
>> and related patches which moved lots of this functionality to
>> the iomap subsystem.
>>
>> Hence, this is a -stable only patch, targeted to fix this
>> corruption vector without other major code changes.
>>
>> Fixes: 78f0cc9d55cb ("xfs: don't use delalloc extents for COW on files with extsize hints")
>> Cc: <stable@vger.kernel.org> # 5.4.x
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> ---
>>
>> V2: Fix typo in subject, add reviewers
>>
>> I've tested this with a targeted reproducer (in next email) as well as
>> with xfstests.
>>
>> There is also now a testcase for xfstests submitted upstream
>>
>> Stable folk, not sure how to send a "stable only" patch, or if that's even
>> valid.  Assuming you're willing to accept it, I would still like to have
>> some formal Reviewed-by's from the xfs developer community before it gets
>> merged.
> 
> This is perfect stable-process-wise :) Will wait for reviews/acks before
> merging.

Thansk Sasha - the reviews/acks were given for V1 (hch & darrick), V2 adds them to the
commit log (see above) and fixes a typo in the subject.

Thanks,
-Eric

Re: [PATCH STABLE V2] xfs: trim IO to found COW extent limit

[Darrick J. Wong]

On Fri, Oct 02, 2020 at 08:19:43AM -0500, Eric Sandeen wrote:
> On 10/2/20 8:07 AM, Sasha Levin wrote:
> > On Thu, Oct 01, 2020 at 08:34:48AM -0500, Eric Sandeen wrote:
> >> A bug existed in the XFS reflink code between v5.1 and v5.5 in which
> >> the mapping for a COW IO was not trimmed to the mapping of the COW
> >> extent that was found.  This resulted in a too-short copy, and
> >> corruption of other files which shared the original extent.
> >>
> >> (This happened only when extent size hints were set, which bypasses
> >> delalloc and led to this code path.)
> >>
> >> This was (inadvertently) fixed upstream with
> >>
> >> 36adcbace24e "xfs: fill out the srcmap in iomap_begin"
> >>
> >> and related patches which moved lots of this functionality to
> >> the iomap subsystem.
> >>
> >> Hence, this is a -stable only patch, targeted to fix this
> >> corruption vector without other major code changes.
> >>
> >> Fixes: 78f0cc9d55cb ("xfs: don't use delalloc extents for COW on files with extsize hints")
> >> Cc: <stable@vger.kernel.org> # 5.4.x
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >> ---
> >>
> >> V2: Fix typo in subject, add reviewers
> >>
> >> I've tested this with a targeted reproducer (in next email) as well as
> >> with xfstests.
> >>
> >> There is also now a testcase for xfstests submitted upstream
> >>
> >> Stable folk, not sure how to send a "stable only" patch, or if that's even
> >> valid.  Assuming you're willing to accept it, I would still like to have
> >> some formal Reviewed-by's from the xfs developer community before it gets
> >> merged.
> > 
> > This is perfect stable-process-wise :) Will wait for reviews/acks before
> > merging.
> 
> Thansk Sasha - the reviews/acks were given for V1 (hch & darrick), V2
> adds them to the commit log (see above) and fixes a typo in the
> subject.

Still looks ok ;)

Still-Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Thanks,
> -Eric
> 

Re: [PATCH STABLE V2] xfs: trim IO to found COW extent limit

[Sasha Levin]

On Fri, Oct 02, 2020 at 08:19:43AM -0500, Eric Sandeen wrote:
>On 10/2/20 8:07 AM, Sasha Levin wrote:
>> On Thu, Oct 01, 2020 at 08:34:48AM -0500, Eric Sandeen wrote:
>>> A bug existed in the XFS reflink code between v5.1 and v5.5 in which
>>> the mapping for a COW IO was not trimmed to the mapping of the COW
>>> extent that was found.  This resulted in a too-short copy, and
>>> corruption of other files which shared the original extent.
>>>
>>> (This happened only when extent size hints were set, which bypasses
>>> delalloc and led to this code path.)
>>>
>>> This was (inadvertently) fixed upstream with
>>>
>>> 36adcbace24e "xfs: fill out the srcmap in iomap_begin"
>>>
>>> and related patches which moved lots of this functionality to
>>> the iomap subsystem.
>>>
>>> Hence, this is a -stable only patch, targeted to fix this
>>> corruption vector without other major code changes.
>>>
>>> Fixes: 78f0cc9d55cb ("xfs: don't use delalloc extents for COW on files with extsize hints")
>>> Cc: <stable@vger.kernel.org> # 5.4.x
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>
>>> V2: Fix typo in subject, add reviewers
>>>
>>> I've tested this with a targeted reproducer (in next email) as well as
>>> with xfstests.
>>>
>>> There is also now a testcase for xfstests submitted upstream
>>>
>>> Stable folk, not sure how to send a "stable only" patch, or if that's even
>>> valid.  Assuming you're willing to accept it, I would still like to have
>>> some formal Reviewed-by's from the xfs developer community before it gets
>>> merged.
>>
>> This is perfect stable-process-wise :) Will wait for reviews/acks before
>> merging.
>
>Thansk Sasha - the reviews/acks were given for V1 (hch & darrick), V2 adds them to the
>commit log (see above) and fixes a typo in the subject.

Ah, I see. Queued up!

-- 
Thanks,
Sasha