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

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

[Qu Wenruo]

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
+
+_scratch_unmount
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/336.out b/tests/btrfs/336.out
new file mode 100644
index 00000000..9263628e
--- /dev/null
+++ b/tests/btrfs/336.out
@@ -0,0 +1,2 @@
+QA output created by 336
+Silence is golden
-- 
2.47.1

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

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

Otherwise it looks fine, thanks.

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

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

[Qu Wenruo]

在 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?

Thanks,
Qu


> 
> Otherwise it looks fine, thanks.
> 

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

[Filipe Manana]

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.

Thanks.

>
> Thanks,
> Qu
>
>
> >
> > Otherwise it looks fine, thanks.
> >
>

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

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

Thanks, Anand

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

[Qu Wenruo]

在 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
> 
> 

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

[Anand Jain]

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?

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

[Qu Wenruo]

在 2025/5/13 19:27, Anand Jain 写道:
[...]
>>>
>>> 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?
> 

Well, you reviewed the v2 version and applied, thus I guess it's easier 
to add the group on your side:

https://lore.kernel.org/linux-btrfs/60d18eaa-c6b0-45f3-952a-437abfd25636@oracle.com/

Thanks,
Qu