All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Omar Sandoval <osandov@osandov.com>
Cc: linux-btrfs@vger.kernel.org, kernel-team@fb.com,
	David Sterba <dsterba@suse.com>,
	Timofey Titovets <nefelim4ag@gmail.com>
Subject: Re: [PATCH 1/2] Btrfs: fix clone vs chattr NODATASUM race
Date: Wed, 23 May 2018 20:22:03 +0200	[thread overview]
Message-ID: <20180523182203.GD6649@twin.jikos.cz> (raw)
In-Reply-To: <d8b7c9b717e879eff65bb01e4247696811e975f1.1527026445.git.osandov@fb.com>

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

  parent reply	other threads:[~2018-05-23 18:24 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180523182203.GD6649@twin.jikos.cz \
    --to=dsterba@suse.cz \
    --cc=dsterba@suse.com \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=nefelim4ag@gmail.com \
    --cc=osandov@osandov.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.