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

Filipe Manana
2025-05-12T07:55:14Z

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