[PATCH 0/2] Btrfs: fix partly checksummed file races

[PATCH 0/2] Btrfs: fix partly checksummed file races

[Omar Sandoval]

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

[PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race

[Omar Sandoval]

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

[PATCH 2/2] Btrfs: fix dedupe vs chattr NODATASUM race

[Omar Sandoval]

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

Re: [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race

[Nikolay Borisov]

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)
> 

Re: [PATCH 2/2] Btrfs: fix dedupe vs chattr NODATASUM race

[Nikolay Borisov]

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);
> 

Re: [PATCH 0/2] Btrfs: fix partly checksummed file races

[David Sterba]

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.

Re: [PATCH 0/2] Btrfs: fix partly checksummed file races

[Omar Sandoval]

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.

Re: [PATCH 0/2] Btrfs: fix partly checksummed file races

[David Sterba]

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.

Re: [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race

[David Sterba]

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