btrfs: handle non-fatal errors in btrfs_qgroup_inherit()
diff mbox

Message ID 20160331005748.GI15881@wotan.suse.de
State Accepted
Headers show

Commit Message

Mark Fasheh March 31, 2016, 12:57 a.m. UTC
create_pending_snapshot() will go readonly on _any_ error return from
btrfs_qgroup_inherit(). If qgroups are enabled, a user can crash their fs by
just making a snapshot and asking it to inherit from an invalid qgroup. For
example:

$ btrfs sub snap -i 1/10 /btrfs/ /btrfs/foo

Will cause a transaction abort.

Fix this by only throwing errors in btrfs_qgroup_inherit() when we know
going readonly is acceptable.

The following xfstests test case reproduces this bug:

  seq=`basename $0`
  seqres=$RESULT_DIR/$seq
  echo "QA output created by $seq"

  here=`pwd`
  tmp=/tmp/$$
  status=1	# failure is the default!
  trap "_cleanup; exit \$status" 0 1 2 3 15

  _cleanup()
  {
  	cd /
  	rm -f $tmp.*
  }

  # get standard environment, filters and checks
  . ./common/rc
  . ./common/filter

  # remove previous $seqres.full before test
  rm -f $seqres.full

  # real QA test starts here
  _supported_fs btrfs
  _supported_os Linux
  _require_scratch

  rm -f $seqres.full

  _scratch_mkfs
  _scratch_mount
  _run_btrfs_util_prog quota enable $SCRATCH_MNT
  # The qgroup '1/10' does not exist and should be silently ignored
  _run_btrfs_util_prog subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1

  _scratch_unmount

  echo "Silence is golden"

  status=0
  exit

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/qgroup.c | 54 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

Comments

Qu Wenruo March 31, 2016, 2:20 a.m. UTC | #1
Mark Fasheh wrote on 2016/03/30 17:57 -0700:
> create_pending_snapshot() will go readonly on _any_ error return from
> btrfs_qgroup_inherit(). If qgroups are enabled, a user can crash their fs by
> just making a snapshot and asking it to inherit from an invalid qgroup. For
> example:
>
> $ btrfs sub snap -i 1/10 /btrfs/ /btrfs/foo
>
> Will cause a transaction abort.
>
> Fix this by only throwing errors in btrfs_qgroup_inherit() when we know
> going readonly is acceptable.
>
> The following xfstests test case reproduces this bug:
>
>    seq=`basename $0`
>    seqres=$RESULT_DIR/$seq
>    echo "QA output created by $seq"
>
>    here=`pwd`
>    tmp=/tmp/$$
>    status=1	# failure is the default!
>    trap "_cleanup; exit \$status" 0 1 2 3 15
>
>    _cleanup()
>    {
>    	cd /
>    	rm -f $tmp.*
>    }
>
>    # get standard environment, filters and checks
>    . ./common/rc
>    . ./common/filter
>
>    # remove previous $seqres.full before test
>    rm -f $seqres.full
>
>    # real QA test starts here
>    _supported_fs btrfs
>    _supported_os Linux
>    _require_scratch
>
>    rm -f $seqres.full
>
>    _scratch_mkfs
>    _scratch_mount
>    _run_btrfs_util_prog quota enable $SCRATCH_MNT
>    # The qgroup '1/10' does not exist and should be silently ignored
>    _run_btrfs_util_prog subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1
>
>    _scratch_unmount
>
>    echo "Silence is golden"
>
>    status=0
>    exit
>
> Signed-off-by: Mark Fasheh <mfasheh@suse.de>

Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Looks good to me, and right current check is too restrict and will cause 
annoying abort_transaction.

Although silently ignore invalid assign is somewhat too casual, that's 
already much better than current abort_transaction.

Thanks,
Qu
> ---
>   fs/btrfs/qgroup.c | 54 ++++++++++++++++++++++++++++++++----------------------
>   1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 994dab0..9e11955 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1851,8 +1851,10 @@ out:
>   }
>
>   /*
> - * copy the acounting information between qgroups. This is necessary when a
> - * snapshot or a subvolume is created
> + * Copy the acounting information between qgroups. This is necessary
> + * when a snapshot or a subvolume is created. Throwing an error will
> + * cause a transaction abort so we take extra care here to only error
> + * when a readonly fs is a reasonable outcome.
>    */
>   int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>   			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
> @@ -1882,15 +1884,15 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>   		       2 * inherit->num_excl_copies;
>   		for (i = 0; i < nums; ++i) {
>   			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
> -			if (!srcgroup) {
> -				ret = -EINVAL;
> -				goto out;
> -			}
>
> -			if ((srcgroup->qgroupid >> 48) <= (objectid >> 48)) {
> -				ret = -EINVAL;
> -				goto out;
> -			}
> +			/*
> +			 * Zero out invalid groups so we can ignore
> +			 * them later.
> +			 */
> +			if (!srcgroup ||
> +			    ((srcgroup->qgroupid >> 48) <= (objectid >> 48)))
> +				*i_qgroups = 0ULL;
> +
>   			++i_qgroups;
>   		}
>   	}
> @@ -1925,17 +1927,19 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>   	 */
>   	if (inherit) {
>   		i_qgroups = (u64 *)(inherit + 1);
> -		for (i = 0; i < inherit->num_qgroups; ++i) {
> +		for (i = 0; i < inherit->num_qgroups; ++i, ++i_qgroups) {
> +			if (*i_qgroups == 0)
> +				continue;
>   			ret = add_qgroup_relation_item(trans, quota_root,
>   						       objectid, *i_qgroups);
> -			if (ret)
> +			if (ret && ret != -EEXIST)
>   				goto out;
>   			ret = add_qgroup_relation_item(trans, quota_root,
>   						       *i_qgroups, objectid);
> -			if (ret)
> +			if (ret && ret != -EEXIST)
>   				goto out;
> -			++i_qgroups;
>   		}
> +		ret = 0;
>   	}
>
>
> @@ -1996,17 +2000,22 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>
>   	i_qgroups = (u64 *)(inherit + 1);
>   	for (i = 0; i < inherit->num_qgroups; ++i) {
> -		ret = add_relation_rb(quota_root->fs_info, objectid,
> -				      *i_qgroups);
> -		if (ret)
> -			goto unlock;
> +		if (*i_qgroups) {
> +			ret = add_relation_rb(quota_root->fs_info, objectid,
> +					      *i_qgroups);
> +			if (ret)
> +				goto unlock;
> +		}
>   		++i_qgroups;
>   	}
>
> -	for (i = 0; i <  inherit->num_ref_copies; ++i) {
> +	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]);
>
> @@ -2017,12 +2026,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>
>   		dst->rfer = src->rfer - level_size;
>   		dst->rfer_cmpr = src->rfer_cmpr - level_size;
> -		i_qgroups += 2;
>   	}
> -	for (i = 0; i <  inherit->num_excl_copies; ++i) {
> +	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]);
>
> @@ -2033,7 +2044,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>
>   		dst->excl = src->excl + level_size;
>   		dst->excl_cmpr = src->excl_cmpr + level_size;
> -		i_qgroups += 2;
>   	}
>
>   unlock:
>


--
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
Filipe Manana April 6, 2016, 9:22 a.m. UTC | #2
On Thu, Mar 31, 2016 at 1:57 AM, Mark Fasheh <mfasheh@suse.de> wrote:
> create_pending_snapshot() will go readonly on _any_ error return from
> btrfs_qgroup_inherit(). If qgroups are enabled, a user can crash their fs by
> just making a snapshot and asking it to inherit from an invalid qgroup. For
> example:
>
> $ btrfs sub snap -i 1/10 /btrfs/ /btrfs/foo
>
> Will cause a transaction abort.
>
> Fix this by only throwing errors in btrfs_qgroup_inherit() when we know
> going readonly is acceptable.
>
> The following xfstests test case reproduces this bug:
>
>   seq=`basename $0`
>   seqres=$RESULT_DIR/$seq
>   echo "QA output created by $seq"
>
>   here=`pwd`
>   tmp=/tmp/$$
>   status=1      # failure is the default!
>   trap "_cleanup; exit \$status" 0 1 2 3 15
>
>   _cleanup()
>   {
>         cd /
>         rm -f $tmp.*
>   }
>
>   # get standard environment, filters and checks
>   . ./common/rc
>   . ./common/filter
>
>   # remove previous $seqres.full before test
>   rm -f $seqres.full
>
>   # real QA test starts here
>   _supported_fs btrfs
>   _supported_os Linux
>   _require_scratch
>
>   rm -f $seqres.full
>
>   _scratch_mkfs
>   _scratch_mount
>   _run_btrfs_util_prog quota enable $SCRATCH_MNT
>   # The qgroup '1/10' does not exist and should be silently ignored
>   _run_btrfs_util_prog subvolume snapshot -i 1/10 $SCRATCH_MNT $SCRATCH_MNT/snap1
>
>   _scratch_unmount
>
>   echo "Silence is golden"
>
>   status=0
>   exit

Mark, did you forgot to submit a patch with the test case for fstests,
or is there any other reason why you didn't do it?
Thanks.

>
> Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> ---
>  fs/btrfs/qgroup.c | 54 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 994dab0..9e11955 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1851,8 +1851,10 @@ out:
>  }
>
>  /*
> - * copy the acounting information between qgroups. This is necessary when a
> - * snapshot or a subvolume is created
> + * Copy the acounting information between qgroups. This is necessary
> + * when a snapshot or a subvolume is created. Throwing an error will
> + * cause a transaction abort so we take extra care here to only error
> + * when a readonly fs is a reasonable outcome.
>   */
>  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>                          struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
> @@ -1882,15 +1884,15 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>                        2 * inherit->num_excl_copies;
>                 for (i = 0; i < nums; ++i) {
>                         srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
> -                       if (!srcgroup) {
> -                               ret = -EINVAL;
> -                               goto out;
> -                       }
>
> -                       if ((srcgroup->qgroupid >> 48) <= (objectid >> 48)) {
> -                               ret = -EINVAL;
> -                               goto out;
> -                       }
> +                       /*
> +                        * Zero out invalid groups so we can ignore
> +                        * them later.
> +                        */
> +                       if (!srcgroup ||
> +                           ((srcgroup->qgroupid >> 48) <= (objectid >> 48)))
> +                               *i_qgroups = 0ULL;
> +
>                         ++i_qgroups;
>                 }
>         }
> @@ -1925,17 +1927,19 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>          */
>         if (inherit) {
>                 i_qgroups = (u64 *)(inherit + 1);
> -               for (i = 0; i < inherit->num_qgroups; ++i) {
> +               for (i = 0; i < inherit->num_qgroups; ++i, ++i_qgroups) {
> +                       if (*i_qgroups == 0)
> +                               continue;
>                         ret = add_qgroup_relation_item(trans, quota_root,
>                                                        objectid, *i_qgroups);
> -                       if (ret)
> +                       if (ret && ret != -EEXIST)
>                                 goto out;
>                         ret = add_qgroup_relation_item(trans, quota_root,
>                                                        *i_qgroups, objectid);
> -                       if (ret)
> +                       if (ret && ret != -EEXIST)
>                                 goto out;
> -                       ++i_qgroups;
>                 }
> +               ret = 0;
>         }
>
>
> @@ -1996,17 +2000,22 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>
>         i_qgroups = (u64 *)(inherit + 1);
>         for (i = 0; i < inherit->num_qgroups; ++i) {
> -               ret = add_relation_rb(quota_root->fs_info, objectid,
> -                                     *i_qgroups);
> -               if (ret)
> -                       goto unlock;
> +               if (*i_qgroups) {
> +                       ret = add_relation_rb(quota_root->fs_info, objectid,
> +                                             *i_qgroups);
> +                       if (ret)
> +                               goto unlock;
> +               }
>                 ++i_qgroups;
>         }
>
> -       for (i = 0; i <  inherit->num_ref_copies; ++i) {
> +       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]);
>
> @@ -2017,12 +2026,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>
>                 dst->rfer = src->rfer - level_size;
>                 dst->rfer_cmpr = src->rfer_cmpr - level_size;
> -               i_qgroups += 2;
>         }
> -       for (i = 0; i <  inherit->num_excl_copies; ++i) {
> +       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]);
>
> @@ -2033,7 +2044,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>
>                 dst->excl = src->excl + level_size;
>                 dst->excl_cmpr = src->excl_cmpr + level_size;
> -               i_qgroups += 2;
>         }
>
>  unlock:
> --
> 2.1.4
>
> --
> 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
Mark Fasheh April 6, 2016, 7:48 p.m. UTC | #3
On Wed, Apr 06, 2016 at 10:22:57AM +0100, Filipe Manana wrote:
> Mark, did you forgot to submit a patch with the test case for fstests,
> or is there any other reason why you didn't do it?

No, I was just waiting to see how my fix did in review. I'll be shooting
that test over later today.
	--Mark

--
Mark Fasheh
--
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

Patch
diff mbox

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 994dab0..9e11955 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1851,8 +1851,10 @@  out:
 }
 
 /*
- * copy the acounting information between qgroups. This is necessary when a
- * snapshot or a subvolume is created
+ * Copy the acounting information between qgroups. This is necessary
+ * when a snapshot or a subvolume is created. Throwing an error will
+ * cause a transaction abort so we take extra care here to only error
+ * when a readonly fs is a reasonable outcome.
  */
 int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
@@ -1882,15 +1884,15 @@  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 		       2 * inherit->num_excl_copies;
 		for (i = 0; i < nums; ++i) {
 			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
-			if (!srcgroup) {
-				ret = -EINVAL;
-				goto out;
-			}
 
-			if ((srcgroup->qgroupid >> 48) <= (objectid >> 48)) {
-				ret = -EINVAL;
-				goto out;
-			}
+			/*
+			 * Zero out invalid groups so we can ignore
+			 * them later.
+			 */
+			if (!srcgroup ||
+			    ((srcgroup->qgroupid >> 48) <= (objectid >> 48)))
+				*i_qgroups = 0ULL;
+
 			++i_qgroups;
 		}
 	}
@@ -1925,17 +1927,19 @@  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 	 */
 	if (inherit) {
 		i_qgroups = (u64 *)(inherit + 1);
-		for (i = 0; i < inherit->num_qgroups; ++i) {
+		for (i = 0; i < inherit->num_qgroups; ++i, ++i_qgroups) {
+			if (*i_qgroups == 0)
+				continue;
 			ret = add_qgroup_relation_item(trans, quota_root,
 						       objectid, *i_qgroups);
-			if (ret)
+			if (ret && ret != -EEXIST)
 				goto out;
 			ret = add_qgroup_relation_item(trans, quota_root,
 						       *i_qgroups, objectid);
-			if (ret)
+			if (ret && ret != -EEXIST)
 				goto out;
-			++i_qgroups;
 		}
+		ret = 0;
 	}
 
 
@@ -1996,17 +2000,22 @@  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 
 	i_qgroups = (u64 *)(inherit + 1);
 	for (i = 0; i < inherit->num_qgroups; ++i) {
-		ret = add_relation_rb(quota_root->fs_info, objectid,
-				      *i_qgroups);
-		if (ret)
-			goto unlock;
+		if (*i_qgroups) {
+			ret = add_relation_rb(quota_root->fs_info, objectid,
+					      *i_qgroups);
+			if (ret)
+				goto unlock;
+		}
 		++i_qgroups;
 	}
 
-	for (i = 0; i <  inherit->num_ref_copies; ++i) {
+	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]);
 
@@ -2017,12 +2026,14 @@  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 
 		dst->rfer = src->rfer - level_size;
 		dst->rfer_cmpr = src->rfer_cmpr - level_size;
-		i_qgroups += 2;
 	}
-	for (i = 0; i <  inherit->num_excl_copies; ++i) {
+	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]);
 
@@ -2033,7 +2044,6 @@  int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 
 		dst->excl = src->excl + level_size;
 		dst->excl_cmpr = src->excl_cmpr + level_size;
-		i_qgroups += 2;
 	}
 
 unlock: