On Sat, Jan 27, 2024 at 8:44 PM Qu Wenruo <wqu@xxxxxxxx> wrote:
>
> [BUG]
> There is a report about reading a zstd compressed inline file extent
> would lead to either a VM_BUG_ON() crash, or lead to incorrect file
> content.
>
> [CAUSE]
> The root cause is a incorrect memcpy_to_page() call, which uses
> incorrect page offset, and can lead to either the VM_BUG_ON() as we may
> write beyond the page boundary, or writes into the incorrect offset of
> the page.
>
> [TEST CASE]
> The test case would:
>
> - Mount with the specified compress algorithm
> - Create a 4K file
> - Verify the 4K file is all inlined and compressed
> - Verify the content of the initial write
> - Cycle mount to drop all the page cache
> - Verify the content of the file again
> - Unmount and fsck the fs
>
> This workload would be applied to all supported compression algorithms.
> And it can catch the problem correctly by triggering VM_BUG_ON(), as our
> workload would result decompressed extent size to be 4K, and would
> trigger the VM_BUG_ON() 100%.
> And with the revert or the new fix, the test case can pass safely.
>
> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> ---
> tests/btrfs/310 | 83 +++++++++++++++++++++++++++++++++++++++++++++
> tests/btrfs/310.out | 2 ++
> 2 files changed, 85 insertions(+)
> create mode 100755 tests/btrfs/310
> create mode 100644 tests/btrfs/310.out
> ---
> Changelog:
> v2:
> - Add a comment on why a "sync" is needed
> - Update the failure case comment
> The specific design of the inline extent size is ensured to trigger
> VM_BUG_ON(), thus remove the data corruption case.
>
> diff --git a/tests/btrfs/310 b/tests/btrfs/310
> new file mode 100755
> index 00000000..507485a4
> --- /dev/null
> +++ b/tests/btrfs/310
> @@ -0,0 +1,83 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2024 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 310
> +#
> +# Make sure reading on an compressed inline extent is behaving correctly
> +#
> +. ./common/preamble
> +_begin_fstest auto quick compress
> +
> +# Import common functions.
> +# . ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
> +
> +# This test require inlined compressed extents creation, and all the writes
> +# are designed for 4K sector size.
> +_require_btrfs_inline_extents_creation
> +_require_btrfs_support_sectorsize 4096
> +
> +_fixed_by_kernel_commit e01a83e12604 \
> + "Revert \"btrfs: zstd: fix and simplify the inline extent decompression\""
> +
I assume that this will be updated once we land a fixed commit? We
should ensure we keep track of those things...
> +# The correct md5 for the correct 4K file filled with "0xcd"
> +md5sum_correct="5fed275e7617a806f94c173746a2a723"
> +
> +workload()
> +{
> + local algo="$1"
> +
> + echo "=== Testing compression algorithm ${algo} ===" >> $seqres.full
> + _scratch_mkfs >> $seqres.full
> + _scratch_mount -o compress=${algo}
> +
> + _pwrite_byte 0xcd 0 4k "$SCRATCH_MNT/inline_file" > /dev/null
> + result=$(_md5_checksum "$SCRATCH_MNT/inline_file")
> + echo "after initial write, md5sum=${result}" >> $seqres.full
> + if [ "$result" != "$md5sum_correct" ]; then
> + _fail "initial write results incorrect content for \"$algo\""
> + fi
> + # Writeback data to get correct fiemap result, or we got FIEMAP_DEALLOC
> + # without compression/inline flags.
> + sync
> +
> + $XFS_IO_PROG -c "fiemap -v" $SCRATCH_MNT/inline_file | tail -n 1 > $tmp.fiemap
> + cat $tmp.fiemap >> $seqres.full
> + # Make sure we got an inlined compressed file extent.
> + # 0x200 means inlined, 0x100 means not block aligned, 0x8 means encoded
> + # (compressed in this case), and 0x1 means the last extent.
> + if ! grep -q "0x309" $tmp.fiemap; then
> + rm -f -- $tmp.fiemap
> + _notrun "No compressed inline extent created, maybe subpage?"
> + fi
> + rm -f -- $tmp.fiemap
> +
> + # Unmount to clear the page cache.
> + _scratch_cycle_mount
> +
> + # For v6.8-rc1 without the revert or the newer fix, this would
> + # lead to VM_BUG_ON() thus crash
> + result=$(_md5_checksum "$SCRATCH_MNT/inline_file")
> + echo "after cycle mount, md5sum=${result}" >> $seqres.full
> + if [ "$result" != "$md5sum_correct" ]; then
> + _fail "read for compressed inline extent failed for \"$algo\""
> + fi
> + _scratch_unmount
> + _check_scratch_fs
> +}
> +
> +algo_list=($(_btrfs_compression_algos))
> +for algo in ${algo_list[@]}; do
> + workload $algo
> +done
> +
> +echo "Silence is golden"
> +
> +status=0
> +exit
> diff --git a/tests/btrfs/310.out b/tests/btrfs/310.out
> new file mode 100644
> index 00000000..7b9eaf78
> --- /dev/null
> +++ b/tests/btrfs/310.out
> @@ -0,0 +1,2 @@
> +QA output created by 310
> +Silence is golden
> --
> 2.42.0
>
>
Otherwise, LGTM.
Reviewed-by: Neal Gompa <neal@xxxxxxxxx>
--
真実はいつも一つ！/ Always, there's only one truth!
