* [PATCH 0/2] Btrfs: fix partly checksummed file races
@ 2018-05-22 22:02 Omar Sandoval
2018-05-22 22:02 ` [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race Omar Sandoval
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Omar Sandoval @ 2018-05-22 22:02 UTC (permalink / raw)
To: linux-btrfs; +Cc: kernel-team, David Sterba, Timofey Titovets
From: Omar Sandoval <osandov@fb.com>
Clone and dedupe both have checks to make sure that we're not mixing
checksummed and not checksummed extents in a single file. However, both
checks are racy; we need to have the inodes locked or else the flags can
change after we check. The clone check has always been wrong. The dedupe
check was previously correct but is broken in kdave/for-next.
Based on kdave/for-next. Note that there's a Fixes: tag in there
referencing a commit in the for-next branch, so that would have to be
updated if the commit gets rebased. These patches are also available at
https://github.com/osandov/linux/tree/btrfs-nodatasum-race.
Thanks!
Omar Sandoval (2):
Btrfs: fix clone vs chattr NODATASUM race
Btrfs: fix dedupe vs chattr NODATASUM race
fs/btrfs/ioctl.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)
--
2.17.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race 2018-05-22 22:02 [PATCH 0/2] Btrfs: fix partly checksummed file races Omar Sandoval @ 2018-05-22 22:02 ` Omar Sandoval 2018-05-23 6:07 ` Nikolay Borisov 2018-05-23 18:22 ` David Sterba 2018-05-22 22:02 ` [PATCH 2/2] Btrfs: fix dedupe " Omar Sandoval 2018-05-23 10:17 ` [PATCH 0/2] Btrfs: fix partly checksummed file races David Sterba 2 siblings, 2 replies; 9+ messages in thread From: Omar Sandoval @ 2018-05-22 22:02 UTC (permalink / raw) To: linux-btrfs; +Cc: kernel-team, David Sterba, Timofey Titovets From: Omar Sandoval <osandov@fb.com> In btrfs_clone_files(), we must check the NODATASUM flag while the inodes are locked. Otherwise, it's possible that btrfs_ioctl_setflags() will change the flags after we check and we can end up with a party checksummed file. Fixes: 0e7b824c4ef9 ("Btrfs: don't make a file partly checksummed through file clone") Signed-off-by: Omar Sandoval <osandov@fb.com> --- fs/btrfs/ioctl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index cf0d3bc6f625..784e267aad32 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4280,11 +4280,6 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, src->i_sb != inode->i_sb) return -EXDEV; - /* don't make the dst file partly checksummed */ - if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != - (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) - return -EINVAL; - if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode)) return -EISDIR; @@ -4294,6 +4289,13 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, inode_lock(src); } + /* don't make the dst file partly checksummed */ + if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != + (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { + ret = -EINVAL; + goto out_unlock; + } + /* determine range to clone */ ret = -EINVAL; if (off + len > src->i_size || off + len < off) -- 2.17.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race 2018-05-22 22:02 ` [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race Omar Sandoval @ 2018-05-23 6:07 ` Nikolay Borisov 2018-05-23 18:22 ` David Sterba 1 sibling, 0 replies; 9+ messages in thread From: Nikolay Borisov @ 2018-05-23 6:07 UTC (permalink / raw) To: Omar Sandoval, linux-btrfs; +Cc: kernel-team, David Sterba, Timofey Titovets On 23.05.2018 01:02, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > In btrfs_clone_files(), we must check the NODATASUM flag while the > inodes are locked. Otherwise, it's possible that btrfs_ioctl_setflags() > will change the flags after we check and we can end up with a party > checksummed file. > > Fixes: 0e7b824c4ef9 ("Btrfs: don't make a file partly checksummed through file clone") > Signed-off-by: Omar Sandoval <osandov@fb.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/ioctl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index cf0d3bc6f625..784e267aad32 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4280,11 +4280,6 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, > src->i_sb != inode->i_sb) > return -EXDEV; > > - /* don't make the dst file partly checksummed */ > - if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != > - (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) > - return -EINVAL; > - > if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode)) > return -EISDIR; > > @@ -4294,6 +4289,13 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, > inode_lock(src); > } > > + /* don't make the dst file partly checksummed */ > + if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != > + (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { > + ret = -EINVAL; > + goto out_unlock; > + } > + > /* determine range to clone */ > ret = -EINVAL; > if (off + len > src->i_size || off + len < off) > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race 2018-05-22 22:02 ` [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race Omar Sandoval 2018-05-23 6:07 ` Nikolay Borisov @ 2018-05-23 18:22 ` David Sterba 1 sibling, 0 replies; 9+ messages in thread From: David Sterba @ 2018-05-23 18:22 UTC (permalink / raw) To: Omar Sandoval; +Cc: linux-btrfs, kernel-team, David Sterba, Timofey Titovets On Tue, May 22, 2018 at 03:02:12PM -0700, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > In btrfs_clone_files(), we must check the NODATASUM flag while the > inodes are locked. Otherwise, it's possible that btrfs_ioctl_setflags() > will change the flags after we check and we can end up with a party > checksummed file. The race window is only a few instructions in size, between the if and the locks which is: 3834 if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode)) 3835 return -EISDIR; where the setflags must be run and toggle the nodatacow flag (provided the file size is 0). The clone will block on the inode lock, segflags takes the inode lock, changes flags, releases log and clone continues. Not impossible but still needs a lot of bad luck to hit unintentionally. Reviewed-by: David Sterba <dsterba@suse.com> > Fixes: 0e7b824c4ef9 ("Btrfs: don't make a file partly checksummed through file clone") > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > fs/btrfs/ioctl.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index cf0d3bc6f625..784e267aad32 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4280,11 +4280,6 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, > src->i_sb != inode->i_sb) > return -EXDEV; > > - /* don't make the dst file partly checksummed */ > - if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != > - (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) > - return -EINVAL; > - > if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode)) > return -EISDIR; > > @@ -4294,6 +4289,13 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, > inode_lock(src); > } > > + /* don't make the dst file partly checksummed */ > + if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != > + (BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM)) { > + ret = -EINVAL; > + goto out_unlock; > + } > + > /* determine range to clone */ > ret = -EINVAL; > if (off + len > src->i_size || off + len < off) > -- > 2.17.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] Btrfs: fix dedupe vs chattr NODATASUM race 2018-05-22 22:02 [PATCH 0/2] Btrfs: fix partly checksummed file races Omar Sandoval 2018-05-22 22:02 ` [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race Omar Sandoval @ 2018-05-22 22:02 ` Omar Sandoval 2018-05-23 6:10 ` Nikolay Borisov 2018-05-23 10:17 ` [PATCH 0/2] Btrfs: fix partly checksummed file races David Sterba 2 siblings, 1 reply; 9+ messages in thread From: Omar Sandoval @ 2018-05-22 22:02 UTC (permalink / raw) To: linux-btrfs; +Cc: kernel-team, David Sterba, Timofey Titovets From: Omar Sandoval <osandov@fb.com> In btrfs_extent_same(), we must check the NODATASUM flag while the inodes are locked. Otherwise, it's possible that btrfs_ioctl_setflags() will change the flags after we check. This was correct until a recent change. Fixes: 0a38effca37c ("Btrfs: split btrfs_extent_same") Signed-off-by: Omar Sandoval <osandov@fb.com> --- fs/btrfs/ioctl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 784e267aad32..c2263bf4d6f5 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3586,12 +3586,6 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, if (olen == 0) return 0; - /* don't make the dst file partly checksummed */ - if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != - (BTRFS_I(dst)->flags & BTRFS_INODE_NODATASUM)) { - return -EINVAL; - } - tail_len = olen % BTRFS_MAX_DEDUPE_LEN; chunk_count = div_u64(olen, BTRFS_MAX_DEDUPE_LEN); if (chunk_count == 0) @@ -3624,6 +3618,13 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, else btrfs_double_inode_lock(src, dst); + /* don't make the dst file partly checksummed */ + if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != + (BTRFS_I(dst)->flags & BTRFS_INODE_NODATASUM)) { + ret = -EINVAL; + goto out; + } + for (i = 0; i < chunk_count; i++) { ret = btrfs_extent_same_range(src, loff, BTRFS_MAX_DEDUPE_LEN, dst, dst_loff, &cmp); -- 2.17.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Btrfs: fix dedupe vs chattr NODATASUM race 2018-05-22 22:02 ` [PATCH 2/2] Btrfs: fix dedupe " Omar Sandoval @ 2018-05-23 6:10 ` Nikolay Borisov 0 siblings, 0 replies; 9+ messages in thread From: Nikolay Borisov @ 2018-05-23 6:10 UTC (permalink / raw) To: Omar Sandoval, linux-btrfs; +Cc: kernel-team, David Sterba, Timofey Titovets On 23.05.2018 01:02, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > In btrfs_extent_same(), we must check the NODATASUM flag while the > inodes are locked. Otherwise, it's possible that btrfs_ioctl_setflags() > will change the flags after we check. This was correct until a recent > change. > > Fixes: 0a38effca37c ("Btrfs: split btrfs_extent_same") > Signed-off-by: Omar Sandoval <osandov@fb.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/ioctl.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 784e267aad32..c2263bf4d6f5 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3586,12 +3586,6 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, > if (olen == 0) > return 0; > > - /* don't make the dst file partly checksummed */ > - if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != > - (BTRFS_I(dst)->flags & BTRFS_INODE_NODATASUM)) { > - return -EINVAL; > - } > - > tail_len = olen % BTRFS_MAX_DEDUPE_LEN; > chunk_count = div_u64(olen, BTRFS_MAX_DEDUPE_LEN); > if (chunk_count == 0) > @@ -3624,6 +3618,13 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, > else > btrfs_double_inode_lock(src, dst); > > + /* don't make the dst file partly checksummed */ > + if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != > + (BTRFS_I(dst)->flags & BTRFS_INODE_NODATASUM)) { > + ret = -EINVAL; > + goto out; > + } > + > for (i = 0; i < chunk_count; i++) { > ret = btrfs_extent_same_range(src, loff, BTRFS_MAX_DEDUPE_LEN, > dst, dst_loff, &cmp); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Btrfs: fix partly checksummed file races 2018-05-22 22:02 [PATCH 0/2] Btrfs: fix partly checksummed file races Omar Sandoval 2018-05-22 22:02 ` [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race Omar Sandoval 2018-05-22 22:02 ` [PATCH 2/2] Btrfs: fix dedupe " Omar Sandoval @ 2018-05-23 10:17 ` David Sterba 2018-05-23 17:14 ` Omar Sandoval 2 siblings, 1 reply; 9+ messages in thread From: David Sterba @ 2018-05-23 10:17 UTC (permalink / raw) To: Omar Sandoval; +Cc: linux-btrfs, kernel-team, David Sterba, Timofey Titovets On Tue, May 22, 2018 at 03:02:11PM -0700, Omar Sandoval wrote: > Based on kdave/for-next. Note that there's a Fixes: tag in there > referencing a commit in the for-next branch, so that would have to be > updated if the commit gets rebased. These patches are also available at > https://github.com/osandov/linux/tree/btrfs-nodatasum-race. If the original patch is not in any released or frozen branch, then the fix should be folded to the original patch. The for-next branch is for preview, testing and catching bugs that slip past the review. And gets reassembled frequently so referencing a patch from there does not make sense. Sending the fixups as patches is ok, replies to the original thread might get lost in the noise. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Btrfs: fix partly checksummed file races 2018-05-23 10:17 ` [PATCH 0/2] Btrfs: fix partly checksummed file races David Sterba @ 2018-05-23 17:14 ` Omar Sandoval 2018-05-23 18:03 ` David Sterba 0 siblings, 1 reply; 9+ messages in thread From: Omar Sandoval @ 2018-05-23 17:14 UTC (permalink / raw) To: David Sterba; +Cc: linux-btrfs, kernel-team, David Sterba, Timofey Titovets On Wed, May 23, 2018 at 12:17:14PM +0200, David Sterba wrote: > On Tue, May 22, 2018 at 03:02:11PM -0700, Omar Sandoval wrote: > > Based on kdave/for-next. Note that there's a Fixes: tag in there > > referencing a commit in the for-next branch, so that would have to be > > updated if the commit gets rebased. These patches are also available at > > https://github.com/osandov/linux/tree/btrfs-nodatasum-race. > > If the original patch is not in any released or frozen branch, then the > fix should be folded to the original patch. The for-next branch is for > preview, testing and catching bugs that slip past the review. And gets > reassembled frequently so referencing a patch from there does not make > sense. > > Sending the fixups as patches is ok, replies to the original thread > might get lost in the noise. Ok, let's fold it in. I pushed Timofey's series with the fix folded in here: https://github.com/osandov/linux/tree/btrfs-ioctl-fixes, based on misc-next with Timofey's patches removed. The diff vs his original patches is the same as my patch: diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 65d709002775..75c66ac77fd7 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3113,12 +3113,6 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, if (olen == 0) return 0; - /* don't make the dst file partly checksummed */ - if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != - (BTRFS_I(dst)->flags & BTRFS_INODE_NODATASUM)) { - return -EINVAL; - } - tail_len = olen % BTRFS_MAX_DEDUPE_LEN; chunk_count = div_u64(olen, BTRFS_MAX_DEDUPE_LEN); if (chunk_count == 0) @@ -3151,6 +3145,13 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 olen, else btrfs_double_inode_lock(src, dst); + /* don't make the dst file partly checksummed */ + if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) != + (BTRFS_I(dst)->flags & BTRFS_INODE_NODATASUM)) { + ret = -EINVAL; + goto out; + } + for (i = 0; i < chunk_count; i++) { ret = btrfs_extent_same_range(src, loff, BTRFS_MAX_DEDUPE_LEN, dst, dst_loff, &cmp); The clone fix and device remove fix are in that branch, too. Let me know if you'd prefer it as patches. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Btrfs: fix partly checksummed file races 2018-05-23 17:14 ` Omar Sandoval @ 2018-05-23 18:03 ` David Sterba 0 siblings, 0 replies; 9+ messages in thread From: David Sterba @ 2018-05-23 18:03 UTC (permalink / raw) To: Omar Sandoval Cc: David Sterba, linux-btrfs, kernel-team, David Sterba, Timofey Titovets On Wed, May 23, 2018 at 10:14:14AM -0700, Omar Sandoval wrote: > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > The clone fix and device remove fix are in that branch, too. Let me know > if you'd prefer it as patches. The diff like that is fine, there likely will be more conflicts with followup patches that need to be resolved anyway. If you do that and find something worth pointing out then it's good to be mentioned, but otherwise I'm not asking you or other reporter to also post the resolved version. Thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-05-23 18:24 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-22 22:02 [PATCH 0/2] Btrfs: fix partly checksummed file races Omar Sandoval 2018-05-22 22:02 ` [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race Omar Sandoval 2018-05-23 6:07 ` Nikolay Borisov 2018-05-23 18:22 ` David Sterba 2018-05-22 22:02 ` [PATCH 2/2] Btrfs: fix dedupe " Omar Sandoval 2018-05-23 6:10 ` Nikolay Borisov 2018-05-23 10:17 ` [PATCH 0/2] Btrfs: fix partly checksummed file races David Sterba 2018-05-23 17:14 ` Omar Sandoval 2018-05-23 18:03 ` David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.