| Message ID | 20201119141956.6488-1-lhenriques@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ceph: add a new test for cross quota realms renames | expand |
On Thu, Nov 19, 2020 at 02:19:56PM +0000, Luis Henriques wrote: > For the moment cross quota realms renames has been disabled in CephFS > after a bug has been found while renaming files created and truncated. > This allowed clients to easily circumvent quotas. > > Link: https://tracker.ceph.com/issues/48203 > Signed-off-by: Luis Henriques <lhenriques@suse.de> Thanks for the test! It'd be great if ceph folks could help review & test it as well, as I don't have a cephfs test env yet.. Some minor comments below. > --- > tests/ceph/004 | 94 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/ceph/004.out | 2 + > tests/ceph/group | 1 + > 3 files changed, 97 insertions(+) > create mode 100755 tests/ceph/004 > create mode 100644 tests/ceph/004.out > > diff --git a/tests/ceph/004 b/tests/ceph/004 > new file mode 100755 > index 000000000000..4021666b138e > --- /dev/null > +++ b/tests/ceph/004 > @@ -0,0 +1,94 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 004 > +# > +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase > +# that *should* fail: > +# > +# mkdir files limit > +# truncate files/file -s 10G > +# setfattr limit -n ceph.quota.max_bytes -v 1000000 > +# mv files limit/ > +# > +# Because we're creating a new file and truncating it, we have Fx caps and thus > +# the truncate operation will be cached. This prevents the MDSs from updating > +# the quota realms and thus the client will allow the above rename(2) to happen. > +# > +# The bug resulted in dropping support for cross quota-realms renames, reverting > +# kernel commit dffdcd71458e ("ceph: allow rename operation under different > +# quota realms"). > +# > +# So, the above test will now fail with a -EXDEV or, in the future (when we have > +# a proper fix), with -EDQUOT. > +# > +# This bug was tracker here: > +# > +# https://tracker.ceph.com/issues/48203 > +# > +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 > +. ./common/attr > + > +# remove previous $seqres.full before test > +rm -f $seqres.full > + > +# real QA test starts here > + > +_supported_fs ceph > +_require_attrs > +_require_test Need '_require_test_program "rename"' as well. > + > +workdir=$TEST_DIR/test-$seq > + > +orig1=$workdir/orig1 > +orig2=$workdir/orig2 > +file1=$orig1/file > +file2=$orig2/file > +dest=$workdir/dest > + > +rm -rf $workdir > +mkdir $workdir > +mkdir $orig1 $orig2 $dest > + > +# set quota to 1m > +$SETFATTR_PROG -n ceph.quota.max_bytes -v 1000000 $dest > +# set quota to 20g > +$SETFATTR_PROG -n ceph.quota.max_bytes -v 20000000000 $orig2 > + > +# > +# The following 2 testcases shall fail with either -EXDEV or -EDQUOT > +# > + > +# from 'root' realm to $dest realm > +$XFS_IO_PROG -f -c "truncate 10G" $file1 > +$here/src/rename $orig1 $dest/new1 >> $seqres.full 2>&1 > +[ $? -ne 1 ] && _fatal "cross quota realms rename succeeded" Use _fail instead of _fatal in test, _fatal is usually used in common helpers, to report internal errors such as wrong usage of helpers. > + > +# from $orig2 realm to $dest realm > +$XFS_IO_PROG -f -c "truncate 10G" $file2 > +$here/src/rename $orig2 $dest/new2 >> $seqres.full 2>&1 > +[ $? -ne 1 ] && _fatal "cross quota realms rename succeeded" Same here. Thanks, Eryu > + > +echo "Silence is golden" > + > +# success, all done > +status=0 > +exit > diff --git a/tests/ceph/004.out b/tests/ceph/004.out > new file mode 100644 > index 000000000000..af8614ae45ac > --- /dev/null > +++ b/tests/ceph/004.out > @@ -0,0 +1,2 @@ > +QA output created by 004 > +Silence is golden > diff --git a/tests/ceph/group b/tests/ceph/group > index adbf61547766..47903d21966c 100644 > --- a/tests/ceph/group > +++ b/tests/ceph/group > @@ -1,3 +1,4 @@ > 001 auto quick copy > 002 auto quick copy > 003 auto quick copy > +004 auto quick quota
Eryu Guan <guan@eryu.me> writes: > On Thu, Nov 19, 2020 at 02:19:56PM +0000, Luis Henriques wrote: >> For the moment cross quota realms renames has been disabled in CephFS >> after a bug has been found while renaming files created and truncated. >> This allowed clients to easily circumvent quotas. >> >> Link: https://tracker.ceph.com/issues/48203 >> Signed-off-by: Luis Henriques <lhenriques@suse.de> > > Thanks for the test! It'd be great if ceph folks could help review & > test it as well, as I don't have a cephfs test env yet.. > > Some minor comments below. Thanks for your review, Eryu. I'll send out v2 with your comments implemented. Cheers, -- Luis >> --- >> tests/ceph/004 | 94 ++++++++++++++++++++++++++++++++++++++++++++++ >> tests/ceph/004.out | 2 + >> tests/ceph/group | 1 + >> 3 files changed, 97 insertions(+) >> create mode 100755 tests/ceph/004 >> create mode 100644 tests/ceph/004.out >> >> diff --git a/tests/ceph/004 b/tests/ceph/004 >> new file mode 100755 >> index 000000000000..4021666b138e >> --- /dev/null >> +++ b/tests/ceph/004 >> @@ -0,0 +1,94 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. >> +# >> +# FS QA Test 004 >> +# >> +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase >> +# that *should* fail: >> +# >> +# mkdir files limit >> +# truncate files/file -s 10G >> +# setfattr limit -n ceph.quota.max_bytes -v 1000000 >> +# mv files limit/ >> +# >> +# Because we're creating a new file and truncating it, we have Fx caps and thus >> +# the truncate operation will be cached. This prevents the MDSs from updating >> +# the quota realms and thus the client will allow the above rename(2) to happen. >> +# >> +# The bug resulted in dropping support for cross quota-realms renames, reverting >> +# kernel commit dffdcd71458e ("ceph: allow rename operation under different >> +# quota realms"). >> +# >> +# So, the above test will now fail with a -EXDEV or, in the future (when we have >> +# a proper fix), with -EDQUOT. >> +# >> +# This bug was tracker here: >> +# >> +# https://tracker.ceph.com/issues/48203 >> +# >> +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 >> +. ./common/attr >> + >> +# remove previous $seqres.full before test >> +rm -f $seqres.full >> + >> +# real QA test starts here >> + >> +_supported_fs ceph >> +_require_attrs >> +_require_test > > Need '_require_test_program "rename"' as well. > >> + >> +workdir=$TEST_DIR/test-$seq >> + >> +orig1=$workdir/orig1 >> +orig2=$workdir/orig2 >> +file1=$orig1/file >> +file2=$orig2/file >> +dest=$workdir/dest >> + >> +rm -rf $workdir >> +mkdir $workdir >> +mkdir $orig1 $orig2 $dest >> + >> +# set quota to 1m >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v 1000000 $dest >> +# set quota to 20g >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v 20000000000 $orig2 >> + >> +# >> +# The following 2 testcases shall fail with either -EXDEV or -EDQUOT >> +# >> + >> +# from 'root' realm to $dest realm >> +$XFS_IO_PROG -f -c "truncate 10G" $file1 >> +$here/src/rename $orig1 $dest/new1 >> $seqres.full 2>&1 >> +[ $? -ne 1 ] && _fatal "cross quota realms rename succeeded" > > Use _fail instead of _fatal in test, _fatal is usually used in common > helpers, to report internal errors such as wrong usage of helpers. > >> + >> +# from $orig2 realm to $dest realm >> +$XFS_IO_PROG -f -c "truncate 10G" $file2 >> +$here/src/rename $orig2 $dest/new2 >> $seqres.full 2>&1 >> +[ $? -ne 1 ] && _fatal "cross quota realms rename succeeded" > > Same here. > > Thanks, > Eryu > >> + >> +echo "Silence is golden" >> + >> +# success, all done >> +status=0 >> +exit >> diff --git a/tests/ceph/004.out b/tests/ceph/004.out >> new file mode 100644 >> index 000000000000..af8614ae45ac >> --- /dev/null >> +++ b/tests/ceph/004.out >> @@ -0,0 +1,2 @@ >> +QA output created by 004 >> +Silence is golden >> diff --git a/tests/ceph/group b/tests/ceph/group >> index adbf61547766..47903d21966c 100644 >> --- a/tests/ceph/group >> +++ b/tests/ceph/group >> @@ -1,3 +1,4 @@ >> 001 auto quick copy >> 002 auto quick copy >> 003 auto quick copy >> +004 auto quick quota
On Mon, 2020-11-23 at 10:34 +0000, Luis Henriques wrote: > For the moment cross quota realms renames has been disabled in CephFS > after a bug has been found while renaming files created and truncated. > This allowed clients to easily circumvent quotas. > > Link: https://tracker.ceph.com/issues/48203 > Signed-off-by: Luis Henriques <lhenriques@suse.de> > --- > v2: implemented Eryu review comments: > - Added _require_test_program "rename" > - Use _fail instead of _fatal > > tests/ceph/004 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/ceph/004.out | 2 + > tests/ceph/group | 1 + > 3 files changed, 98 insertions(+) > create mode 100755 tests/ceph/004 > create mode 100644 tests/ceph/004.out > > diff --git a/tests/ceph/004 b/tests/ceph/004 > new file mode 100755 > index 000000000000..53094d8dfadc > --- /dev/null > +++ b/tests/ceph/004 > @@ -0,0 +1,95 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 004 > +# > +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase > +# that *should* fail: > +# > +# mkdir files limit > +# truncate files/file -s 10G > +# setfattr limit -n ceph.quota.max_bytes -v 1000000 > +# mv files limit/ > +# > +# Because we're creating a new file and truncating it, we have Fx caps and thus > +# the truncate operation will be cached. This prevents the MDSs from updating > +# the quota realms and thus the client will allow the above rename(2) to happen. > +# Note that it can be difficult to predict which caps you get from the MDS. It's not _required_ to pass out anything like Fx if it doesn't want to, but in general, it does if it can. It's not a blocker for merging this test, but I wonder if we ought to come up with some way to ensure that the client was given the caps we expect when testing stuff like this. Maybe we ought to consider adding a new ceph.caps vxattr that shows the caps we hold for a particular file? Then we could consult that when doing a test like this to make sure we got what we expected. > +# The bug resulted in dropping support for cross quota-realms renames, reverting > +# kernel commit dffdcd71458e ("ceph: allow rename operation under different > +# quota realms"). > +# > +# So, the above test will now fail with a -EXDEV or, in the future (when we have > +# a proper fix), with -EDQUOT. > +# > +# This bug was tracker here: > +# > +# https://tracker.ceph.com/issues/48203 > +# > +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 > +. ./common/attr > + > +# remove previous $seqres.full before test > +rm -f $seqres.full > + > +# real QA test starts here > + > +_supported_fs ceph > +_require_attrs > +_require_test > +_require_test_program "rename" > + > +workdir=$TEST_DIR/test-$seq > + > +orig1=$workdir/orig1 > +orig2=$workdir/orig2 > +file1=$orig1/file > +file2=$orig2/file > +dest=$workdir/dest > + > +rm -rf $workdir > +mkdir $workdir > +mkdir $orig1 $orig2 $dest > + > +# set quota to 1m > +$SETFATTR_PROG -n ceph.quota.max_bytes -v 1000000 $dest > +# set quota to 20g > +$SETFATTR_PROG -n ceph.quota.max_bytes -v 20000000000 $orig2 > + > +# > +# The following 2 testcases shall fail with either -EXDEV or -EDQUOT > +# > + > +# from 'root' realm to $dest realm > +$XFS_IO_PROG -f -c "truncate 10G" $file1 > +$here/src/rename $orig1 $dest/new1 >> $seqres.full 2>&1 > +[ $? -ne 1 ] && _fail "cross quota realms rename succeeded" > + > +# from $orig2 realm to $dest realm > +$XFS_IO_PROG -f -c "truncate 10G" $file2 > +$here/src/rename $orig2 $dest/new2 >> $seqres.full 2>&1 > +[ $? -ne 1 ] && _fail "cross quota realms rename succeeded" > + > +echo "Silence is golden" > + > +# success, all done > +status=0 > +exit > diff --git a/tests/ceph/004.out b/tests/ceph/004.out > new file mode 100644 > index 000000000000..af8614ae45ac > --- /dev/null > +++ b/tests/ceph/004.out > @@ -0,0 +1,2 @@ > +QA output created by 004 > +Silence is golden > diff --git a/tests/ceph/group b/tests/ceph/group > index adbf61547766..47903d21966c 100644 > --- a/tests/ceph/group > +++ b/tests/ceph/group > @@ -1,3 +1,4 @@ > 001 auto quick copy > 002 auto quick copy > 003 auto quick copy > +004 auto quick quota This looks good to me. Reviewed-by: Jeff Layton <jlayton@kernel.org>
Jeff Layton <jlayton@kernel.org> writes: > On Mon, 2020-11-23 at 10:34 +0000, Luis Henriques wrote: >> For the moment cross quota realms renames has been disabled in CephFS >> after a bug has been found while renaming files created and truncated. >> This allowed clients to easily circumvent quotas. >> >> Link: https://tracker.ceph.com/issues/48203 >> Signed-off-by: Luis Henriques <lhenriques@suse.de> >> --- >> v2: implemented Eryu review comments: >> - Added _require_test_program "rename" >> - Use _fail instead of _fatal >> >> tests/ceph/004 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ >> tests/ceph/004.out | 2 + >> tests/ceph/group | 1 + >> 3 files changed, 98 insertions(+) >> create mode 100755 tests/ceph/004 >> create mode 100644 tests/ceph/004.out >> >> diff --git a/tests/ceph/004 b/tests/ceph/004 >> new file mode 100755 >> index 000000000000..53094d8dfadc >> --- /dev/null >> +++ b/tests/ceph/004 >> @@ -0,0 +1,95 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. >> +# >> +# FS QA Test 004 >> +# >> +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase >> +# that *should* fail: >> +# >> +# mkdir files limit >> +# truncate files/file -s 10G >> +# setfattr limit -n ceph.quota.max_bytes -v 1000000 >> +# mv files limit/ >> +# >> +# Because we're creating a new file and truncating it, we have Fx caps and thus >> +# the truncate operation will be cached. This prevents the MDSs from updating >> +# the quota realms and thus the client will allow the above rename(2) to happen. >> +# > > Note that it can be difficult to predict which caps you get from the > MDS. It's not _required_ to pass out anything like Fx if it doesn't want > to, but in general, it does if it can. > > It's not a blocker for merging this test, but I wonder if we ought to > come up with some way to ensure that the client was given the caps we > expect when testing stuff like this. > > Maybe we ought to consider adding a new ceph.caps vxattr that shows the > caps we hold for a particular file? Then we could consult that when > doing a test like this to make sure we got what we expected. Sure, I can hack a patch for doing that and send it out for review. That's actually trivial, I believe. This test assumes the caps for the truncated file will be 'Fsxcrwb' but I didn't confirm with the MDS which conditions are actually required for this to happen. Also, I guess that if the test is executed with several clients, these caps may change pretty quickly (and maybe even with a single very slow client with a very short caps timeout). Obviously, ensuring the client has the caps we expect at the time we do the actual rename is racy and they can change in the meantime. Is it worth the trouble? Cheers, -- Luis >> +# The bug resulted in dropping support for cross quota-realms renames, reverting >> +# kernel commit dffdcd71458e ("ceph: allow rename operation under different >> +# quota realms"). >> +# >> +# So, the above test will now fail with a -EXDEV or, in the future (when we have >> +# a proper fix), with -EDQUOT. >> +# >> +# This bug was tracker here: >> +# >> +# https://tracker.ceph.com/issues/48203 >> +# >> +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 >> +. ./common/attr >> + >> +# remove previous $seqres.full before test >> +rm -f $seqres.full >> + >> +# real QA test starts here >> + >> +_supported_fs ceph >> +_require_attrs >> +_require_test >> +_require_test_program "rename" >> + >> +workdir=$TEST_DIR/test-$seq >> + >> +orig1=$workdir/orig1 >> +orig2=$workdir/orig2 >> +file1=$orig1/file >> +file2=$orig2/file >> +dest=$workdir/dest >> + >> +rm -rf $workdir >> +mkdir $workdir >> +mkdir $orig1 $orig2 $dest >> + >> +# set quota to 1m >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v 1000000 $dest >> +# set quota to 20g >> +$SETFATTR_PROG -n ceph.quota.max_bytes -v 20000000000 $orig2 >> + >> +# >> +# The following 2 testcases shall fail with either -EXDEV or -EDQUOT >> +# >> + >> +# from 'root' realm to $dest realm >> +$XFS_IO_PROG -f -c "truncate 10G" $file1 >> +$here/src/rename $orig1 $dest/new1 >> $seqres.full 2>&1 >> +[ $? -ne 1 ] && _fail "cross quota realms rename succeeded" >> + >> +# from $orig2 realm to $dest realm >> +$XFS_IO_PROG -f -c "truncate 10G" $file2 >> +$here/src/rename $orig2 $dest/new2 >> $seqres.full 2>&1 >> +[ $? -ne 1 ] && _fail "cross quota realms rename succeeded" >> + >> +echo "Silence is golden" >> + >> +# success, all done >> +status=0 >> +exit >> diff --git a/tests/ceph/004.out b/tests/ceph/004.out >> new file mode 100644 >> index 000000000000..af8614ae45ac >> --- /dev/null >> +++ b/tests/ceph/004.out >> @@ -0,0 +1,2 @@ >> +QA output created by 004 >> +Silence is golden >> diff --git a/tests/ceph/group b/tests/ceph/group >> index adbf61547766..47903d21966c 100644 >> --- a/tests/ceph/group >> +++ b/tests/ceph/group >> @@ -1,3 +1,4 @@ >> 001 auto quick copy >> 002 auto quick copy >> 003 auto quick copy >> +004 auto quick quota > > This looks good to me. > > Reviewed-by: Jeff Layton <jlayton@kernel.org> >
On Mon, 2020-11-23 at 14:43 +0000, Luis Henriques wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > On Mon, 2020-11-23 at 10:34 +0000, Luis Henriques wrote: > > > For the moment cross quota realms renames has been disabled in CephFS > > > after a bug has been found while renaming files created and truncated. > > > This allowed clients to easily circumvent quotas. > > > > > > Link: https://tracker.ceph.com/issues/48203 > > > Signed-off-by: Luis Henriques <lhenriques@suse.de> > > > --- > > > v2: implemented Eryu review comments: > > > - Added _require_test_program "rename" > > > - Use _fail instead of _fatal > > > > > > tests/ceph/004 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ > > > tests/ceph/004.out | 2 + > > > tests/ceph/group | 1 + > > > 3 files changed, 98 insertions(+) > > > create mode 100755 tests/ceph/004 > > > create mode 100644 tests/ceph/004.out > > > > > > diff --git a/tests/ceph/004 b/tests/ceph/004 > > > new file mode 100755 > > > index 000000000000..53094d8dfadc > > > --- /dev/null > > > +++ b/tests/ceph/004 > > > @@ -0,0 +1,95 @@ > > > +#! /bin/bash > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. > > > +# > > > +# FS QA Test 004 > > > +# > > > +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase > > > +# that *should* fail: > > > +# > > > +# mkdir files limit > > > +# truncate files/file -s 10G > > > +# setfattr limit -n ceph.quota.max_bytes -v 1000000 > > > +# mv files limit/ > > > +# > > > +# Because we're creating a new file and truncating it, we have Fx caps and thus > > > +# the truncate operation will be cached. This prevents the MDSs from updating > > > +# the quota realms and thus the client will allow the above rename(2) to happen. > > > +# > > > > Note that it can be difficult to predict which caps you get from the > > MDS. It's not _required_ to pass out anything like Fx if it doesn't want > > to, but in general, it does if it can. > > > > It's not a blocker for merging this test, but I wonder if we ought to > > come up with some way to ensure that the client was given the caps we > > expect when testing stuff like this. > > > > Maybe we ought to consider adding a new ceph.caps vxattr that shows the > > caps we hold for a particular file? Then we could consult that when > > doing a test like this to make sure we got what we expected. > > Sure, I can hack a patch for doing that and send it out for review. > That's actually trivial, I believe. > > This test assumes the caps for the truncated file will be 'Fsxcrwb' but I > didn't confirm with the MDS which conditions are actually required for > this to happen. Also, I guess that if the test is executed with several > clients, these caps may change pretty quickly (and maybe even with a > single very slow client with a very short caps timeout). > > Obviously, ensuring the client has the caps we expect at the time we do > the actual rename is racy and they can change in the meantime. Is it > worth the trouble? I think it's useful. Cap/mds lock handling is an area where we have really poor visibility in cephfs. a/ It's not always 100% clear what metadata is under which cap. Sometimes it's really weird. For example, you need Fs to get the link count on a directory -- Ls has no meaning there, which is not intuitive at all. b/ Subtle changes in the MDS or client can affect what caps are granted or revoked in a given situation. Having better visibility into the caps held by the client is potentially very useful for troubleshooting _why_ certain tests might fail, and may also help us catch subtle changes that prevent problems in the future. -- Jeff Layton <jlayton@kernel.org>
Jeff Layton <jlayton@kernel.org> writes: > On Mon, 2020-11-23 at 14:43 +0000, Luis Henriques wrote: >> Jeff Layton <jlayton@kernel.org> writes: >> >> > On Mon, 2020-11-23 at 10:34 +0000, Luis Henriques wrote: >> > > For the moment cross quota realms renames has been disabled in CephFS >> > > after a bug has been found while renaming files created and truncated. >> > > This allowed clients to easily circumvent quotas. >> > > >> > > Link: https://tracker.ceph.com/issues/48203 >> > > Signed-off-by: Luis Henriques <lhenriques@suse.de> >> > > --- >> > > v2: implemented Eryu review comments: >> > > - Added _require_test_program "rename" >> > > - Use _fail instead of _fatal >> > > >> > > tests/ceph/004 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ >> > > tests/ceph/004.out | 2 + >> > > tests/ceph/group | 1 + >> > > 3 files changed, 98 insertions(+) >> > > create mode 100755 tests/ceph/004 >> > > create mode 100644 tests/ceph/004.out >> > > >> > > diff --git a/tests/ceph/004 b/tests/ceph/004 >> > > new file mode 100755 >> > > index 000000000000..53094d8dfadc >> > > --- /dev/null >> > > +++ b/tests/ceph/004 >> > > @@ -0,0 +1,95 @@ >> > > +#! /bin/bash >> > > +# SPDX-License-Identifier: GPL-2.0 >> > > +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. >> > > +# >> > > +# FS QA Test 004 >> > > +# >> > > +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase >> > > +# that *should* fail: >> > > +# >> > > +# mkdir files limit >> > > +# truncate files/file -s 10G >> > > +# setfattr limit -n ceph.quota.max_bytes -v 1000000 >> > > +# mv files limit/ >> > > +# >> > > +# Because we're creating a new file and truncating it, we have Fx caps and thus >> > > +# the truncate operation will be cached. This prevents the MDSs from updating >> > > +# the quota realms and thus the client will allow the above rename(2) to happen. >> > > +# >> > >> > Note that it can be difficult to predict which caps you get from the >> > MDS. It's not _required_ to pass out anything like Fx if it doesn't want >> > to, but in general, it does if it can. >> > >> > It's not a blocker for merging this test, but I wonder if we ought to >> > come up with some way to ensure that the client was given the caps we >> > expect when testing stuff like this. >> > >> > Maybe we ought to consider adding a new ceph.caps vxattr that shows the >> > caps we hold for a particular file? Then we could consult that when >> > doing a test like this to make sure we got what we expected. >> >> Sure, I can hack a patch for doing that and send it out for review. >> That's actually trivial, I believe. >> >> This test assumes the caps for the truncated file will be 'Fsxcrwb' but I >> didn't confirm with the MDS which conditions are actually required for >> this to happen. Also, I guess that if the test is executed with several >> clients, these caps may change pretty quickly (and maybe even with a >> single very slow client with a very short caps timeout). >> >> Obviously, ensuring the client has the caps we expect at the time we do >> the actual rename is racy and they can change in the meantime. Is it >> worth the trouble? > > > I think it's useful. Cap/mds lock handling is an area where we have > really poor visibility in cephfs. > > a/ It's not always 100% clear what metadata is under which cap. > Sometimes it's really weird. For example, you need Fs to get the link > count on a directory -- Ls has no meaning there, which is not intuitive > at all. > > b/ Subtle changes in the MDS or client can affect what caps are granted > or revoked in a given situation. > > Having better visibility into the caps held by the client is potentially > very useful for troubleshooting _why_ certain tests might fail, and may > also help us catch subtle changes that prevent problems in the future. Sure, I completely agree with this. My question was more about adding an extra check to the test. Basically, the new test will be something like: (0. ensure 'getfattr -n ceph.caps' works; skip test if it doesn't) 1. truncate file 2. check that file caps includes Fsxcrwb 3. do the rename I'll send out v3 if that's what you had in mind. Cheers, -- Luis
On Mon, 2020-11-23 at 16:24 +0000, Luis Henriques wrote: > Jeff Layton <jlayton@kernel.org> writes: > > > On Mon, 2020-11-23 at 14:43 +0000, Luis Henriques wrote: > > > Jeff Layton <jlayton@kernel.org> writes: > > > > > > > On Mon, 2020-11-23 at 10:34 +0000, Luis Henriques wrote: > > > > > For the moment cross quota realms renames has been disabled in CephFS > > > > > after a bug has been found while renaming files created and truncated. > > > > > This allowed clients to easily circumvent quotas. > > > > > > > > > > Link: https://tracker.ceph.com/issues/48203 > > > > > Signed-off-by: Luis Henriques <lhenriques@suse.de> > > > > > --- > > > > > v2: implemented Eryu review comments: > > > > > - Added _require_test_program "rename" > > > > > - Use _fail instead of _fatal > > > > > > > > > > tests/ceph/004 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/ceph/004.out | 2 + > > > > > tests/ceph/group | 1 + > > > > > 3 files changed, 98 insertions(+) > > > > > create mode 100755 tests/ceph/004 > > > > > create mode 100644 tests/ceph/004.out > > > > > > > > > > diff --git a/tests/ceph/004 b/tests/ceph/004 > > > > > new file mode 100755 > > > > > index 000000000000..53094d8dfadc > > > > > --- /dev/null > > > > > +++ b/tests/ceph/004 > > > > > @@ -0,0 +1,95 @@ > > > > > +#! /bin/bash > > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > > +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. > > > > > +# > > > > > +# FS QA Test 004 > > > > > +# > > > > > +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase > > > > > +# that *should* fail: > > > > > +# > > > > > +# mkdir files limit > > > > > +# truncate files/file -s 10G > > > > > +# setfattr limit -n ceph.quota.max_bytes -v 1000000 > > > > > +# mv files limit/ > > > > > +# > > > > > +# Because we're creating a new file and truncating it, we have Fx caps and thus > > > > > +# the truncate operation will be cached. This prevents the MDSs from updating > > > > > +# the quota realms and thus the client will allow the above rename(2) to happen. > > > > > +# > > > > > > > > Note that it can be difficult to predict which caps you get from the > > > > MDS. It's not _required_ to pass out anything like Fx if it doesn't want > > > > to, but in general, it does if it can. > > > > > > > > It's not a blocker for merging this test, but I wonder if we ought to > > > > come up with some way to ensure that the client was given the caps we > > > > expect when testing stuff like this. > > > > > > > > Maybe we ought to consider adding a new ceph.caps vxattr that shows the > > > > caps we hold for a particular file? Then we could consult that when > > > > doing a test like this to make sure we got what we expected. > > > > > > Sure, I can hack a patch for doing that and send it out for review. > > > That's actually trivial, I believe. > > > > > > This test assumes the caps for the truncated file will be 'Fsxcrwb' but I > > > didn't confirm with the MDS which conditions are actually required for > > > this to happen. Also, I guess that if the test is executed with several > > > clients, these caps may change pretty quickly (and maybe even with a > > > single very slow client with a very short caps timeout). > > > > > > Obviously, ensuring the client has the caps we expect at the time we do > > > the actual rename is racy and they can change in the meantime. Is it > > > worth the trouble? > > > > > > I think it's useful. Cap/mds lock handling is an area where we have > > really poor visibility in cephfs. > > > > a/ It's not always 100% clear what metadata is under which cap. > > Sometimes it's really weird. For example, you need Fs to get the link > > count on a directory -- Ls has no meaning there, which is not intuitive > > at all. > > > > b/ Subtle changes in the MDS or client can affect what caps are granted > > or revoked in a given situation. > > > > Having better visibility into the caps held by the client is potentially > > very useful for troubleshooting _why_ certain tests might fail, and may > > also help us catch subtle changes that prevent problems in the future. > > Sure, I completely agree with this. My question was more about adding an > extra check to the test. Basically, the new test will be something like: > > (0. ensure 'getfattr -n ceph.caps' works; skip test if it doesn't) > 1. truncate file > 2. check that file caps includes Fsxcrwb > 3. do the rename > Sounds reasonable. You may not even need to test for that whole cap set either. For this test, you probably just need to ensure that it got Fs. I'd be a little leery about failing the test if we got a different set of caps that still happened to contain Fs. -- Jeff Layton <jlayton@kernel.org>
Jeff Layton <jlayton@kernel.org> writes: > On Mon, 2020-11-23 at 16:24 +0000, Luis Henriques wrote: >> Jeff Layton <jlayton@kernel.org> writes: >> >> > On Mon, 2020-11-23 at 14:43 +0000, Luis Henriques wrote: >> > > Jeff Layton <jlayton@kernel.org> writes: >> > > >> > > > On Mon, 2020-11-23 at 10:34 +0000, Luis Henriques wrote: >> > > > > For the moment cross quota realms renames has been disabled in CephFS >> > > > > after a bug has been found while renaming files created and truncated. >> > > > > This allowed clients to easily circumvent quotas. >> > > > > >> > > > > Link: https://tracker.ceph.com/issues/48203 >> > > > > Signed-off-by: Luis Henriques <lhenriques@suse.de> >> > > > > --- >> > > > > v2: implemented Eryu review comments: >> > > > > - Added _require_test_program "rename" >> > > > > - Use _fail instead of _fatal >> > > > > >> > > > > tests/ceph/004 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ >> > > > > tests/ceph/004.out | 2 + >> > > > > tests/ceph/group | 1 + >> > > > > 3 files changed, 98 insertions(+) >> > > > > create mode 100755 tests/ceph/004 >> > > > > create mode 100644 tests/ceph/004.out >> > > > > >> > > > > diff --git a/tests/ceph/004 b/tests/ceph/004 >> > > > > new file mode 100755 >> > > > > index 000000000000..53094d8dfadc >> > > > > --- /dev/null >> > > > > +++ b/tests/ceph/004 >> > > > > @@ -0,0 +1,95 @@ >> > > > > +#! /bin/bash >> > > > > +# SPDX-License-Identifier: GPL-2.0 >> > > > > +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. >> > > > > +# >> > > > > +# FS QA Test 004 >> > > > > +# >> > > > > +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase >> > > > > +# that *should* fail: >> > > > > +# >> > > > > +# mkdir files limit >> > > > > +# truncate files/file -s 10G >> > > > > +# setfattr limit -n ceph.quota.max_bytes -v 1000000 >> > > > > +# mv files limit/ >> > > > > +# >> > > > > +# Because we're creating a new file and truncating it, we have Fx caps and thus >> > > > > +# the truncate operation will be cached. This prevents the MDSs from updating >> > > > > +# the quota realms and thus the client will allow the above rename(2) to happen. >> > > > > +# >> > > > >> > > > Note that it can be difficult to predict which caps you get from the >> > > > MDS. It's not _required_ to pass out anything like Fx if it doesn't want >> > > > to, but in general, it does if it can. >> > > > >> > > > It's not a blocker for merging this test, but I wonder if we ought to >> > > > come up with some way to ensure that the client was given the caps we >> > > > expect when testing stuff like this. >> > > > >> > > > Maybe we ought to consider adding a new ceph.caps vxattr that shows the >> > > > caps we hold for a particular file? Then we could consult that when >> > > > doing a test like this to make sure we got what we expected. >> > > >> > > Sure, I can hack a patch for doing that and send it out for review. >> > > That's actually trivial, I believe. >> > > >> > > This test assumes the caps for the truncated file will be 'Fsxcrwb' but I >> > > didn't confirm with the MDS which conditions are actually required for >> > > this to happen. Also, I guess that if the test is executed with several >> > > clients, these caps may change pretty quickly (and maybe even with a >> > > single very slow client with a very short caps timeout). >> > > >> > > Obviously, ensuring the client has the caps we expect at the time we do >> > > the actual rename is racy and they can change in the meantime. Is it >> > > worth the trouble? >> > >> > >> > I think it's useful. Cap/mds lock handling is an area where we have >> > really poor visibility in cephfs. >> > >> > a/ It's not always 100% clear what metadata is under which cap. >> > Sometimes it's really weird. For example, you need Fs to get the link >> > count on a directory -- Ls has no meaning there, which is not intuitive >> > at all. >> > >> > b/ Subtle changes in the MDS or client can affect what caps are granted >> > or revoked in a given situation. >> > >> > Having better visibility into the caps held by the client is potentially >> > very useful for troubleshooting _why_ certain tests might fail, and may >> > also help us catch subtle changes that prevent problems in the future. >> >> Sure, I completely agree with this. My question was more about adding an >> extra check to the test. Basically, the new test will be something like: >> >> (0. ensure 'getfattr -n ceph.caps' works; skip test if it doesn't) >> 1. truncate file >> 2. check that file caps includes Fsxcrwb >> 3. do the rename >> > > Sounds reasonable. You may not even need to test for that whole cap set > either. For this test, you probably just need to ensure that it got Fs. > I'd be a little leery about failing the test if we got a different set > of caps that still happened to contain Fs. Great, thanks for the feedback. I'll re-submit this test once we have a patch for the new vxattr ready. Cheers, -- Luis
diff --git a/tests/ceph/004 b/tests/ceph/004 new file mode 100755 index 000000000000..4021666b138e --- /dev/null +++ b/tests/ceph/004 @@ -0,0 +1,94 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2020 SUSE Linux Products GmbH. All Rights Reserved. +# +# FS QA Test 004 +# +# Tests a bug fix found in cephfs quotas handling. Here's a simplified testcase +# that *should* fail: +# +# mkdir files limit +# truncate files/file -s 10G +# setfattr limit -n ceph.quota.max_bytes -v 1000000 +# mv files limit/ +# +# Because we're creating a new file and truncating it, we have Fx caps and thus +# the truncate operation will be cached. This prevents the MDSs from updating +# the quota realms and thus the client will allow the above rename(2) to happen. +# +# The bug resulted in dropping support for cross quota-realms renames, reverting +# kernel commit dffdcd71458e ("ceph: allow rename operation under different +# quota realms"). +# +# So, the above test will now fail with a -EXDEV or, in the future (when we have +# a proper fix), with -EDQUOT. +# +# This bug was tracker here: +# +# https://tracker.ceph.com/issues/48203 +# +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 +. ./common/attr + +# remove previous $seqres.full before test +rm -f $seqres.full + +# real QA test starts here + +_supported_fs ceph +_require_attrs +_require_test + +workdir=$TEST_DIR/test-$seq + +orig1=$workdir/orig1 +orig2=$workdir/orig2 +file1=$orig1/file +file2=$orig2/file +dest=$workdir/dest + +rm -rf $workdir +mkdir $workdir +mkdir $orig1 $orig2 $dest + +# set quota to 1m +$SETFATTR_PROG -n ceph.quota.max_bytes -v 1000000 $dest +# set quota to 20g +$SETFATTR_PROG -n ceph.quota.max_bytes -v 20000000000 $orig2 + +# +# The following 2 testcases shall fail with either -EXDEV or -EDQUOT +# + +# from 'root' realm to $dest realm +$XFS_IO_PROG -f -c "truncate 10G" $file1 +$here/src/rename $orig1 $dest/new1 >> $seqres.full 2>&1 +[ $? -ne 1 ] && _fatal "cross quota realms rename succeeded" + +# from $orig2 realm to $dest realm +$XFS_IO_PROG -f -c "truncate 10G" $file2 +$here/src/rename $orig2 $dest/new2 >> $seqres.full 2>&1 +[ $? -ne 1 ] && _fatal "cross quota realms rename succeeded" + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/ceph/004.out b/tests/ceph/004.out new file mode 100644 index 000000000000..af8614ae45ac --- /dev/null +++ b/tests/ceph/004.out @@ -0,0 +1,2 @@ +QA output created by 004 +Silence is golden diff --git a/tests/ceph/group b/tests/ceph/group index adbf61547766..47903d21966c 100644 --- a/tests/ceph/group +++ b/tests/ceph/group @@ -1,3 +1,4 @@ 001 auto quick copy 002 auto quick copy 003 auto quick copy +004 auto quick quota
For the moment cross quota realms renames has been disabled in CephFS after a bug has been found while renaming files created and truncated. This allowed clients to easily circumvent quotas. Link: https://tracker.ceph.com/issues/48203 Signed-off-by: Luis Henriques <lhenriques@suse.de> --- tests/ceph/004 | 94 ++++++++++++++++++++++++++++++++++++++++++++++ tests/ceph/004.out | 2 + tests/ceph/group | 1 + 3 files changed, 97 insertions(+) create mode 100755 tests/ceph/004 create mode 100644 tests/ceph/004.out