Re: [PATCH v2 3/3] f2fs/020: do sanity check on i_xattr_nid

[Chao Yu <chao@kernel.org>]

On 2025/8/15 23:12, Zorro Lang wrote:
> On Fri, Aug 15, 2025 at 04:02:12PM +0800, Chao Yu wrote:
>> On 8/15/25 05:18, Zorro Lang wrote:
>>> On Thu, Aug 14, 2025 at 05:07:12PM +0800, Chao Yu wrote:
>>>> This is a regression test:
>>>> 1. create directory
>>>> 2. add a new xattr entry to create xattr node
>>>> 3. use inject.f2fs to inject nid of xattr node w/ ino in a file
>>>> 4. check whether f2fs kernel module will detect and report such
>>>>     corruption in the file
>>>>
>>>> Cc: Jaegeuk Kim <jaegeuk@kernel.org>
>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>> ---
>>>> v3:
>>>> - use _require_scratch_nocheck instead of _require_scratch
>>>> - add missing _require_attrs user
>>>> - add comments for why exporting MKFS_OPTIONS=""
>>>>   tests/f2fs/020     | 51 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>   tests/f2fs/020.out |  2 ++
>>>>   2 files changed, 53 insertions(+)
>>>>   create mode 100755 tests/f2fs/020
>>>>   create mode 100644 tests/f2fs/020.out
>>>>
>>>> diff --git a/tests/f2fs/020 b/tests/f2fs/020
>>>> new file mode 100755
>>>> index 00000000..7ce1f92f
>>>> --- /dev/null
>>>> +++ b/tests/f2fs/020
>>>> @@ -0,0 +1,51 @@
>>>> +#! /bin/bash
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# Copyright (c) 2025 Chao Yu.  All Rights Reserved.
>>>> +#
>>>> +# FS QA Test No. f2fs/020
>>>> +#
>>>> +# This is a regression test:
>>>> +# 1. create directory
>>>> +# 2. add a new xattr entry to create xattr node
>>>> +# 3. use inject.f2fs to inject nid of xattr node w/ ino in a file
>>>> +# 4. check whether f2fs kernel module will detect and report such
>>>> +#    corruption in the file
>>>> +#
>>>> +. ./common/preamble
>>>> +_begin_fstest auto quick rw
>>>> +
>>>> +. ./common/attr
>>>> +
>>>> +_fixed_by_kernel_commit 061cf3a84bde \
>>>> +	"f2fs: fix to do sanity check on ino and xnid"
>>>> +
>>>> +_require_scratch_nocheck
>>>> +_require_command "$F2FS_INJECT_PROG" inject.f2fs
>>>> +_require_attrs user
>>>> +
>>>> +# remove all mkfs options to avoid layout change of on-disk inode
>>>> +export MKFS_OPTIONS=""
>>>> +
>>>> +testdir=$SCRATCH_MNT/testdir
>>>> +
>>>> +_scratch_mkfs >> $seqres.full
>>>> +_scratch_mount "-o user_xattr,noinline_xattr"
>>>> +
>>>> +mkdir $testdir
>>>> +# add a new xattr entry to create xattr node
>>>> +$SETFATTR_PROG -n user.abc -v 123 $testdir
>>>> +
>>>> +_scratch_unmount
>>>> +
>>>> +# inject i_xattr_nid w/ nid of inode node
>>>> +$F2FS_INJECT_PROG --node --mb i_xattr_nid --nid 4 --val 4 $SCRATCH_DEV >> $seqres.full
>>
>> Zorro, I missed one thing, this testcase relies on a f2fs-tools patch, please apply
>> this before running the testcase.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/commit/?h=dev-test&id=2174461cdd0a22edc5e7d172605bff4740582438
>>
>> Do I need to create a _require_* function for f2fs to verify whether f2fs-tools
>> support the required feature or not?
> 
> Actually for further f2fs testing (depends on injection), we'd better to have related
> functions, you can refer to:
> 
> _require_xfs_db_command
> 
> Or more xfs_db related functions in common/xfs if you need.

Thanks for the information.

> 
> If you don't want to add this _require_ function in this patch (this's not the only
> one f2fs case uses inject.f2fs), I think you can send another patchset to do that
> later (just let this case fails on old f2fs temporarily :).

Okay, will introduce _require_ function in separated patch. :)

Thanks,

> 
> Thanks,
> Zorro
> 
>>
>>>> +
>>>> +_scratch_mount
>>
>> Oh, I may missed to add "-o acl,user_xattr" here.
>>
>>>> +# may potentially trigger a deadlock w/ double inode_lock on parent's inode
>>>> +mkdir $testdir/dir >> $seqres.full 2>&1
>>>> +_scratch_unmount
>>>
>>> Can you provide a test result to prove this test case trigger a failure
>>> on general kernel (not debug kernel). If it can, then this version is good
>>> to me :)
>>
>> Well, expectation is it will hang kernel, so that the f2fs/020 will stop here
>> once we encounter the issue.
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>> Zorro
>>>
>>>> +
>>>> +echo "Silence is golden"
>>>> +
>>>> +status=0
>>>> +exit
>>>> diff --git a/tests/f2fs/020.out b/tests/f2fs/020.out
>>>> new file mode 100644
>>>> index 00000000..20d7944e
>>>> --- /dev/null
>>>> +++ b/tests/f2fs/020.out
>>>> @@ -0,0 +1,2 @@
>>>> +QA output created by 020
>>>> +Silence is golden
>>>> -- 
>>>> 2.49.0
>>>>
>>>
>>
>