[PATCH v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device

[PATCH v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device

[Qu Wenruo]

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
+
+# 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

Re: [PATCH v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device

[Nikolay Borisov]

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
> 

Re: [PATCH v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device

[Qu Wenruo]

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

Re: [PATCH v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device

[Filipe Manana]

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
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device

[David Sterba]

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.