On Sat, Sep 03, 2022 at 07:12:54PM +0800, Guo Xuenan wrote: > Hi Zorro: > > On 2022/9/3 17:57, Zorro Lang wrote: > > On Sat, Sep 03, 2022 at 11:51:13AM +0800, Guo Xuenan wrote: > > > Hi Zorro： > > > > > > On 2022/9/3 9:39, Zorro Lang wrote: > > > > On Fri, Sep 02, 2022 at 05:40:46PM +0800, Guo Xuenan wrote: > > > > > Test leaf dir allocting new block when bestfree array size > > > > > less than data blocks count, which may lead to UAF. > > > > > > > > > > Signed-off-by: Guo Xuenan <guoxuenan@xxxxxxxxxx> > > > > > --- > > > > > tests/xfs/554 | 48 +++++++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/xfs/554.out | 6 ++++++ > > > > > 2 files changed, 54 insertions(+) > > > > > create mode 100755 tests/xfs/554 > > > > > create mode 100644 tests/xfs/554.out > > > > > > > > > > diff --git a/tests/xfs/554 b/tests/xfs/554 > > > > > new file mode 100755 > > > > > index 00000000..fcf45731 > > > > > --- /dev/null > > > > > +++ b/tests/xfs/554 > > > > > @@ -0,0 +1,48 @@ > > > > > +#! /bin/bash > > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > > +# Copyright (c) 2022 Huawei Limited. All Rights Reserved. > > > > > +# > > > > > +# FS QA Test No. 554 > > > > > +# > > > > > +# Test leaf dir bestfree array size match with dir disk size > > > > Is it for a known bug? known commit id? > > > The bug is being solved and waitting to be reviewed here[v1/v2]. > > > [v1] > > > https://lore.kernel.org/all/20220902094046.3891252-1-guoxuenan@xxxxxxxxxx/ > > > [v2] > > > https://lore.kernel.org/all/20220831121639.3060527-1-guoxuenan@xxxxxxxxxx/ > > > > > +# > > > > > +. ./common/preamble > > > > > +_begin_fstest auto quick > > > > > + > > > > > +# Import common functions. > > > > > +. ./common/populate > > > > > + > > > > > +# real QA test starts here > > > > > +_supported_fs xfs > > > > > +_require_scratch > > > > Do you need V5 xfs? Or v4 is fine? > > > > _require_scratch_xfs_crc ?? > Both v4 and v5 have this problem, OK, that's fine if you can reproduce this bug on both. > > > > > +_require_check_dmesg > > > > > + > > > > > +echo "Format and mount" > > > > > +_scratch_mkfs > $seqres.full 2>&1 > > > > > +_scratch_mount >> $seqres.full 2>&1 > > If _scratch_mount fails, the testing will exit directly, so generally we just > > run _scratch_mount. > OK, you are right. > > > > > + > > > > > +echo "Create and check leaf dir" > > > > > +blksz="$(stat -f -c '%s' "${SCRATCH_MNT}")" > > > > > +dblksz="$($XFS_INFO_PROG "${SCRATCH_DEV}" | grep naming.*bsize | sed -e 's/^.*bsize=//g' -e 's/\([0-9]*\).*$/\1/g')" > > > > Why do you need these two kinds of block size for xfs? And you sometimes > > > > use the former, sometimes use the later? If you'd like to get the xfs data > > > > block size, you can: > > > > > > > > _scratch_mkfs | _filter_mkfs >>$seqres.full 2>$tmp.mkfs > > > > . $tmp.mkfs > > > > > > > > Then "dbsize" is what you want. > > > > > > > > > +leaf_lblk="$((32 * 1073741824 / blksz))" > > > > > +node_lblk="$((64 * 1073741824 / blksz))" > > > > I didn't see the "node_lblk" is used in this case, looks like you don't want to > > > > get directory node blocks in this case. > > > It's really needed here, must define leaf_lblk and node_lblk before calling > > > __populate_check_xfs_dir > > > or an waring will be printed by the function. > > Oh, so these two global parameters are used for later __populate_check_xfs_dir. > > Hmm.. are "blksz" and "dblksz" necessary too, for someone __populate_* helper > > you used? I really don't understand why we need them both. These helpers are > > written by Darrick, I think he learns about that more :) > yes, there are same usage for eg. xfs/113 xfs/101 ... OK, cc Darrick to ask why we need both dblksz and blksz at here? > > > > > +__populate_create_dir "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF" "$((dblksz / 12))" > > > > > +leaf_dir="$(__populate_find_inode "${SCRATCH_MNT}/S_IFDIR.FMT_LEAF")" > > > > > +_scratch_unmount > > > > > +__populate_check_xfs_dir "${leaf_dir}" "leaf" > > > > > + > > > > > +echo "Inject bad bestfress array size" > > > > > +_scratch_xfs_db -x -c "inode ${leaf_dir}" -c "dblock 8388608" -c "write ltail.bestcount 0" > > > > As you tried to detect xfs block size above, so it might not 4k block size, so > > > > 8388608 is not fixed. > > > > > > > > According to the kernel definition: > > > > #define XFS_DIR2_DATA_ALIGN_LOG 3 > > > > #define XFS_DIR2_SPACE_SIZE (1ULL << (32 + XFS_DIR2_DATA_ALIGN_LOG)) > > > > #define XFS_DIR2_LEAF_SPACE 1 > > > > #define XFS_DIR2_LEAF_OFFSET (XFS_DIR2_LEAF_SPACE * XFS_DIR2_SPACE_SIZE) > > > > > > > > The XFS_DIR2_LEAF_OFFSET = 1 * (1 << (32 + 3)) = 1<<35 = 34359738368 = 32GB, so > > > > the fixed logical offset of leaf extent is 34359738368 bytes, then the offset > > > > block number should be "34359738368 / dbsize". 8388608 is only for 4k block > > > > size. > > > Sorry, you are totally right! it should be "dblock ${leaf_lblk}" > > That looks better. > > > > > > > + > > > > > +echo "Test add entry to dir" > > > > > +_scratch_mount > > > > > +touch ${SCRATCH_MNT}/S_IFDIR.FMT_LEAF/{1..100}.txt > /dev/null 2>&1 > > > > > +_scratch_unmount 2>&1 > > This "2>&1" looks useless, I think it can be removed > OK, in v2 it will disappear :） > > > > > +_repair_scratch_fs >> $seqres.full 2>&1 > > > > Can you explain more about this testing steps? The xfs has been corrupted, then > > > > we expect is can be mounted. And create 100 new files on that corrupted dir, > > > > do you expect the 100 files can be created successfully? Or what ever, even > > > > nothing be created? > > > since we have create an leaf dir,and set bestfree count to 0; then, need to > > > touch some files to > > > trigger the problem, the action will be failed as expected. > > OK, I think you can add more comments to explain this part. Due to you make > > a obvious corruption at first, then try to mount and write the corrupted fs, > > there must be some error happen, so you'd better to explain what do you > > expect, and what's not. > Thanks a lot, and I'm happliy accept your suggestion, It's really a bit > confusing here, > I will add some specific description > > > > What's the xfs_repair expect? Fix all curruption and left a clean xfs? > > > Adding repair is really not necessary, only toavoid _check_xfs_filesystem > > > warning > > > " _check_xfs_filesystem: filesystem on /dev/sdb is inconsistent (r)" > > Except you'd like to verify if xfs_repair can fix this corruption. Or replace > > _require_scatch with _require_scratch_nocheck, that will help you avoid known > > fs corruption warning. Then you can remove _repair_scratch_fs and above > > _scratch_unmount. > Yep, you got my point, since it's my fisrt contribution code for fstests, > and > I didn't figure out how to disable the post fsck execution. so great, I will > add _require_scratch_nocheck. Sure, welcome more patches from you :) > > > > > + > > > > > +# check demsg error > > > > > +_check_dmesg > > > > Which above step will trigger a dmesg you want to check? What kind of dmesg do > > > dmesg eg: > > > [ 80.543884] XFS (sdb): Internal error xfs_dir2_data_use_free at line 1200 > > > of file fs/xfs/libxfs/xfs_dir2_data.c. Caller > > > xfs_dir2_data_use_free+0xb3/0xeb0 > > > [ 80.545141] CPU: 2 PID: 2978 Comm: touch Not tainted 6.0.0-rc3+ #115 > > > [ 80.545715] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > > > 1.13.0-1ubuntu1.1 04/01/2014 > > > [ 80.546546] Call Trace: > > > [ 80.546785] <TASK> > > > [ 80.546985] dump_stack_lvl+0x4d/0x66 > > > [ 80.547335] xfs_corruption_error+0x132/0x150 > > > [ 80.548391] ? xfs_dir2_data_use_free+0xb3/0xeb0 > > > [ 80.548901] ? xfs_dir2_data_use_free+0xb3/0xeb0 > > > [ 80.549319] ? xfs_dir2_data_use_free+0xb3/0xeb0 > > > [ 80.550190] xfs_dir2_data_use_free+0x198/0xeb0 > > > [ 80.550718] ? xfs_dir2_data_use_free+0xb3/0xeb0 > > > [ 80.551140] xfs_dir2_leaf_addname+0xa59/0x1ac0 > > > [ 80.551881] ? _raw_spin_unlock_irqrestore+0x42/0x80 > > > [ 80.552403] ? xfs_dir2_leaf_search_hash+0x300/0x300 > > > or > > > [ 201.405239] BUG: KASAN: slab-out-of-bounds in > > > xfs_dir2_leaf_addname+0x1995/0x1ac0 > > > [ 201.406179] Write of size 2 at addr ffff888078c33000 by task touch/7433 > > > [ 201.407010] > > > [ 201.407217] CPU: 6 PID: 7433 Comm: touch Not tainted 6.0.0-rc3+ #115 > > > [ 201.408016] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > > > 1.13.0-1ubuntu1.1 04/01/2014 > > > [ 201.409143] Call Trace: > > > [ 201.409461] <TASK> > > > [ 201.409740] dump_stack_lvl+0x4d/0x66 > > > [ 201.410214] print_report.cold+0xf6/0x691 > > > [ 201.410730] ? xfs_dir3_data_init+0x18e/0x960 > > > > > > UAF/slab-out-of bound etc... > > Look at the _check_dmesg, it checks "Internal error" and "\bBUG:" etc, so I > > think it can catch above dmesg error, you can remove this _check_dmesg and > > run again, to make sure if it works as you wish. > OK, I will check it again. > > > > you want to check? I think xfstests checks dmesg at the end of each test case, > > > > except you need to check some special one, or need a special filter? > > > check demsg without filter seems enough, so i did not add special filter. > > > > Thanks, > > > > Zorro > > > > > > > > > + > > > > > +# success, all done > > > > > +status=0 > > > > > +exit > > > > > diff --git a/tests/xfs/554.out b/tests/xfs/554.out > > > > > new file mode 100644 > > > > > index 00000000..ea1f30cc > > > > > --- /dev/null > > > > > +++ b/tests/xfs/554.out > > > > > @@ -0,0 +1,6 @@ > > > > > +QA output created by 554 > > > > > +Format and mount > > > > > +Create and check leaf dir > > > > > +Inject bad bestfress array size > > > > > +ltail.bestcount = 0 > > > > > +Test add entry to dir > > > > > -- > > > > > 2.25.1 > > > > > > > > > . > > . > Thanks > Xuenan >
