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

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

[Yang Xu]

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(-)

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

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

[Darrick J. Wong]

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
> 

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

[xuyang2018.jy]

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
>>

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

[Yang Xu]

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
commit a1de97fe296c ("xfs: Fix the free logic of state in xfs_attr_node_hasname").

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

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

[Eryu Guan]

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.

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

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

[xuyang2018.jy]

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.

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

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

[Zorro Lang]

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



> 
> 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

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

[xuyang2018.jy]

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
>