Re: [PATCH] btrfs: add missing check for nocow and compression inode flags

[Nikolay Borisov <nborisov@suse.com>]

On 10.07.20 г. 13:05 ч., David Sterba wrote:
> User Forza reported on IRC that some invalid combinations of file
> attributes are accepted by chattr.
> 
> The NODATACOW and compression file flags/attributes are mutually
> exclusive, but they could be set by 'chattr +c +C' on an empty file. The
> nodatacow will be in effect because it's checked first in
> btrfs_run_delalloc_range.
> 
> Extend the flag validation to catch the following cases:
> 
>   - input flags are conflicting
>   - old and new flags are conflicting
>   - initialize the local variable with inode flags after inode ls locked
> 
> CC: stable@vger.kernel.org # 4.4+
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ioctl.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 3a566cf71fc6..0c13bb38425b 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -164,8 +164,11 @@ static int btrfs_ioctl_getflags(struct file *file, void __user *arg)
>  	return 0;
>  }
>  
> -/* Check if @flags are a supported and valid set of FS_*_FL flags */
> -static int check_fsflags(unsigned int flags)
> +/*
> + * Check if @flags are a supported and valid set of FS_*_FL flags and that
> + * the old and new flags are not conflicting
> + */
> +static int check_fsflags(unsigned int old_flags, unsigned int flags)
>  {
>  	if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | \
>  		      FS_NOATIME_FL | FS_NODUMP_FL | \
> @@ -174,9 +177,19 @@ static int check_fsflags(unsigned int flags)
>  		      FS_NOCOW_FL))
>  		return -EOPNOTSUPP;
>  
> +	/* COMPR and NOCOMP on new/old are valid */
>  	if ((flags & FS_NOCOMP_FL) && (flags & FS_COMPR_FL))
>  		return -EINVAL;
>  
> +	if ((flags & FS_COMPR_FL) && (flags & FS_NOCOW_FL))
> +		return -EINVAL;
> +
> +	/* NOCOW and compression options are mutually exclusive */
> +	if ((old_flags & FS_NOCOW_FL) && (flags & (FS_COMPR_FL | FS_NOCOMP_FL)))

Why is NOCOW and setting NOCOMP (which would really be a NOOP) an
invalid combination?

> +		return -EINVAL;
> +	if ((flags & FS_NOCOW_FL) && (old_flags & (FS_COMPR_FL | FS_NOCOMP_FL)))
> +		return -EINVAL;

Same thing here, just inverted?

> +
>  	return 0;
>  }
>  
> @@ -190,7 +203,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	unsigned int fsflags, old_fsflags;
>  	int ret;
>  	const char *comp = NULL;
> -	u32 binode_flags = binode->flags;
> +	u32 binode_flags;
>  
>  	if (!inode_owner_or_capable(inode))
>  		return -EPERM;
> @@ -201,22 +214,23 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	if (copy_from_user(&fsflags, arg, sizeof(fsflags)))
>  		return -EFAULT;
>  
> -	ret = check_fsflags(fsflags);
> -	if (ret)
> -		return ret;
> -
>  	ret = mnt_want_write_file(file);
>  	if (ret)
>  		return ret;
>  
>  	inode_lock(inode);
> -
>  	fsflags = btrfs_mask_fsflags_for_type(inode, fsflags);
>  	old_fsflags = btrfs_inode_flags_to_fsflags(binode->flags);
> +
>  	ret = vfs_ioc_setflags_prepare(inode, old_fsflags, fsflags);
>  	if (ret)
>  		goto out_unlock;
>  
> +	ret = check_fsflags(old_fsflags, fsflags);
> +	if (ret)
> +		goto out_unlock;
> +
> +	binode_flags = binode->flags;
>  	if (fsflags & FS_SYNC_FL)
>  		binode_flags |= BTRFS_INODE_SYNC;
>  	else
>