On Wed, Dec 14, 2022 at 03:12:01PM -0800, Boris Burkov wrote:
> If we create a file that has an inline extent followed by a prealloc
> extent, then attempt to use the logical to inode ioctl on the prealloc
> extent, but in the overwritten range, backref resolution will process
> the inline extent. Depending on the leaf eb layout, this can panic.
> Add a new test for this condition. In the long run, we can add spew when
> we read out-of-bounds fields of inline extent items and simplify this
> test to look for dmesg warnings rather than trying to force a fairly
> fragile panic (dependent on non-standardized details of leaf layout).
>
> The test causes a kernel panic unless:
> btrfs: fix logical_ino ioctl panic
Hi,
Thanks for this new case.
Is this patch merged, does it has a commit id? BTW, please mark this
known issue by _fixed_by_kernel_commit in case source code.
> is applied to the kernel.
>
> Signed-off-by: Boris Burkov <boris@xxxxxx>
> ---
> tests/btrfs/279 | 95 +++++++++++++++++++++++++++++++++++++++++++++
> tests/btrfs/279.out | 2 +
> 2 files changed, 97 insertions(+)
> create mode 100755 tests/btrfs/279
> create mode 100644 tests/btrfs/279.out
>
> diff --git a/tests/btrfs/279 b/tests/btrfs/279
> new file mode 100755
> index 00000000..ef77f84b
> --- /dev/null
> +++ b/tests/btrfs/279
btrfs/279 was just token last weekend, please help to rebase to the latest
fstests for-next branch, or change the 279 to a big enough number which won't
cause conflict (I'll give it a proper number when merge it).
> @@ -0,0 +1,95 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Meta Platforms, Inc. All Rights Reserved.
> +#
> +# FS QA Test 279
> +#
> +# Given a file with extents:
> +# [0 : 4096) (inline)
> +# [4096 : N] (prealloc)
> +# if a user uses the ioctl BTRFS_IOC_LOGICAL_INO[_V2] asking for the file of the
> +# non-inline extent, it results in reading the offset field of the inline
> +# extent, which is meaningless (it is full of user data..). If we are
> +# particularly lucky, it can be past the end of the extent buffer, resulting in
> +# a crash.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/dmlogwrites
Does this case really use helpers in above two included files?
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
> +_require_xfs_io_command "falloc"
Add this case to prealloc or preallocrw group
> +_require_xfs_io_command "fsync"
> +_require_xfs_io_command "pwrite"
> +
> +dump_tree() {
> + $BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV
_require_btrfs_command inspect-internal dump-tree
> +}
> +
> +get_extent_data() {
> + local ino=$1
> + dump_tree $SCRATCH_DEV | grep -A4 "($ino EXTENT_DATA "
> +}
> +
> +get_prealloc_offset() {
> + local ino=$1
> + get_extent_data $ino | grep "disk byte" | awk '{print $5}'
> +}
> +
> +# This test needs to create conditions s.t. the special inode's inline extent
> +# is the first item in a leaf. Therefore, fix a leaf size and add
> +# items that are otherwise not necessary to reproduce the inline-prealloc
> +# condition to get to such a state.
> +#
> +# Roughly, the idea for getting the right item fill is to:
> +# 1. create an extra file with a variable sized inline extent item
> +# 2. create our evil file that will cause the panic
> +# 3. create a whole bunch of files to create a bunch of dir/index items
> +# 4. size the variable extent item s.t. the evil extent item is item 0 in the
> +# next leaf
> +#
> +# We do it in this somewhat convoluted way because the dir and index items all
> +# come before any inode, inode_ref, or extent_data items. So we can predictably
> +# create a bunch of them, then sneak in a funny sized extent to round out the
> +# difference.
> +
> +_scratch_mkfs "--nodesize 16k" >/dev/null
We recommend calling _fail at here if _scratch_mkfs fails with a *specified*
option.
_scratch_mkfs "--nodesize 16k" >>$seqres.full || _fail "mkfs failed"
> +_scratch_mount
> +
> +f=$SCRATCH_MNT/f
> +
> +# the variable extra "leaf padding" file
> +$XFS_IO_PROG -fc "pwrite -q 0 700" $f.pad
> +
> +# the evil file with an inline extent followed by a prealloc extent
> +# created by falloc with keep-size, then two non-truncating writes to the front
> +touch $f.evil
Not sure if you reall use this touch command as you've used "-f" option for
xfs_io.
> +$XFS_IO_PROG -fc "falloc -k 0 1m" $f.evil
> +$XFS_IO_PROG -fc fsync $f.evil
> +ino=$(stat -c '%i' $f.evil)
> +logical=$(get_prealloc_offset $ino)
> +$XFS_IO_PROG -fc "pwrite -q 0 23" $f.evil
> +$XFS_IO_PROG -fc fsync $f.evil
> +$XFS_IO_PROG -fc "pwrite -q 0 23" $f.evil
> +$XFS_IO_PROG -fc fsync $f.evil
xfs_io commands can be combined in one line, to make the code looks clear (except
you hope the fd closed after each command done) E.g:
$XFS_IO_PROG -fc "falloc -k 0 1m" -c fsync $f.evil
ino=$(stat -c '%i' $f.evil)
logical=$(get_prealloc_offset $ino)
$XFS_IO_PROG -c "pwrite -q 0 23" -c fsync \
-c "pwrite -q 0 23" -c fsync \
$f.evil
> +sync
> +
> +# a bunch of inodes to stuff dir items in front of the extent items
> +for i in $(seq 122); do
> + $XFS_IO_PROG -fc "pwrite -q 0 8192" $f.$i
> +done
> +sync
> +
> +btrfs inspect-internal logical-resolve $logical $SCRATCH_MNT | _filter_scratch
^^
$BTRFS_UTIL_PROG
_require_btrfs_command inspect-internal logical-resolve
The .out file only has "Silence is golden", what's this _filter_scratch used for?
> +_scratch_unmount
Is the ending _scratch_unmount useful?
> +
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/btrfs/279.out b/tests/btrfs/279.out
> new file mode 100644
> index 00000000..c5906093
> --- /dev/null
> +++ b/tests/btrfs/279.out
> @@ -0,0 +1,2 @@
> +QA output created by 279
> +Silence is golden
> --
> 2.38.1
>
