* [PATCH] btrfs: handle non-fatal errors in btrfs_qgroup_inherit()
@ 2016-03-31 0:57 Mark Fasheh
2016-03-31 2:20 ` Qu Wenruo
2016-04-06 9:22 ` Filipe Manana
0 siblings, 2 replies; 4+ messages in thread
From: Mark Fasheh @ 2016-03-31 0:57 UTC (permalink / raw)
To: linux-btrfs; +Cc: clm, jbacik
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: handle non-fatal errors in btrfs_qgroup_inherit()
2016-03-31 0:57 [PATCH] btrfs: handle non-fatal errors in btrfs_qgroup_inherit() Mark Fasheh
@ 2016-03-31 2:20 ` Qu Wenruo
2016-04-06 9:22 ` Filipe Manana
1 sibling, 0 replies; 4+ messages in thread
From: Qu Wenruo @ 2016-03-31 2:20 UTC (permalink / raw)
To: Mark Fasheh, linux-btrfs; +Cc: clm, jbacik
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:
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: handle non-fatal errors in btrfs_qgroup_inherit()
2016-03-31 0:57 [PATCH] btrfs: handle non-fatal errors in btrfs_qgroup_inherit() Mark Fasheh
2016-03-31 2:20 ` Qu Wenruo
@ 2016-04-06 9:22 ` Filipe Manana
2016-04-06 19:48 ` Mark Fasheh
1 sibling, 1 reply; 4+ messages in thread
From: Filipe Manana @ 2016-04-06 9:22 UTC (permalink / raw)
To: Mark Fasheh; +Cc: linux-btrfs@vger.kernel.org, Chris Mason, Josef Bacik
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
--
Filipe David Manana,
"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves.
That's why all progress depends on unreasonable men."
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] btrfs: handle non-fatal errors in btrfs_qgroup_inherit()
2016-04-06 9:22 ` Filipe Manana
@ 2016-04-06 19:48 ` Mark Fasheh
0 siblings, 0 replies; 4+ messages in thread
From: Mark Fasheh @ 2016-04-06 19:48 UTC (permalink / raw)
To: Filipe Manana; +Cc: linux-btrfs@vger.kernel.org, Chris Mason, Josef Bacik
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-04-06 19:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-31 0:57 [PATCH] btrfs: handle non-fatal errors in btrfs_qgroup_inherit() Mark Fasheh
2016-03-31 2:20 ` Qu Wenruo
2016-04-06 9:22 ` Filipe Manana
2016-04-06 19:48 ` Mark Fasheh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).