[PATCH 0/3] btrfs: qgroup: Deprecate unused features for btrfs_qgroup_inherit()

[PATCH 0/3] btrfs: qgroup: Deprecate unused features for btrfs_qgroup_inherit()

[Qu Wenruo]

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/qgroup_inherit_check
Which is based on v4.19-rc1 tag.

This patchset will first set btrfs_qgroup_inherit structure size limit
from PAGE_SIZE to fixed SZ_4K.
I understand this normally will cause compatibility problem, but
considering how minor this feature is and no sane guy should use it for
over 100 qgroups, it should be fine in real world.

The 2nd patch introduce check function for btrfs_qgroup_inherit
structure and deprecates the following features:
1) limit set
   Never utilized by btrfs-progs from the beginning.

2) copy rfer/excl
   Although btrfs-progs provides support for it as a hidden,
   undocumented feature, it's the easiest way to screw up qgroup
   numbers.
   And we already have patches wondering around the ML to remove such
   support.

The last one will just cleanup the code for unsupported features.

Qu Wenruo (3):
  btrfs: Set qgroup inherit size limit to SZ_4K instead of page size
  btrfs: qgroup: Validate btrfs_qgroup_inherit structure before passing
    it to qgroup
  btrfs: qgroup: Remove deprecated feature support in
    btrfs_qgorup_inhert()

 fs/btrfs/ioctl.c           |  5 +-
 fs/btrfs/qgroup.c          | 96 ++++++++++++++++----------------------
 fs/btrfs/qgroup.h          |  2 +
 include/uapi/linux/btrfs.h | 18 +++----
 4 files changed, 55 insertions(+), 66 deletions(-)

-- 
2.18.0

[PATCH 1/3] btrfs: Set qgroup inherit size limit to SZ_4K instead of page size

[Qu Wenruo]

Change btrfs_qgroup_inherit maximum size from PAGE_SIZE to SZ_4K to make
it consistent across different architectures.

Although in theory this could lead to incompatibility, but considering
how rare btrfs_qgroup_inherit is used, it's still not too late to change
it without impacting a large user base.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ioctl.c           | 2 +-
 include/uapi/linux/btrfs.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 63600dc2ac4c..5db8680b40a9 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1811,7 +1811,7 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 	if (vol_args->flags & BTRFS_SUBVOL_RDONLY)
 		readonly = true;
 	if (vol_args->flags & BTRFS_SUBVOL_QGROUP_INHERIT) {
-		if (vol_args->size > PAGE_SIZE) {
+		if (vol_args->size > BTRFS_QGROUP_INHERIT_MAX_SIZE) {
 			ret = -EINVAL;
 			goto free_args;
 		}
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 5ca1d21fc4a7..311edb65567c 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -82,6 +82,7 @@ struct btrfs_qgroup_limit {
  */
 #define BTRFS_QGROUP_INHERIT_SET_LIMITS	(1ULL << 0)
 
+#define BTRFS_QGROUP_INHERIT_MAX_SIZE	(SZ_4K)
 struct btrfs_qgroup_inherit {
 	__u64	flags;
 	__u64	num_qgroups;
-- 
2.18.0

[PATCH 2/3] btrfs: qgroup: Validate btrfs_qgroup_inherit structure before passing it to qgroup

[Qu Wenruo]

btrfs_qgroup_inherit structure doesn't goes through much validation
check.

Now do a comprehensive check for it, including:
1) inherit size
   Should not exceeding SZ_4K and its num_qgroups should not
   exceed its size passed in btrfs_ioctl_vol_args_v2.

2) flags
   Should not include any unknown flags
   (In fact, no flag is supported at all now)
   Btrfs-progs never has such ability to set flags for btrfs_qgroup_inherit.

3) limit
   Should not contain anything.
   Btrfs-progs never has such ability to set limit for btrfs_qgroup_inherit.

4) rfer/excl copy
   Deprecated feature.
   Btrfs-progs has such interface but never documented and we're already
   going to remove such ability.
   It's the easiest way to screw up qgroup numbers.

3) Qgroupid
   Comprehensive check is already in btrfs_qgroup_inherit(), here we
   only check if there is any obviously invalid qgroupid (0).

Coverity-id: 1021055
Reported-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ioctl.c           |  3 +++
 fs/btrfs/qgroup.c          | 39 ++++++++++++++++++++++++++++++++++++++
 fs/btrfs/qgroup.h          |  2 ++
 include/uapi/linux/btrfs.h | 17 ++++++++---------
 4 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5db8680b40a9..4f5f453d5d07 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1820,6 +1820,9 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
 			ret = PTR_ERR(inherit);
 			goto free_args;
 		}
+		ret = btrfs_validate_inherit(inherit, vol_args->size);
+		if (ret < 0)
+			goto free_args;
 	}
 
 	ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 4353bb69bb86..53daf73b0de9 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2232,6 +2232,45 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans)
 	return ret;
 }
 
+/*
+ * To make sure the inherit passed in is valid
+ *
+ * Here we only check flags and rule out some no-longer supported features.
+ * And we only do very basis qgroupid check to ensure there is no obviously
+ * invalid qgroupid (0). Detailed qgroupid check will be done in
+ * btrfs_qgroup_inherit().
+ */
+int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
+			   u64 inherit_size)
+{
+	u64 i;
+
+	if (inherit->flags & ~BTRFS_QGROUP_INHERIT_FLAGS_SUPP)
+		return -ENOTTY;
+	/* Qgroup rfer/excl copy is deprecated */
+	if (inherit->num_excl_copies || inherit->num_ref_copies)
+		return -ENOTTY;
+
+	/* Since SET_LIMITS is never used, @lim should all be zeroed */
+	if (inherit->lim.max_excl || inherit->lim.max_rfer ||
+	    inherit->lim.rsv_excl || inherit->lim.rsv_rfer ||
+	    inherit->lim.flags)
+		return -ENOTTY;
+
+	/* Size check */
+	if (sizeof(u64) * inherit->num_qgroups + sizeof(*inherit) >
+	    min_t(u64, BTRFS_QGROUP_INHERIT_MAX_SIZE, inherit_size))
+		return -EINVAL;
+
+
+	/* Qgroup 0/0 is not allowed */
+	for (i = 0; i < inherit->num_qgroups; i++) {
+		if (inherit->qgroups[i] == 0)
+			return -EINVAL;
+	}
+	return 0;
+}
+
 /*
  * Copy the accounting information between qgroups. This is necessary
  * when a snapshot or a subvolume is created. Throwing an error will
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 54b8bb282c0e..1bf9c584be70 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -241,6 +241,8 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 				struct ulist *new_roots);
 int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans);
 int btrfs_run_qgroups(struct btrfs_trans_handle *trans);
+int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
+			   u64 inherit_size);
 int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 			 u64 objectid, struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 311edb65567c..5a5532a20019 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -74,21 +74,20 @@ struct btrfs_qgroup_limit {
 	__u64	rsv_excl;
 };
 
-/*
- * flags definition for qgroup inheritance
- *
- * Used by:
- * struct btrfs_qgroup_inherit.flags
- */
+/* flags definition for qgroup inheritance (DEPRECATED) */
 #define BTRFS_QGROUP_INHERIT_SET_LIMITS	(1ULL << 0)
 
+/* No supported flags */
+#define BTRFS_QGROUP_INHERIT_FLAGS_SUPP (0)
+
 #define BTRFS_QGROUP_INHERIT_MAX_SIZE	(SZ_4K)
+
 struct btrfs_qgroup_inherit {
 	__u64	flags;
 	__u64	num_qgroups;
-	__u64	num_ref_copies;
-	__u64	num_excl_copies;
-	struct btrfs_qgroup_limit lim;
+	__u64	num_ref_copies;		/* DEPRECATED */
+	__u64	num_excl_copies;	/* DEPRECATED */
+	struct btrfs_qgroup_limit lim;	/* DEPRECATED */
 	__u64	qgroups[0];
 };
 
-- 
2.18.0

[PATCH 3/3] btrfs: qgroup: Remove deprecated feature support in btrfs_qgorup_inhert()

[Qu Wenruo]

Since btrfs_validate_inherit() will not allow features like copy
rfer/excl and limit set, remove these dead code.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 57 +----------------------------------------------
 1 file changed, 1 insertion(+), 56 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 53daf73b0de9..b57577f45ff3 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2301,8 +2301,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 
 	if (inherit) {
 		i_qgroups = (u64 *)(inherit + 1);
-		nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
-		       2 * inherit->num_excl_copies;
+		nums = inherit->num_qgroups;
 		for (i = 0; i < nums; ++i) {
 			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
 
@@ -2354,23 +2353,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 		goto unlock;
 	}
 
-	if (inherit && inherit->flags & BTRFS_QGROUP_INHERIT_SET_LIMITS) {
-		dstgroup->lim_flags = inherit->lim.flags;
-		dstgroup->max_rfer = inherit->lim.max_rfer;
-		dstgroup->max_excl = inherit->lim.max_excl;
-		dstgroup->rsv_rfer = inherit->lim.rsv_rfer;
-		dstgroup->rsv_excl = inherit->lim.rsv_excl;
-
-		ret = update_qgroup_limit_item(trans, dstgroup);
-		if (ret) {
-			fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
-			btrfs_info(fs_info,
-				   "unable to update quota limit for %llu",
-				   dstgroup->qgroupid);
-			goto unlock;
-		}
-	}
-
 	if (srcid) {
 		srcgroup = find_qgroup_rb(fs_info, srcid);
 		if (!srcgroup)
@@ -2413,43 +2395,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
 		++i_qgroups;
 	}
 
-	for (i = 0; i <  inherit->num_ref_copies; ++i, i_qgroups += 2) {
-		struct btrfs_qgroup *src;
-		struct btrfs_qgroup *dst;
-
-		if (!i_qgroups[0] || !i_qgroups[1])
-			continue;
-
-		src = find_qgroup_rb(fs_info, i_qgroups[0]);
-		dst = find_qgroup_rb(fs_info, i_qgroups[1]);
-
-		if (!src || !dst) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-
-		dst->rfer = src->rfer - level_size;
-		dst->rfer_cmpr = src->rfer_cmpr - level_size;
-	}
-	for (i = 0; i <  inherit->num_excl_copies; ++i, i_qgroups += 2) {
-		struct btrfs_qgroup *src;
-		struct btrfs_qgroup *dst;
-
-		if (!i_qgroups[0] || !i_qgroups[1])
-			continue;
-
-		src = find_qgroup_rb(fs_info, i_qgroups[0]);
-		dst = find_qgroup_rb(fs_info, i_qgroups[1]);
-
-		if (!src || !dst) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-
-		dst->excl = src->excl + level_size;
-		dst->excl_cmpr = src->excl_cmpr + level_size;
-	}
-
 unlock:
 	spin_unlock(&fs_info->qgroup_lock);
 out:
-- 
2.18.0

Re: [PATCH 0/3] btrfs: qgroup: Deprecate unused features for btrfs_qgroup_inherit()

[David Sterba]

On Fri, Aug 31, 2018 at 10:29:27AM +0800, Qu Wenruo wrote:
> This patchset can be fetched from github:
> https://github.com/adam900710/linux/tree/qgroup_inherit_check
> Which is based on v4.19-rc1 tag.
> 
> This patchset will first set btrfs_qgroup_inherit structure size limit
> from PAGE_SIZE to fixed SZ_4K.
> I understand this normally will cause compatibility problem, but
> considering how minor this feature is and no sane guy should use it for
> over 100 qgroups, it should be fine in real world.

Agreed, please update the changelog of 1st patch with description on
what will stop working and under what conditions. The 4k limit sounds
good enough, the real difference would be on architectures with larger
page sizes where the feature would be used.

> The 2nd patch introduce check function for btrfs_qgroup_inherit
> structure and deprecates the following features:
> 1) limit set
>    Never utilized by btrfs-progs from the beginning.
> 
> 2) copy rfer/excl
>    Although btrfs-progs provides support for it as a hidden,
>    undocumented feature, it's the easiest way to screw up qgroup
>    numbers.
>    And we already have patches wondering around the ML to remove such
>    support.

The deprecation should be done in a few steps. First issue a warning
that the feature is deprecated and will be removed in release X. Then
wait until somebody complains (or not) and remove the code in release X.

The X is something like 4.22, ie. at least 2 cycles after the
deprecation warning is added.

Re: [PATCH 0/3] btrfs: qgroup: Deprecate unused features for btrfs_qgroup_inherit()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1867 bytes --]



On 2018/9/5 下午9:00, David Sterba wrote:
> On Fri, Aug 31, 2018 at 10:29:27AM +0800, Qu Wenruo wrote:
>> This patchset can be fetched from github:
>> https://github.com/adam900710/linux/tree/qgroup_inherit_check
>> Which is based on v4.19-rc1 tag.
>>
>> This patchset will first set btrfs_qgroup_inherit structure size limit
>> from PAGE_SIZE to fixed SZ_4K.
>> I understand this normally will cause compatibility problem, but
>> considering how minor this feature is and no sane guy should use it for
>> over 100 qgroups, it should be fine in real world.
> 
> Agreed, please update the changelog of 1st patch with description on
> what will stop working and under what conditions. The 4k limit sounds
> good enough, the real difference would be on architectures with larger
> page sizes where the feature would be used.

No problem.

> 
>> The 2nd patch introduce check function for btrfs_qgroup_inherit
>> structure and deprecates the following features:
>> 1) limit set
>>    Never utilized by btrfs-progs from the beginning.
>>
>> 2) copy rfer/excl
>>    Although btrfs-progs provides support for it as a hidden,
>>    undocumented feature, it's the easiest way to screw up qgroup
>>    numbers.
>>    And we already have patches wondering around the ML to remove such
>>    support.
> 
> The deprecation should be done in a few steps. First issue a warning
> that the feature is deprecated and will be removed in release X. Then
> wait until somebody complains (or not) and remove the code in release X.
> 
> The X is something like 4.22, ie. at least 2 cycles after the
> deprecation warning is added.

Thanks for the deprecation progress.

However I'm wondering if the "release X" is really needed in the warning
message.
(I may forgot to submit the real deprecation patch for that release).

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] btrfs: qgroup: Deprecate unused features for btrfs_qgroup_inherit()

[David Sterba]

On Thu, Sep 06, 2018 at 09:25:45PM +0800, Qu Wenruo wrote:
> > The deprecation should be done in a few steps. First issue a warning
> > that the feature is deprecated and will be removed in release X. Then
> > wait until somebody complains (or not) and remove the code in release X.
> > 
> > The X is something like 4.22, ie. at least 2 cycles after the
> > deprecation warning is added.
> 
> Thanks for the deprecation progress.
> 
> However I'm wondering if the "release X" is really needed in the warning
> message.

The specific version or date is IMHO a good practice. For features known
to be used the deprecation period may be years long and hard to predict
the version. In case of the qgroup inheritance it's a minor feature set
reduction and the 2 release period is there just in case. We can be
specific here and we should put that into the message.

> (I may forgot to submit the real deprecation patch for that release).

I document the feature deprecation and removal on the wiki changelog, so
if you forget, slipping one release causes no harm but I think somebody
will notice in time.

Re: [PATCH 2/3] btrfs: qgroup: Validate btrfs_qgroup_inherit structure before passing it to qgroup

[Omar Sandoval]

On Fri, Aug 31, 2018 at 10:29:29AM +0800, Qu Wenruo wrote:
> btrfs_qgroup_inherit structure doesn't goes through much validation
> check.
> 
> Now do a comprehensive check for it, including:
> 1) inherit size
>    Should not exceeding SZ_4K and its num_qgroups should not
>    exceed its size passed in btrfs_ioctl_vol_args_v2.
> 
> 2) flags
>    Should not include any unknown flags
>    (In fact, no flag is supported at all now)
>    Btrfs-progs never has such ability to set flags for btrfs_qgroup_inherit.
> 
> 3) limit
>    Should not contain anything.
>    Btrfs-progs never has such ability to set limit for btrfs_qgroup_inherit.
> 
> 4) rfer/excl copy
>    Deprecated feature.
>    Btrfs-progs has such interface but never documented and we're already
>    going to remove such ability.
>    It's the easiest way to screw up qgroup numbers.
> 
> 3) Qgroupid
>    Comprehensive check is already in btrfs_qgroup_inherit(), here we
>    only check if there is any obviously invalid qgroupid (0).
> 
> Coverity-id: 1021055
> Reported-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ioctl.c           |  3 +++
>  fs/btrfs/qgroup.c          | 39 ++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/qgroup.h          |  2 ++
>  include/uapi/linux/btrfs.h | 17 ++++++++---------
>  4 files changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 5db8680b40a9..4f5f453d5d07 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1820,6 +1820,9 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
>  			ret = PTR_ERR(inherit);
>  			goto free_args;
>  		}
> +		ret = btrfs_validate_inherit(inherit, vol_args->size);
> +		if (ret < 0)
> +			goto free_args;
>  	}
>  
>  	ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 4353bb69bb86..53daf73b0de9 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2232,6 +2232,45 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans)
>  	return ret;
>  }
>  
> +/*
> + * To make sure the inherit passed in is valid
> + *
> + * Here we only check flags and rule out some no-longer supported features.
> + * And we only do very basis qgroupid check to ensure there is no obviously
> + * invalid qgroupid (0). Detailed qgroupid check will be done in
> + * btrfs_qgroup_inherit().
> + */
> +int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
> +			   u64 inherit_size)
> +{
> +	u64 i;
> +
> +	if (inherit->flags & ~BTRFS_QGROUP_INHERIT_FLAGS_SUPP)
> +		return -ENOTTY;
> +	/* Qgroup rfer/excl copy is deprecated */
> +	if (inherit->num_excl_copies || inherit->num_ref_copies)
> +		return -ENOTTY;
> +
> +	/* Since SET_LIMITS is never used, @lim should all be zeroed */
> +	if (inherit->lim.max_excl || inherit->lim.max_rfer ||
> +	    inherit->lim.rsv_excl || inherit->lim.rsv_rfer ||
> +	    inherit->lim.flags)
> +		return -ENOTTY;

Why -ENOTTY? I think these should all be -EINVAL.

> +	/* Size check */
> +	if (sizeof(u64) * inherit->num_qgroups + sizeof(*inherit) >

This arithmetic can overflow...

> +	    min_t(u64, BTRFS_QGROUP_INHERIT_MAX_SIZE, inherit_size))
> +		return -EINVAL;
> +
> +
> +	/* Qgroup 0/0 is not allowed */
> +	for (i = 0; i < inherit->num_qgroups; i++) {
> +		if (inherit->qgroups[i] == 0)

Which means we can access out of bounds here.

> +			return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Copy the accounting information between qgroups. This is necessary
>   * when a snapshot or a subvolume is created. Throwing an error will
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index 54b8bb282c0e..1bf9c584be70 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -241,6 +241,8 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>  				struct ulist *new_roots);
>  int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans);
>  int btrfs_run_qgroups(struct btrfs_trans_handle *trans);
> +int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
> +			   u64 inherit_size);
>  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  			 u64 objectid, struct btrfs_qgroup_inherit *inherit);
>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index 311edb65567c..5a5532a20019 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -74,21 +74,20 @@ struct btrfs_qgroup_limit {
>  	__u64	rsv_excl;
>  };
>  
> -/*
> - * flags definition for qgroup inheritance
> - *
> - * Used by:
> - * struct btrfs_qgroup_inherit.flags
> - */
> +/* flags definition for qgroup inheritance (DEPRECATED) */
>  #define BTRFS_QGROUP_INHERIT_SET_LIMITS	(1ULL << 0)
>  
> +/* No supported flags */
> +#define BTRFS_QGROUP_INHERIT_FLAGS_SUPP (0)
> +
>  #define BTRFS_QGROUP_INHERIT_MAX_SIZE	(SZ_4K)
> +
>  struct btrfs_qgroup_inherit {
>  	__u64	flags;
>  	__u64	num_qgroups;
> -	__u64	num_ref_copies;
> -	__u64	num_excl_copies;
> -	struct btrfs_qgroup_limit lim;
> +	__u64	num_ref_copies;		/* DEPRECATED */
> +	__u64	num_excl_copies;	/* DEPRECATED */
> +	struct btrfs_qgroup_limit lim;	/* DEPRECATED */
>  	__u64	qgroups[0];
>  };
>  
> -- 
> 2.18.0
> 

Re: [PATCH 3/3] btrfs: qgroup: Remove deprecated feature support in btrfs_qgorup_inhert()

[Omar Sandoval]

On Fri, Aug 31, 2018 at 10:29:30AM +0800, Qu Wenruo wrote:
> Since btrfs_validate_inherit() will not allow features like copy
> rfer/excl and limit set, remove these dead code.

Reviewed-by: Omar Sandoval <osandov@fb.com>

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 57 +----------------------------------------------
>  1 file changed, 1 insertion(+), 56 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 53daf73b0de9..b57577f45ff3 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2301,8 +2301,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  
>  	if (inherit) {
>  		i_qgroups = (u64 *)(inherit + 1);
> -		nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
> -		       2 * inherit->num_excl_copies;
> +		nums = inherit->num_qgroups;
>  		for (i = 0; i < nums; ++i) {
>  			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
>  
> @@ -2354,23 +2353,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  		goto unlock;
>  	}
>  
> -	if (inherit && inherit->flags & BTRFS_QGROUP_INHERIT_SET_LIMITS) {
> -		dstgroup->lim_flags = inherit->lim.flags;
> -		dstgroup->max_rfer = inherit->lim.max_rfer;
> -		dstgroup->max_excl = inherit->lim.max_excl;
> -		dstgroup->rsv_rfer = inherit->lim.rsv_rfer;
> -		dstgroup->rsv_excl = inherit->lim.rsv_excl;
> -
> -		ret = update_qgroup_limit_item(trans, dstgroup);
> -		if (ret) {
> -			fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
> -			btrfs_info(fs_info,
> -				   "unable to update quota limit for %llu",
> -				   dstgroup->qgroupid);
> -			goto unlock;
> -		}
> -	}
> -
>  	if (srcid) {
>  		srcgroup = find_qgroup_rb(fs_info, srcid);
>  		if (!srcgroup)
> @@ -2413,43 +2395,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>  		++i_qgroups;
>  	}
>  
> -	for (i = 0; i <  inherit->num_ref_copies; ++i, i_qgroups += 2) {
> -		struct btrfs_qgroup *src;
> -		struct btrfs_qgroup *dst;
> -
> -		if (!i_qgroups[0] || !i_qgroups[1])
> -			continue;
> -
> -		src = find_qgroup_rb(fs_info, i_qgroups[0]);
> -		dst = find_qgroup_rb(fs_info, i_qgroups[1]);
> -
> -		if (!src || !dst) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -
> -		dst->rfer = src->rfer - level_size;
> -		dst->rfer_cmpr = src->rfer_cmpr - level_size;
> -	}
> -	for (i = 0; i <  inherit->num_excl_copies; ++i, i_qgroups += 2) {
> -		struct btrfs_qgroup *src;
> -		struct btrfs_qgroup *dst;
> -
> -		if (!i_qgroups[0] || !i_qgroups[1])
> -			continue;
> -
> -		src = find_qgroup_rb(fs_info, i_qgroups[0]);
> -		dst = find_qgroup_rb(fs_info, i_qgroups[1]);
> -
> -		if (!src || !dst) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -
> -		dst->excl = src->excl + level_size;
> -		dst->excl_cmpr = src->excl_cmpr + level_size;
> -	}
> -
>  unlock:
>  	spin_unlock(&fs_info->qgroup_lock);
>  out:
> -- 
> 2.18.0
> 

Re: [PATCH 2/3] btrfs: qgroup: Validate btrfs_qgroup_inherit structure before passing it to qgroup

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5942 bytes --]



On 2018/9/8 上午4:50, Omar Sandoval wrote:
> On Fri, Aug 31, 2018 at 10:29:29AM +0800, Qu Wenruo wrote:
>> btrfs_qgroup_inherit structure doesn't goes through much validation
>> check.
>>
>> Now do a comprehensive check for it, including:
>> 1) inherit size
>>    Should not exceeding SZ_4K and its num_qgroups should not
>>    exceed its size passed in btrfs_ioctl_vol_args_v2.
>>
>> 2) flags
>>    Should not include any unknown flags
>>    (In fact, no flag is supported at all now)
>>    Btrfs-progs never has such ability to set flags for btrfs_qgroup_inherit.
>>
>> 3) limit
>>    Should not contain anything.
>>    Btrfs-progs never has such ability to set limit for btrfs_qgroup_inherit.
>>
>> 4) rfer/excl copy
>>    Deprecated feature.
>>    Btrfs-progs has such interface but never documented and we're already
>>    going to remove such ability.
>>    It's the easiest way to screw up qgroup numbers.
>>
>> 3) Qgroupid
>>    Comprehensive check is already in btrfs_qgroup_inherit(), here we
>>    only check if there is any obviously invalid qgroupid (0).
>>
>> Coverity-id: 1021055
>> Reported-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/ioctl.c           |  3 +++
>>  fs/btrfs/qgroup.c          | 39 ++++++++++++++++++++++++++++++++++++++
>>  fs/btrfs/qgroup.h          |  2 ++
>>  include/uapi/linux/btrfs.h | 17 ++++++++---------
>>  4 files changed, 52 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 5db8680b40a9..4f5f453d5d07 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1820,6 +1820,9 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file,
>>  			ret = PTR_ERR(inherit);
>>  			goto free_args;
>>  		}
>> +		ret = btrfs_validate_inherit(inherit, vol_args->size);
>> +		if (ret < 0)
>> +			goto free_args;
>>  	}
>>  
>>  	ret = btrfs_ioctl_snap_create_transid(file, vol_args->name,
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 4353bb69bb86..53daf73b0de9 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -2232,6 +2232,45 @@ int btrfs_run_qgroups(struct btrfs_trans_handle *trans)
>>  	return ret;
>>  }
>>  
>> +/*
>> + * To make sure the inherit passed in is valid
>> + *
>> + * Here we only check flags and rule out some no-longer supported features.
>> + * And we only do very basis qgroupid check to ensure there is no obviously
>> + * invalid qgroupid (0). Detailed qgroupid check will be done in
>> + * btrfs_qgroup_inherit().
>> + */
>> +int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
>> +			   u64 inherit_size)
>> +{
>> +	u64 i;
>> +
>> +	if (inherit->flags & ~BTRFS_QGROUP_INHERIT_FLAGS_SUPP)
>> +		return -ENOTTY;
>> +	/* Qgroup rfer/excl copy is deprecated */
>> +	if (inherit->num_excl_copies || inherit->num_ref_copies)
>> +		return -ENOTTY;
>> +
>> +	/* Since SET_LIMITS is never used, @lim should all be zeroed */
>> +	if (inherit->lim.max_excl || inherit->lim.max_rfer ||
>> +	    inherit->lim.rsv_excl || inherit->lim.rsv_rfer ||
>> +	    inherit->lim.flags)
>> +		return -ENOTTY;
> 
> Why -ENOTTY? I think these should all be -EINVAL.

Changed in v2.

Now it only outputs warning message.

> 
>> +	/* Size check */
>> +	if (sizeof(u64) * inherit->num_qgroups + sizeof(*inherit) >
> 
> This arithmetic can overflow...

Oh, forgot that possibility.

inherit->num_qgroups/excl_copies/rfer_coopies should be checked against
(BTRFS_QGROUP_INHERIT_MAX_SIZE - sizeof(*inherit))/(sizeof(u64).

Thanks,
Qu

> 
>> +	    min_t(u64, BTRFS_QGROUP_INHERIT_MAX_SIZE, inherit_size))
>> +		return -EINVAL;
>> +
>> +
>> +	/* Qgroup 0/0 is not allowed */
>> +	for (i = 0; i < inherit->num_qgroups; i++) {
>> +		if (inherit->qgroups[i] == 0)
> 
> Which means we can access out of bounds here.
> 
>> +			return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>>  /*
>>   * Copy the accounting information between qgroups. This is necessary
>>   * when a snapshot or a subvolume is created. Throwing an error will
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index 54b8bb282c0e..1bf9c584be70 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -241,6 +241,8 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>>  				struct ulist *new_roots);
>>  int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans);
>>  int btrfs_run_qgroups(struct btrfs_trans_handle *trans);
>> +int btrfs_validate_inherit(struct btrfs_qgroup_inherit *inherit,
>> +			   u64 inherit_size);
>>  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>>  			 u64 objectid, struct btrfs_qgroup_inherit *inherit);
>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
>> index 311edb65567c..5a5532a20019 100644
>> --- a/include/uapi/linux/btrfs.h
>> +++ b/include/uapi/linux/btrfs.h
>> @@ -74,21 +74,20 @@ struct btrfs_qgroup_limit {
>>  	__u64	rsv_excl;
>>  };
>>  
>> -/*
>> - * flags definition for qgroup inheritance
>> - *
>> - * Used by:
>> - * struct btrfs_qgroup_inherit.flags
>> - */
>> +/* flags definition for qgroup inheritance (DEPRECATED) */
>>  #define BTRFS_QGROUP_INHERIT_SET_LIMITS	(1ULL << 0)
>>  
>> +/* No supported flags */
>> +#define BTRFS_QGROUP_INHERIT_FLAGS_SUPP (0)
>> +
>>  #define BTRFS_QGROUP_INHERIT_MAX_SIZE	(SZ_4K)
>> +
>>  struct btrfs_qgroup_inherit {
>>  	__u64	flags;
>>  	__u64	num_qgroups;
>> -	__u64	num_ref_copies;
>> -	__u64	num_excl_copies;
>> -	struct btrfs_qgroup_limit lim;
>> +	__u64	num_ref_copies;		/* DEPRECATED */
>> +	__u64	num_excl_copies;	/* DEPRECATED */
>> +	struct btrfs_qgroup_limit lim;	/* DEPRECATED */
>>  	__u64	qgroups[0];
>>  };
>>  
>> -- 
>> 2.18.0
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]