diff mbox series

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

Message ID 1637817207-2164-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v2] xfs/126: Add a getxattr opeartion after corrupted xattr | expand

Commit Message

Yang Xu (Fujitsu) Nov. 25, 2021, 5:13 a.m. UTC
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

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 tests/xfs/126 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong Nov. 26, 2021, 11:28 p.m. UTC | #1
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.

--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
>
Yang Xu (Fujitsu) Jan. 6, 2022, 1:41 a.m. UTC | #2
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
>>
diff mbox series

Patch

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