Btrfs: fix partly checksummed file races
cover Btrfs: fix partly checksummed file races
From: Omar Sandoval <[email protected]>
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(-)
Signature · 1 line
2.17.0
PATCH 1/2 Btrfs: fix clone vs chattr NODATASUM race
From: Omar Sandoval <[email protected]>
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.
--- fs/btrfs/ioctl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
fs/btrfs/ioctl.c +7 -5
@@ -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)
Signature · 1 line
2.17.0
Re: [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race
Quoted 9 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
In
btrfs_clone_files(), we must check the NODATASUM flag while the inodes are locked. Otherwise, it's possible thatbtrfs_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 <[email protected]>
Quoted 35 lines
--- fs/btrfs/ioctl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)fs/btrfs/ioctl.c +7
-5@@ -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)
Re: [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race
In reply to Omar SandovalQuoted 6 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
In
btrfs_clone_files(), we must check the NODATASUM flag while the inodes are locked. Otherwise, it's possible thatbtrfs_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.
Quoted 43 lines
Fixes 0e7b824c4ef9 ("Btrfs: don't make a file partly checksummed through file clone")Signed-off-by Omar Sandoval <[email protected]>--- fs/btrfs/ioctl.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)fs/btrfs/ioctl.c +7
-5@@ -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)Signature · 6 lines
2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/2 Btrfs: fix dedupe vs chattr NODATASUM race
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
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.
--- fs/btrfs/ioctl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
fs/btrfs/ioctl.c +7 -6
@@ -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);
Signature · 1 line
2.17.0
Re: [PATCH 2/2] Btrfs: fix dedupe vs chattr NODATASUM race
Quoted 9 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
In
btrfs_extent_same(), we must check the NODATASUM flag while the inodes are locked. Otherwise, it's possible thatbtrfs_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 <[email protected]>
Quoted 36 lines
--- fs/btrfs/ioctl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)fs/btrfs/ioctl.c +7
-6@@ -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);
Quoted 4 lines · Omar Sandoval
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.
Quoted 6 lines · David Sterba
Quoted 4 lines · Omar Sandoval
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.
Quoted 8 lines
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:
fs/btrfs/ioctl.c +7 -6
@@ -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.
Quoted 4 lines · Omar Sandoval
fs/btrfs/ioctl.c +0
-0The 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.