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

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

在 2025/5/13 18:24, Anand Jain 写道:
> 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.

In that case you can add the test case to dangerous group at merge time.

Thanks,
Qu

> 
> Thanks, Anand
> 
>