Re: [PATCH] fstests: btrfs/244: add test case to make sure kernel won't crash when deleting non-existing device

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/8/6 下午7:13, Filipe Manana wrote:
> On Fri, Aug 6, 2021 at 11:47 AM 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
>>
>> The fix is titled "btrfs: fix NULL pointer dereference when deleting
>> device by invalid id".
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   tests/btrfs/244     | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/244.out |  2 ++
>>   2 files changed, 44 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..56eb9e8c
>> --- /dev/null
>> +++ b/tests/btrfs/244
>> @@ -0,0 +1,42 @@
>> +#! /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.
>> +# We don't care about the failure itself, but care whether this would cause
>> +# kernel crash.
>> +$BTRFS_UTIL_PROG device remove 3 $SCRATCH_MNT >> $seqres.full 2>&1
>
> While here we could also check that the operation fails. That avoids
> adding yet another test case if one day we have a regression where the
> operation returns success instead of an error.

That's exactly what I also thought.

But there comes a small problem, the output format may change, thus in
the future the test case may cause false alert.

>
> In fact the test subject and goal should be to verify that removing a
> non-existing device fails and does not cause any harm (kernel crash,
> metadata corruption, etc).

Right, I'll just include the error output into the golden output, so
that we can verify everything, including the expected failure.

For the possible future format change, let the future guys to add a
filter to handle it then.

Thanks,
Qu

>
> Thanks.
>
>> +
>> +echo "Silence is golden"
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/244.out b/tests/btrfs/244.out
>> new file mode 100644
>> index 00000000..440da1f2
>> --- /dev/null
>> +++ b/tests/btrfs/244.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 244
>> +Silence is golden
>> --
>> 2.31.1
>>
>
>