On Wed, Nov 24, 2021 at 12:28 PM Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote:
>
>
>
> On 2021/11/24 19:22, Filipe Manana wrote:
> > On Wed, Nov 24, 2021 at 9:24 AM Qu Wenruo <wqu@xxxxxxxx> wrote:
> >>
> >> Since kernel commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
> >> compatible"), lzo compression no longer respects the max compressed page
> >> limit, and can cause kernel crash.
> >>
> >> The upstream fix is 6f019c0e0193 ("btrfs: fix a out-of-bound access in
> >> copy_compressed_data_to_page()").
> >>
> >> This patch will add such regression test for all possible compress-force
> >> mount options, including lzo, zstd and zlib.
> >>
> >> And since we're here, also make sure the content of the file matches
> >> after a mount cycle.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> >> ---
> >> Changelog:
> >> v2:
> >> - Also test zlib and zstd
> >> - Add file content verification check
> >> ---
> >> tests/btrfs/049 | 56 +++++++++++++++++++++++++++++++++++++++++++++
> >> tests/btrfs/049.out | 6 +++++
> >> 2 files changed, 62 insertions(+)
> >> create mode 100755 tests/btrfs/049
> >>
> >> diff --git a/tests/btrfs/049 b/tests/btrfs/049
> >> new file mode 100755
> >> index 00000000..264e576f
> >> --- /dev/null
> >> +++ b/tests/btrfs/049
> >> @@ -0,0 +1,56 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (C) 2021 SUSE Linux Products GmbH. All Rights Reserved.
> >> +#
> >> +# FS QA Test 049
> >> +#
> >> +# Test if btrfs will crash when using compress-force mount option against
> >> +# incompressible data
> >> +#
> >> +. ./common/preamble
> >> +_begin_fstest auto quick compress dangerous
> >> +
> >> +# Override the default cleanup function.
> >> +_cleanup()
> >> +{
> >> + cd /
> >> + rm -r -f $tmp.*
> >> +}
> >> +
> >> +# Import common functions.
> >> +. ./common/filter
> >> +
> >> +# real QA test starts here
> >> +
> >> +# Modify as appropriate.
> >> +_supported_fs btrfs
> >> +_require_scratch
> >> +
> >> +pagesize=$(get_page_size)
> >> +workload()
> >> +{
> >> + local compression
> >> + compression=$1
> >
> > Could be shorter by doing it in one step:
> >
> > local compression=$1
> >
> >> +
> >> + echo "=== Testing compress-force=$compression ==="
> >> + _scratch_mkfs -s "$pagesize">> $seqres.full
> >> + _scratch_mount -o compress-force="$compression"
> >> + $XFS_IO_PROG -f -c "pwrite -i /dev/urandom 0 $pagesize" \
> >> + "$SCRATCH_MNT/file" > /dev/null
> >> + md5sum "$SCRATCH_MNT/file" > "$tmp.$compression"
> >
> > This doesn't really check if everything we asked to write was actually written.
> > pwrite(2), write(2), etc, return the number of bytes written, which
> > can be less than what we asked for.
> >
> > And using the checksum verification in that way, we are only checking
> > that what we had before unmounting is the same after mounting again.
> > I.e. we are not checking that what was actually written is what we
> > have asked for.
> >
> > We could do something like:
> >
> > data=$(dd count=4096 bs=1 if=/dev/urandom)
> > echo -n "$data" > file
>
> The reason I didn't want to use dd is it doesn't have good enough
> wrapper in fstests.
> (Thus I guess that's also the reason why you use above command to
> workaround it)
>
> If you're really concerned about the block size, it can be easily
> changed using "-b" option of pwrite, to archive the same behavior of the
> dd command.
>
> Furthermore, since we're reading from urandom, isn't it already ensured
> we won't get blocked nor get short read until we're reading very large
> blocks?
I gave dd as an example, but I don't care about dd at all, it can be
xfs_io or anything else.
My whole point was about verifying that what's written to the file is
what we intended to write.
Verifying the checksum is fine when we know exactly what we asked to
write, when the test hard codes what we want to write.
In this case we're asking to write random content, so doing an md5sum
of the file after writing and
then comparing it to what we get after we unmount and mount again, is
not a safe way to test we have the
expected content.
>
> Thus a very basic filter on the pwrite should be enough to make sure we
> really got page sized data written.
It's not just about checking if the size of what was written matches
what we asked to write.
It's all about verifying the written content matches what we asked to
write in the first place (which implicitly ends up verifying the size
as well when using a checksum).
>
> Thanks,
> Qu
>
> >
> > _scratch_cycle_mount
> >
> > check that the the md5sum of file is the same as: echo -n "$data" | md5sum
> >
> > As it is, the test is enough to trigger the original bug, but having
> > such additional checks is more valuable IMO for the long run, and can
> > help prevent other types of regressions too.
> >
> > Thanks Qu.
> >
> >
> >> +
> >> + # When unpatched, compress-force=lzo would crash at data writeback
> >> + _scratch_cycle_mount
> >> +
> >> + # Make sure the content matches
> >> + md5sum -c "$tmp.$compression" | _filter_scratch
> >> + _scratch_unmount
> >> +}
> >> +
> >> +workload lzo
> >> +workload zstd
> >> +workload zlib
> >> +
> >> +# success, all done
> >> +status=0
> >> +exit
> >> diff --git a/tests/btrfs/049.out b/tests/btrfs/049.out
> >> index cb0061b3..258f3c09 100644
> >> --- a/tests/btrfs/049.out
> >> +++ b/tests/btrfs/049.out
> >> @@ -1 +1,7 @@
> >> QA output created by 049
> >> +=== Testing compress-force=lzo ===
> >> +SCRATCH_MNT/file: OK
> >> +=== Testing compress-force=zstd ===
> >> +SCRATCH_MNT/file: OK
> >> +=== Testing compress-force=zlib ===
> >> +SCRATCH_MNT/file: OK
> >> --
> >> 2.34.0
> >>
> >
> >
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
