Re: [PATCH] fstests: btrfs: a new test case to verify scrub and rescue=idatacsums

[Anand Jain <anand.jain@oracle.com>]

On 12/5/25 15:54, Filipe Manana wrote:
> On Mon, May 12, 2025 at 8:51 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>>
>>
>> 在 2025/5/12 17:14, Filipe Manana 写道:
>>> On Mon, May 12, 2025 at 6:26 AM Qu Wenruo <wqu@suse.com> wrote:
>>>>
>>>> There is a kernel bug report that scrub will trigger a NULL pointer
>>>> dereference when rescue=idatacsums mount option is provided.
>>>>
>>>> Add a test case for such situation, to verify kernel can gracefully
>>>> reject scrub when  there is no csum tree.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    tests/btrfs/336     | 32 ++++++++++++++++++++++++++++++++
>>>>    tests/btrfs/336.out |  2 ++
>>>>    2 files changed, 34 insertions(+)
>>>>    create mode 100755 tests/btrfs/336
>>>>    create mode 100644 tests/btrfs/336.out
>>>>
>>>> diff --git a/tests/btrfs/336 b/tests/btrfs/336
>>>> new file mode 100755
>>>> index 00000000..f76a8e21
>>>> --- /dev/null
>>>> +++ b/tests/btrfs/336
>>>> @@ -0,0 +1,32 @@
>>>> +#! /bin/bash
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# Copyright (C) 2025 SUSE Linux Products GmbH. All Rights Reserved.
>>>> +#
>>>> +# FS QA Test 336
>>>> +#
>>>> +# Make sure read-only scrub won't cause NULL pointer dereference with
>>>> +# rescue=idatacsums mount option
>>>> +#
>>>> +. ./common/preamble
>>>> +_begin_fstest auto scrub quick
>>>> +
>>>> +_fixed_by_kernel_commit 6aecd91a5c5b \
>>>> +       "btrfs: avoid NULL pointer dereference if no valid extent tree"
>>>> +
>>>> +_require_scratch
>>>> +_scratch_mkfs >> $seqres.full
>>>> +
>>>> +_try_scratch_mount "-o ro,rescue=ignoredatacsums" > /dev/null 2>&1 ||
>>>> +       _notrun "rescue=ignoredatacsums mount option not supported"
>>>> +
>>>> +# For unpatched kernel this will cause NULL pointer dereference and crash the kernel.
>>>> +# For patched kernel scrub will be gracefully rejected.
>>>> +$BTRFS_UTIL_PROG scrub start -Br $SCRATCH_MNT >> $seqres.full 2>&1
>>>
>>> If the scrub is supposed to fail, as the comment says, then we should
>>> check that it fails.
>>> Right now we're ignoring whether it succeeds or fails.
>>
>> Currently it indeed fails for patched kernel, but I'm not sure if it
>> will keep so in the future.
>>
>> E.g. we can still properly scrub metadata chunks, and for data chunks we
>> may even delayed the csum tree lookup so that if we got an empty data
>> block group, we just do an early exit.
>>
>> Or should I do the failure check, and update the test case when the
>> kernel behavior changes in the future?
> 
> It should check the current expected behaviour, and if that changes
> one day, then update it.
> 
> I always find it terribly confusing when something is called and we
> ignore its stdout/stderr and exit status - it makes one wonder why the
> command is being called, if the author forgot about checking what's
> supposed to happen.

Makes sense.

As there is no way to check if the kernel has commit fix 6aecd91a5c5b
testcase will crash the system. That's a bit concerning.

Thanks, Anand