Re: [PATCH v2] xfs/126: Add a getxattr opeartion after corrupted xattr

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

on 2021/11/27 7:28, Darrick J. Wong wrote:
> On Thu, Nov 25, 2021 at 01:13:27PM +0800, Yang Xu wrote:
>> It is design to reproduce a deadlock on upstream kernel. It is introduced by kernel
>> commit 07120f1abdff ("xfs: Add xfs_has_attr and subroutines") and fixed by kernel
>> commit a1de97fe296c ("xfs: Fix the free logic of state in xfs_attr_node_hasname")[1].
>>
>> [1]https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/commit/?h=for-next&id=a1de97fe296c
>
> NAK, this is a general fuzz test.  Please create a separate targeted
> regression test for the kernel bugfix so that you can control exactly
> which part of the attr leaf block gets corrupted so that the verifier
> will signal error.
Sorry for the late reply.
IMO, getattr or setattr should all report metadata corrupt in 
dmesg(command fail)after the attr leaf block gets corrupted.

I think it should be a common part, not only for this case also for 
other attr corrupt case ie xfs/125. We should add getattr for them after 
attr corrupt to avoid similar problem occurs other places.
what do you think about this?

ps: I should update my commit message for this.

Best Regards
Yang Xu
>
> --D
>
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   tests/xfs/126 | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/xfs/126 b/tests/xfs/126
>> index c3a74b1c..5496f3d7 100755
>> --- a/tests/xfs/126
>> +++ b/tests/xfs/126
>> @@ -7,6 +7,10 @@
>>   # Create and populate an XFS filesystem, corrupt a leaf xattr's data extent,
>>   # then see how the kernel and xfs_repair deal with it.
>>   #
>> +# It is also a regression test for kernel commit:
>> +# a1de97fe296c ("xfs: Fix the free logic of state in xfs_attr_node_hasname")
>> +#
>> +
>>   . ./common/preamble
>>   _begin_fstest fuzzers
>>
>> @@ -69,7 +73,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
>>