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

[Filipe Manana <fdmanana@gmail.com>]

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