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

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

On 13/5/25 16:57, Qu Wenruo wrote:
> 
> 
> 在 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.
> 

Oh, right — we can use dangerous.

I thought you were still sending v2 with Filipe's review comments?