Re: [PATCH v3] xfs/12[4-6]: Add getfattr operation after xattr corrupted

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

on 2022/1/27 12:59, Zorro Lang wrote:
> On Thu, Jan 27, 2022 at 03:02:36AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/1/23 21:09, Eryu Guan wrote:
>>> On Mon, Jan 17, 2022 at 04:22:16PM +0800, Yang Xu wrote:
>>>> Add getfattr operation in these three cases, we can also use xfs/126(corrupt a leaf
>>>> xattr's data extent) to reproduce a deadlock. This deadlock is introduced by kernel
>>>> commit 07120f1abdff ("xfs: Add xfs_has_attr and subroutines") and fixed by kernel
>>>
>>> So this was introduced since 5.9 kernel, and
>>>
>>>> commit a1de97fe296c ("xfs: Fix the free logic of state in xfs_attr_node_hasname").
>>>
>>> fixed in 5.16 kernel.
>>>
>>> And adding this regression test in xfs/126, which passed in the past.
>>> probably would cause all the affected kernels in between start to hang,
>>> and trigger a false positive regression alert.
>>>
>>> So IMO it'd be better to make a separated&   targeted regression test for
>>> this case, as Darrick suggested.
>> Ok, Now, I understand but don't figure out how to setattr  to use leaf
>> attr block accuratly and corrupt leaf attr accuratly even I have read
>> the documentation[1] instead of xfs/126 using generic fuzz way.
>
> Generally, if you keep creating attr entries, until the attr out of an inode
> internal attrfork space, you'll get leaf block and separated attr value blocks.
> If you keep creating more attr entries, until the attr entries out of a block
> size, you'll get node and leaf blocks.
>
>  From xfs/126, Darrick use a while loop to create attrs in about 8 blocks.
>
> nr="$((8 * blksz / 40))"
> seq 0 "${nr}" | while read d; do
>          setfattr -n "user.x$(printf "%.08d" "$d")" -v "0000000000000000" "${SCRATCH_MNT}/attrfile"
> done
>
> (Although I don't know why it's 40. From above setfattr command, I think each xfs_attr_leaf_name_local
> takes 28 bytes, each xfs_attr_leaf_entry takes 8 bytes. And there're node and leaf blocks, not sure
> how Darrick knows it'll take 8 blocks :).
>
> I didn't give it a test, not sure if Darrick would like to get 1 node block and 7 leaf blocks at here.
> Then he loop trash each ablocks (from 1 to end), except the root node block.
>
> loff=1
> while true; do
>          _scratch_xfs_db -x -c "inode ${inode}" -c "ablock ${loff}" -c "stack" | grep -q 'file attr block is unmapped'&&  break
>          _scratch_xfs_db -x -c "inode ${inode}" -c "ablock ${loff}" -c "stack" -c "blocktrash -o 4 -x 32 -y $((blksz * 8)) -z ${FUZZ_ARGS}">>  $seqres.full
>          loff="$((loff + 1))"
> done
>
> Thanks,
> Zorro

Thanks for your explaination, I will figure out each detail after 
Chinese Lunar New Year.

Happy Chinese Lunar New Year in advance.

Best Regards
Yang Xu
>
>
>
>>
>> So if somebody can do this or teach me, I will be pleasure.
>>
>> [1]https://github.com/djwong/xfs-documentation/blob/master/design/XFS_Filesystem_Structure/extended_attributes.asciidoc
>>
>>
>> Best Regards
>> Yang Xu
>>>
>>> Thanks,
>>> Eryu
>>>
>>>>
>>>> Also making getfattr operation after xattr corrupted is a common part, so we can
>>>> test whether the similar deadlock occurs in the future in xfs/124(corrupt a block xattr)
>>>> and xfs/125(corrupt a leaf xattr's index extent).
>>>>
>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>>> ---
>>>>    tests/xfs/124 | 2 +-
>>>>    tests/xfs/125 | 2 +-
>>>>    tests/xfs/126 | 2 +-
>>>>    3 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tests/xfs/124 b/tests/xfs/124
>>>> index 39307218..c3cccfd5 100755
>>>> --- a/tests/xfs/124
>>>> +++ b/tests/xfs/124
>>>> @@ -64,7 +64,7 @@ _scratch_xfs_db -x -c "inode ${inode}" -c 'ablock 0' -c "stack" -c "blocktrash -
>>>>
>>>>    echo "+ mount image&&   modify xattr"
>>>>    if _try_scratch_mount>>   $seqres.full 2>&1; then
>>>> -
>>>> +	getfattr "${SCRATCH_MNT}/attrfile" -n "user.x00000000" 2>   /dev/null&&   _fail "got corrupt xattr"
>>>>    	setfattr -x "user.x00000000" "${SCRATCH_MNT}/attrfile" 2>   /dev/null&&   _fail "modified corrupt xattr"
>>>>    	umount "${SCRATCH_MNT}"
>>>>    fi
>>>> diff --git a/tests/xfs/125 b/tests/xfs/125
>>>> index fb5f5695..a7eb51fb 100755
>>>> --- a/tests/xfs/125
>>>> +++ b/tests/xfs/125
>>>> @@ -64,7 +64,7 @@ _scratch_xfs_db -x -c "inode ${inode}" -c 'ablock 0' -c "stack" -c "blocktrash -
>>>>
>>>>    echo "+ mount image&&   modify xattr"
>>>>    if _try_scratch_mount>>   $seqres.full 2>&1; then
>>>> -
>>>> +	getfattr "${SCRATCH_MNT}/attrfile" -n "user.x00000000" 2>   /dev/null&&   _fail "got corrupt xattr"
>>>>    	setfattr -x "user.x00000000" "${SCRATCH_MNT}/attrfile" 2>   /dev/null&&   _fail "modified corrupt xattr"
>>>>    	umount "${SCRATCH_MNT}"
>>>>    fi
>>>> diff --git a/tests/xfs/126 b/tests/xfs/126
>>>> index c3a74b1c..519d377e 100755
>>>> --- a/tests/xfs/126
>>>> +++ b/tests/xfs/126
>>>> @@ -69,7 +69,7 @@ done
>>>>
>>>>    echo "+ mount image&&   modify xattr"
>>>>    if _try_scratch_mount>>   $seqres.full 2>&1; then
>>>> -
>>>> +	getfattr "${SCRATCH_MNT}/attrfile" -n "user.x00000000" 2>   /dev/null&&   _fail "got corrupt xattr"
>>>>    	setfattr -x "user.x00000000" "${SCRATCH_MNT}/attrfile" 2>   /dev/null&&   _fail "modified corrupt xattr"
>>>>    	umount "${SCRATCH_MNT}"
>>>>    fi
>>>> --
>>>> 2.23.0
>