| Message ID | 1642407736-3898-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] xfs/12[4-6]: Add getfattr operation after xattr corrupted | expand |
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
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
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
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 >
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
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(-)