Re: [PATCH v2] generic: add test for XFS forkoff miscalcution on 32-bit platform

[Gao Xiang <hsiangkao@redhat.com>]

Hi Darrick,

On Mon, Nov 23, 2020 at 10:24:00AM -0800, Darrick J. Wong wrote:
> On Mon, Nov 23, 2020 at 04:20:47PM +0800, Gao Xiang wrote:

...

> > +_supported_fs generic
> > +_require_scratch
> > +_require_attrs user
> 
> Does this require
> 
> _require_no_xfs_bug_on_assert ?
> 

For the sake of harmless testcase, I think that is fine and I will
add it in the next version as
[ $FSTYP = "xfs" ] && _require_no_xfs_bug_on_assert


To clarify the synotom:
if bug_on_assert is on, it will bug_on on the second setfattr,
_scratch_cycle_mount will fail due to
umount: can't unmount /tmp/mnt: Device or resource busy

if _scratch_cycle_mount is removed, _getfattr will hung due to
(I think) unbalanced xfs_ilock(dp, XFS_ILOCK_EXCL) in xfs_attr_set()
and xfs_attr_get() will take the lock again.


if bug_on_assert is off, it will fail on _scratch_cycle_mount due to
log recovery replay fail when mounting as below

[  856.786226] XFS (sda): Mounting V5 Filesystem                       
[  856.802704] XFS (sda): Starting recovery (logdev: internal)                                                                                                                                                    
[  856.806362] XFS: Assertion failed: len <= XFS_DFORK_ASIZE(dip, mp), file: fs/xfs/xfs_inode_item_recover.c, line: 426

but if the line of _scratch_cycle_mount is removed, _getfattr will
success due to
	ifp->if_u1.if_data = krealloc(ifp->if_u1.if_data, roundup(new_size, 4),
				      GFP_NOFS | __GFP_NOFAIL);
	ifp->if_bytes = new_size;
in xfs_idata_realloc(), and xfs_attr_shortform_getvalue() will
temporary ok. yet this testcase have _scratch_cycle_mount anyway.

In any case, it's fine with _getfattr after _scratch_cycle_mount
for this testcase.

And "bug_on_assert is off" does no harm to the system stablity itself
so I think that's better.

> > +
> > +# Use fixed inode size 512, so both v4 and v5 can be tested,
> > +# and also make sure the issue can be triggered if the default
> > +# inode size is changed later.
> > +[ $FSTYP = "xfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -i size=512"
> > +_scratch_mkfs > $seqres.full 2>&1
> > +_scratch_mount
> > +
> > +localfile="${SCRATCH_MNT}/testfile"
> > +touch $localfile
> > +
> > +# value cannot exceed XFS_ATTR_SF_ENTSIZE_MAX (256) or it will turn into
> > +# leaf form directly; the following combination can trigger the issue for
> > +# both v4 (XFS_LITINO = 412) & v5 (XFS_LITINO = 336) fses.
> > +"${SETFATTR_PROG}" -n user.0 -v "`seq 0 80`" "${localfile}"
> > +"${SETFATTR_PROG}" -n user.1 -v "`seq 0 80`" "${localfile}"
> 
> It's probably worth mentioning that the second setattr causes an integer
> underflow that is incorrectly typecast, leading to the assert
> triggering.  Otherwise this seems reasonable to me.

Ok, will try to add in the next version as well.

Thanks,
Gao Xiang

> 
> --D
> 
> > +
> > +# Make sure that changes are written to disk
> > +_scratch_cycle_mount
> > +
> > +# getfattr won't succeed with the expected result if fails
> > +_getfattr --absolute-names -ebase64 -d $localfile | tail -n +2 | sort
> > +
> > +_scratch_unmount
> > +status=0
> > +exit
> > diff --git a/tests/generic/618.out b/tests/generic/618.out
> > new file mode 100644
> > index 00000000..848fdc58
> > --- /dev/null
> > +++ b/tests/generic/618.out
> > @@ -0,0 +1,4 @@
> > +QA output created by 618
> > +
> > +user.0=0sMAoxCjIKMwo0CjUKNgo3CjgKOQoxMAoxMQoxMgoxMwoxNAoxNQoxNgoxNwoxOAoxOQoyMAoyMQoyMgoyMwoyNAoyNQoyNgoyNwoyOAoyOQozMAozMQozMgozMwozNAozNQozNgozNwozOAozOQo0MAo0MQo0Mgo0Mwo0NAo0NQo0Ngo0Nwo0OAo0OQo1MAo1MQo1Mgo1Mwo1NAo1NQo1Ngo1Nwo1OAo1OQo2MAo2MQo2Mgo2Mwo2NAo2NQo2Ngo2Nwo2OAo2OQo3MAo3MQo3Mgo3Mwo3NAo3NQo3Ngo3Nwo3OAo3OQo4MA==
> > +user.1=0sMAoxCjIKMwo0CjUKNgo3CjgKOQoxMAoxMQoxMgoxMwoxNAoxNQoxNgoxNwoxOAoxOQoyMAoyMQoyMgoyMwoyNAoyNQoyNgoyNwoyOAoyOQozMAozMQozMgozMwozNAozNQozNgozNwozOAozOQo0MAo0MQo0Mgo0Mwo0NAo0NQo0Ngo0Nwo0OAo0OQo1MAo1MQo1Mgo1Mwo1NAo1NQo1Ngo1Nwo1OAo1OQo2MAo2MQo2Mgo2Mwo2NAo2NQo2Ngo2Nwo2OAo2OQo3MAo3MQo3Mgo3Mwo3NAo3NQo3Ngo3Nwo3OAo3OQo4MA==
> > diff --git a/tests/generic/group b/tests/generic/group
> > index 94e860b8..eca9d619 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -620,3 +620,4 @@
> >  615 auto rw
> >  616 auto rw io_uring stress
> >  617 auto rw io_uring stress
> > +618 auto quick attr
> > -- 
> > 2.18.4
> > 
>