diff mbox series

btrfs: add missing check for nocow and compression inode flags

Message ID 20200710100553.13567-1-dsterba@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: add missing check for nocow and compression inode flags | expand

Commit Message

David Sterba July 10, 2020, 10:05 a.m. UTC 
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(-)

Comments

Nikolay Borisov July 10, 2020, 10:10 a.m. UTC | #1 
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
>
Johannes Thumshirn July 10, 2020, 10:17 a.m. UTC | #2 
On 10/07/2020 12:06, David Sterba wrote:
> +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)))
> +		return -EINVAL;
> +	if ((flags & FS_NOCOW_FL) && (old_flags & (FS_COMPR_FL | FS_NOCOMP_FL)))
> +		return -EINVAL;
> +


If we'd pass in fs_info to check_fsflags() we could also validate against mount options
which are incompatible with inode flags. Like -o nodatacow and FS_COMPR_FL or 
-o auth_key and FS_NOCOW_FL.
David Sterba July 10, 2020, 1:28 p.m. UTC | #3 
On Fri, Jul 10, 2020 at 10:17:55AM +0000, Johannes Thumshirn wrote:
> On 10/07/2020 12:06, David Sterba wrote:
> > +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)))
> > +		return -EINVAL;
> > +	if ((flags & FS_NOCOW_FL) && (old_flags & (FS_COMPR_FL | FS_NOCOMP_FL)))
> > +		return -EINVAL;
> > +
> 
> 
> If we'd pass in fs_info to check_fsflags() we could also validate against mount options
> which are incompatible with inode flags. Like -o nodatacow and FS_COMPR_FL or 
> -o auth_key and FS_NOCOW_FL.

Same question was asked on IRC too, mount options are independent and
take lower precedence than the inode attributes. A scenario where user
wants to set a nodatacow attribute when the filesystem is mounted with
compress= is valid and can be quite common.
David Sterba July 10, 2020, 1:34 p.m. UTC | #4 
On Fri, Jul 10, 2020 at 01:10:25PM +0300, Nikolay Borisov wrote:
> > +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?

The options are not conflicting directly, like for the compression and
nodatacow, but it still is related to compression so it does not feel
right to allow that even if it's a noop.
Nikolay Borisov July 10, 2020, 2:55 p.m. UTC | #5 
On 10.07.20 г. 16:34 ч., David Sterba wrote:
> On Fri, Jul 10, 2020 at 01:10:25PM +0300, Nikolay Borisov wrote:
>>> +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?
> 
> The options are not conflicting directly, like for the compression and
> nodatacow, but it still is related to compression so it does not feel
> right to allow that even if it's a noop.
> 

Please put this reasoning in the changelog.
Sasha Levin July 16, 2020, 12:27 a.m. UTC | #6 
Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: 4.4+

The bot has tested the following trees: v5.7.8, v5.4.51, v4.19.132, v4.14.188, v4.9.230, v4.4.230.

v5.7.8: Build OK!
v5.4.51: Build OK!
v4.19.132: Failed to apply! Possible dependencies:
    04e6863b19c72 ("btrfs: split btrfs_setxattr calls regarding transaction")
    262c96a3c3670 ("btrfs: refactor btrfs_set_prop and add btrfs_set_prop_trans")
    7715da84f74d5 ("btrfs: merge _btrfs_set_prop helpers")
    8b4d1efc9e6c3 ("btrfs: prop: open code btrfs_set_prop in inherit_prop")
    cac237ae095f6 ("btrfs: rename btrfs_setxattr to btrfs_setxattr_trans")
    d2b8fcfe43155 ("btrfs: modify local copy of btrfs_inode flags")
    f22125e5d8ae1 ("btrfs: refactor btrfs_set_props to validate externally")
    ff9fef559babe ("btrfs: start transaction in btrfs_ioctl_setflags()")

v4.14.188: Failed to apply! Possible dependencies:
    04e6863b19c72 ("btrfs: split btrfs_setxattr calls regarding transaction")
    1905a0f7c7de3 ("btrfs: rename btrfs_mask_flags to reflect which flags it touches")
    262c96a3c3670 ("btrfs: refactor btrfs_set_prop and add btrfs_set_prop_trans")
    38e82de8ccd18 ("btrfs: user proper type for btrfs_mask_flags flags")
    5ba76abfb2336 ("btrfs: rename check_flags to reflect which flags it touches")
    5c57b8b6a4966 ("btrfs: unify naming of flags variables for SETFLAGS and XFLAGS")
    7715da84f74d5 ("btrfs: merge _btrfs_set_prop helpers")
    7852781d94b30 ("btrfs: drop underscores from exported xattr functions")
    7b6a221e5b21f ("btrfs: rename btrfs_update_iflags to reflect which flags it touches")
    8b4d1efc9e6c3 ("btrfs: prop: open code btrfs_set_prop in inherit_prop")
    93370509c24cc ("btrfs: SETFLAGS ioctl: use helper for compression type conversion")
    a157d4fd81dc7 ("btrfs: rename btrfs_flags_to_ioctl to reflect which flags it touches")
    ab0d09361662b ("btrfs: drop extern from function declarations")
    cac237ae095f6 ("btrfs: rename btrfs_setxattr to btrfs_setxattr_trans")
    d2b8fcfe43155 ("btrfs: modify local copy of btrfs_inode flags")
    f22125e5d8ae1 ("btrfs: refactor btrfs_set_props to validate externally")
    ff9fef559babe ("btrfs: start transaction in btrfs_ioctl_setflags()")

v4.9.230: Failed to apply! Possible dependencies:
    0b246afa62b0c ("btrfs: root->fs_info cleanup, add fs_info convenience variables")
    1905a0f7c7de3 ("btrfs: rename btrfs_mask_flags to reflect which flags it touches")
    38e82de8ccd18 ("btrfs: user proper type for btrfs_mask_flags flags")
    5ba76abfb2336 ("btrfs: rename check_flags to reflect which flags it touches")
    5c57b8b6a4966 ("btrfs: unify naming of flags variables for SETFLAGS and XFLAGS")
    62d1f9fe97dd2 ("btrfs: remove trivial helper btrfs_find_tree_block")
    a157d4fd81dc7 ("btrfs: rename btrfs_flags_to_ioctl to reflect which flags it touches")
    cf8cddd38bab3 ("btrfs: don't abuse REQ_OP_* flags for btrfs_map_block")
    da17066c40472 ("btrfs: pull node/sector/stripe sizes out of root and into fs_info")
    de143792253e2 ("btrfs: struct btrfsic_state->root should be an fs_info")
    fb456252d3d9c ("btrfs: root->fs_info cleanup, use fs_info->dev_root everywhere")
    ff9fef559babe ("btrfs: start transaction in btrfs_ioctl_setflags()")

v4.4.230: Failed to apply! Possible dependencies:
    0132761017e01 ("btrfs: fix string and comment grammatical issues and typos")
    09cbfeaf1a5a6 ("mm, fs: get rid of PAGE_CACHE_* and page_cache_{get,release} macros")
    0b246afa62b0c ("btrfs: root->fs_info cleanup, add fs_info convenience variables")
    0e749e54244ee ("dax: increase granularity of dax_clear_blocks() operations")
    1905a0f7c7de3 ("btrfs: rename btrfs_mask_flags to reflect which flags it touches")
    38e82de8ccd18 ("btrfs: user proper type for btrfs_mask_flags flags")
    4420cfd3f51cf ("staging: lustre: format properly all comment blocks for LNet core")
    52db400fcd502 ("pmem, dax: clean up clear_pmem()")
    5ba76abfb2336 ("btrfs: rename check_flags to reflect which flags it touches")
    5c57b8b6a4966 ("btrfs: unify naming of flags variables for SETFLAGS and XFLAGS")
    5fd88337d209d ("staging: lustre: fix all conditional comparison to zero in LNet layer")
    a157d4fd81dc7 ("btrfs: rename btrfs_flags_to_ioctl to reflect which flags it touches")
    b2e0d1625e193 ("dax: fix lifetime of in-kernel dax mappings with dax_map_atomic()")
    bb7ab3b92e46d ("btrfs: Fix misspellings in comments.")
    cf8cddd38bab3 ("btrfs: don't abuse REQ_OP_* flags for btrfs_map_block")
    d1a5f2b4d8a12 ("block: use DAX for partition table reads")
    de143792253e2 ("btrfs: struct btrfsic_state->root should be an fs_info")
    e10624f8c0971 ("pmem: fail io-requests to known bad blocks")
    ff9fef559babe ("btrfs: start transaction in btrfs_ioctl_setflags()")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
diff mbox series

Patch

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)))
+		return -EINVAL;
+	if ((flags & FS_NOCOW_FL) && (old_flags & (FS_COMPR_FL | FS_NOCOMP_FL)))
+		return -EINVAL;
+
 	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