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

[Zorro Lang <zlang@redhat.com>]

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