| Message ID | 20210806113333.328261-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device | expand |
On 6.08.21 г. 14:33, Qu Wenruo wrote: > There is a kernel regression for btrfs, that when passing non-existing > devid to "btrfs device remove" command, kernel will crash due to NULL > pointer dereference. > > The test case is for such regression, it will: > > - Create and mount an empty single-device btrfs > - Try to remove devid 3, which doesn't exist for above fs > - Make sure the command exits properly with expected error message > > The kernel fix is titled "btrfs: fix NULL pointer dereference when > deleting device by invalid id". > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Changelog: > v2: > - Change the subject to also verify the error behavior > - Include the error message into golden output > - Also verify the return value of btrfs command > --- > tests/btrfs/244 | 47 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/244.out | 2 ++ > 2 files changed, 49 insertions(+) > create mode 100755 tests/btrfs/244 > create mode 100644 tests/btrfs/244.out > > diff --git a/tests/btrfs/244 b/tests/btrfs/244 > new file mode 100755 > index 00000000..fbefeedf > --- /dev/null > +++ b/tests/btrfs/244 > @@ -0,0 +1,47 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 244 > +# > +# Make sure "btrfs device remove" won't crash when non-existing devid > +# is provided > +# > +. ./common/preamble > +_begin_fstest auto quick volume dangerous > + > +# Override the default cleanup function. > +# _cleanup() > +# { > +# cd / > +# rm -r -f $tmp.* > +# } > + > +# Import common functions. > +# . ./common/filter > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs btrfs > +_require_scratch > + > +_scratch_mkfs >> $seqres.full 2>&1 > +_scratch_mount > + > +# Above created fs only contains one device with devid 1, device remove 3 > +# should just fail with proper error message showing devid 3 can't be found. > +# Although on unpatched kernel, this will trigger a NULL pointer dereference. > +$BTRFS_UTIL_PROG device remove 3 $SCRATCH_MNT > +ret=$? > + > +if [ $ret -ne 1 ]; then > + echo "Unexpected return value from btrfs command, has $ret expected 1" > +fi <rant> This just shows how broken progs are w.r.t return values. The generally accepted return value is 0 on success, yet it returns 1 on success since the functions implementing this functionality in progs treat the return value as a boolean. </rant> > + > +# Fstests will automatically check the filesystem to make sure metadata is not > +# corrupted. > + > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/244.out b/tests/btrfs/244.out > new file mode 100644 > index 00000000..629adf2a > --- /dev/null > +++ b/tests/btrfs/244.out > @@ -0,0 +1,2 @@ > +QA output created by 244 > +ERROR: error removing devid 3: No such file or directory >
On 2021/8/6 下午7:42, Nikolay Borisov wrote: > > > On 6.08.21 г. 14:33, Qu Wenruo wrote: >> There is a kernel regression for btrfs, that when passing non-existing >> devid to "btrfs device remove" command, kernel will crash due to NULL >> pointer dereference. >> >> The test case is for such regression, it will: >> >> - Create and mount an empty single-device btrfs >> - Try to remove devid 3, which doesn't exist for above fs >> - Make sure the command exits properly with expected error message >> >> The kernel fix is titled "btrfs: fix NULL pointer dereference when >> deleting device by invalid id". >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> Changelog: >> v2: >> - Change the subject to also verify the error behavior >> - Include the error message into golden output >> - Also verify the return value of btrfs command >> --- >> tests/btrfs/244 | 47 +++++++++++++++++++++++++++++++++++++++++++++ >> tests/btrfs/244.out | 2 ++ >> 2 files changed, 49 insertions(+) >> create mode 100755 tests/btrfs/244 >> create mode 100644 tests/btrfs/244.out >> >> diff --git a/tests/btrfs/244 b/tests/btrfs/244 >> new file mode 100755 >> index 00000000..fbefeedf >> --- /dev/null >> +++ b/tests/btrfs/244 >> @@ -0,0 +1,47 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. >> +# >> +# FS QA Test 244 >> +# >> +# Make sure "btrfs device remove" won't crash when non-existing devid >> +# is provided >> +# >> +. ./common/preamble >> +_begin_fstest auto quick volume dangerous >> + >> +# Override the default cleanup function. >> +# _cleanup() >> +# { >> +# cd / >> +# rm -r -f $tmp.* >> +# } >> + >> +# Import common functions. >> +# . ./common/filter >> + >> +# real QA test starts here >> + >> +# Modify as appropriate. >> +_supported_fs btrfs >> +_require_scratch >> + >> +_scratch_mkfs >> $seqres.full 2>&1 >> +_scratch_mount >> + >> +# Above created fs only contains one device with devid 1, device remove 3 >> +# should just fail with proper error message showing devid 3 can't be found. >> +# Although on unpatched kernel, this will trigger a NULL pointer dereference. >> +$BTRFS_UTIL_PROG device remove 3 $SCRATCH_MNT >> +ret=$? >> + >> +if [ $ret -ne 1 ]; then >> + echo "Unexpected return value from btrfs command, has $ret expected 1" >> +fi > > <rant> > This just shows how broken progs are w.r.t return values. The generally > accepted return value is 0 on success, yet it returns 1 on success since > the functions implementing this functionality in progs treat the return > value as a boolean. > </rant> Nope, we're testing a failure case, thus returning 1 is what we expect. Thanks, Qu > > > >> + >> +# Fstests will automatically check the filesystem to make sure metadata is not >> +# corrupted. >> + >> +# success, all done >> +status=0 >> +exit >> diff --git a/tests/btrfs/244.out b/tests/btrfs/244.out >> new file mode 100644 >> index 00000000..629adf2a >> --- /dev/null >> +++ b/tests/btrfs/244.out >> @@ -0,0 +1,2 @@ >> +QA output created by 244 >> +ERROR: error removing devid 3: No such file or directory >>
On Fri, Aug 6, 2021 at 4:44 PM Qu Wenruo <wqu@suse.com> wrote: > > There is a kernel regression for btrfs, that when passing non-existing > devid to "btrfs device remove" command, kernel will crash due to NULL > pointer dereference. > > The test case is for such regression, it will: > > - Create and mount an empty single-device btrfs > - Try to remove devid 3, which doesn't exist for above fs > - Make sure the command exits properly with expected error message > > The kernel fix is titled "btrfs: fix NULL pointer dereference when > deleting device by invalid id". > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > Changelog: > v2: > - Change the subject to also verify the error behavior > - Include the error message into golden output > - Also verify the return value of btrfs command > --- > tests/btrfs/244 | 47 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/244.out | 2 ++ > 2 files changed, 49 insertions(+) > create mode 100755 tests/btrfs/244 > create mode 100644 tests/btrfs/244.out > > diff --git a/tests/btrfs/244 b/tests/btrfs/244 > new file mode 100755 > index 00000000..fbefeedf > --- /dev/null > +++ b/tests/btrfs/244 > @@ -0,0 +1,47 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 244 > +# > +# Make sure "btrfs device remove" won't crash when non-existing devid > +# is provided > +# > +. ./common/preamble > +_begin_fstest auto quick volume dangerous > + > +# Override the default cleanup function. > +# _cleanup() > +# { > +# cd / > +# rm -r -f $tmp.* > +# } > + > +# Import common functions. > +# . ./common/filter > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs btrfs > +_require_scratch > + > +_scratch_mkfs >> $seqres.full 2>&1 > +_scratch_mount > + > +# Above created fs only contains one device with devid 1, device remove 3 > +# should just fail with proper error message showing devid 3 can't be found. > +# Although on unpatched kernel, this will trigger a NULL pointer dereference. > +$BTRFS_UTIL_PROG device remove 3 $SCRATCH_MNT > +ret=$? > + > +if [ $ret -ne 1 ]; then > + echo "Unexpected return value from btrfs command, has $ret expected 1" > +fi I think I would just do "-eq 0" instead, but it's more about a preferred style than anything else, and can always be changed later if needed. Looks good, thanks. Reviewed-by: Filipe Manana <fdmanana@suse.com> > + > +# Fstests will automatically check the filesystem to make sure metadata is not > +# corrupted. > + > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/244.out b/tests/btrfs/244.out > new file mode 100644 > index 00000000..629adf2a > --- /dev/null > +++ b/tests/btrfs/244.out > @@ -0,0 +1,2 @@ > +QA output created by 244 > +ERROR: error removing devid 3: No such file or directory > -- > 2.31.1 >
On Fri, Aug 06, 2021 at 02:42:36PM +0300, Nikolay Borisov wrote: > > +if [ $ret -ne 1 ]; then > > + echo "Unexpected return value from btrfs command, has $ret expected 1" > > +fi > > <rant> > This just shows how broken progs are w.r.t return values. The generally > accepted return value is 0 on success, yet it returns 1 on success since > the functions implementing this functionality in progs treat the return > value as a boolean. > </rant> Heh, not following the common convention of 0 for success, !0 for errors would be crazy and hardly left unnoticed by users.
diff --git a/tests/btrfs/244 b/tests/btrfs/244 new file mode 100755 index 00000000..fbefeedf --- /dev/null +++ b/tests/btrfs/244 @@ -0,0 +1,47 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. +# +# FS QA Test 244 +# +# Make sure "btrfs device remove" won't crash when non-existing devid +# is provided +# +. ./common/preamble +_begin_fstest auto quick volume dangerous + +# Override the default cleanup function. +# _cleanup() +# { +# cd / +# rm -r -f $tmp.* +# } + +# Import common functions. +# . ./common/filter + +# real QA test starts here + +# Modify as appropriate. +_supported_fs btrfs +_require_scratch + +_scratch_mkfs >> $seqres.full 2>&1 +_scratch_mount + +# Above created fs only contains one device with devid 1, device remove 3 +# should just fail with proper error message showing devid 3 can't be found. +# Although on unpatched kernel, this will trigger a NULL pointer dereference. +$BTRFS_UTIL_PROG device remove 3 $SCRATCH_MNT +ret=$? + +if [ $ret -ne 1 ]; then + echo "Unexpected return value from btrfs command, has $ret expected 1" +fi + +# Fstests will automatically check the filesystem to make sure metadata is not +# corrupted. + +# success, all done +status=0 +exit diff --git a/tests/btrfs/244.out b/tests/btrfs/244.out new file mode 100644 index 00000000..629adf2a --- /dev/null +++ b/tests/btrfs/244.out @@ -0,0 +1,2 @@ +QA output created by 244 +ERROR: error removing devid 3: No such file or directory
There is a kernel regression for btrfs, that when passing non-existing devid to "btrfs device remove" command, kernel will crash due to NULL pointer dereference. The test case is for such regression, it will: - Create and mount an empty single-device btrfs - Try to remove devid 3, which doesn't exist for above fs - Make sure the command exits properly with expected error message The kernel fix is titled "btrfs: fix NULL pointer dereference when deleting device by invalid id". Signed-off-by: Qu Wenruo <wqu@suse.com> --- Changelog: v2: - Change the subject to also verify the error behavior - Include the error message into golden output - Also verify the return value of btrfs command --- tests/btrfs/244 | 47 +++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/244.out | 2 ++ 2 files changed, 49 insertions(+) create mode 100755 tests/btrfs/244 create mode 100644 tests/btrfs/244.out