Re: [PATCH] xfs: test delalloc quota leak when chprojid fails

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Feb 03, 2021 at 11:11:55AM -0500, Brian Foster wrote:
> On Tue, Feb 02, 2021 at 11:41:01AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > This is a regression test for a bug in the XFS implementation of
> > FSSETXATTR.  When we try to change a file's project id, the quota
> > reservation code will update the incore quota reservations for delayed
> > allocation blocks.  Unfortunately, it does this before we finish
> > validating all the FSSETXATTR parameters, which means that if we decide
> > to bail out, we also fail to undo the incore changes.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  .gitignore          |    1 +
> >  src/Makefile        |    3 +-
> >  src/chprojid_fail.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/765       |   63 +++++++++++++++++++++++++++++++++++++
> >  tests/xfs/765.out   |    3 ++
> >  tests/xfs/group     |    1 +
> >  6 files changed, 156 insertions(+), 1 deletion(-)
> >  create mode 100644 src/chprojid_fail.c
> >  create mode 100755 tests/xfs/765
> >  create mode 100644 tests/xfs/765.out
> > 
> ...
> > diff --git a/src/chprojid_fail.c b/src/chprojid_fail.c
> > new file mode 100644
> > index 00000000..e7467372
> > --- /dev/null
> > +++ b/src/chprojid_fail.c
> > @@ -0,0 +1,86 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2021 Oracle.  All Rights Reserved.
> > + * Author: Darrick J. Wong <djwong@kernel.org>
> > + *
> > + * Regression test for failing to undo delalloc quota reservations when
> > + * changing project id and we fail some other FSSETXATTR validation.
> > + */
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <sys/ioctl.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +#include <linux/fs.h>
> > +
> > +static char zerobuf[65536];
> > +
> > +int
> > +main(
> > +	int		argc,
> > +	char		*argv[])
> > +{
> > +	struct fsxattr	fa;
> > +	ssize_t		sz;
> > +	int		fd, ret;
> > +
> > +	if (argc < 2) {
> > +		printf("Usage: %s filename\n", argv[0]);
> > +		return 1;
> > +	}
> > +
> > +	fd = open(argv[1], O_CREAT | O_TRUNC | O_RDWR, 0600);
> > +	if (fd < 0) {
> > +		perror(argv[1]);
> > +		return 2;
> > +	}
> > +
> > +	/* Zero the project id and the extent size hint. */
> > +	ret = ioctl(fd, FS_IOC_FSGETXATTR, &fa);
> > +	if (ret) {
> > +		perror("FSGETXATTR check file");
> > +		return 2;
> > +	}
> > +
> > +	if (fa.fsx_projid != 0 || fa.fsx_extsize != 0) {
> > +		fa.fsx_projid = 0;
> > +		fa.fsx_extsize = 0;
> > +		ret = ioctl(fd, FS_IOC_FSSETXATTR, &fa);
> > +		if (ret) {
> > +			perror("FSSETXATTR zeroing");
> > +			return 2;
> > +		}
> > +	}
> > +
> > +	/* Dirty a few kb of a file to create delalloc extents. */
> > +	sz = write(fd, zerobuf, sizeof(zerobuf));
> > +	if (sz != sizeof(zerobuf)) {
> > +		perror("delalloc write");
> > +		return 2;
> > +	}
> > +
> > +	/*
> > +	 * Fail to chprojid and set an extent size hint after we wrote the file.
> > +	 */
> 
> Might be helpful to document exactly why this command should fail.

Fixed.

> > +	ret = ioctl(fd, FS_IOC_FSGETXATTR, &fa);
> > +	if (ret) {
> > +		perror("FSGETXATTR");
> > +		return 2;
> > +	}
> > +
> > +	fa.fsx_projid = 23652;
> > +	fa.fsx_extsize = 2;
> > +	fa.fsx_xflags |= FS_XFLAG_EXTSIZE;
> > +
> > +	ret = ioctl(fd, FS_IOC_FSSETXATTR, &fa);
> > +	if (ret) {
> > +		printf("FSSETXATTRR should fail: %s\n", strerror(errno));
> > +		return 0;
> > +	}
> > +
> > +	/* Uhoh, that FSSETXATTR call should have failed! */
> > +	return 3;
> > +}
> > diff --git a/tests/xfs/765 b/tests/xfs/765
> > new file mode 100755
> > index 00000000..769b545b
> > --- /dev/null
> > +++ b/tests/xfs/765
> > @@ -0,0 +1,63 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0-or-later
> > +# Copyright (c) 2021 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 765
> > +#
> > +# Regression test for failing to undo delalloc quota reservations when changing
> > +# project id but we fail some other part of FSSETXATTR validation.  If we fail
> > +# the test, we trip debugging assertions in dmesg.
> > +#
> > +# The appropriate XFS patch is:
> > +# xfs: fix chown leaking delalloc quota blocks when fssetxattr fails
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1    # failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/quota
> > +
> > +# real QA test starts here
> > +_supported_fs xfs
> > +_require_xfs_debug
> 
> I agree with Zorro that we might as well just let this run with or
> without debug mode..

<nod> Fixed.

> > +_require_command "$FILEFRAG_PROG" filefrag
> > +_require_test_program "chprojid_fail"
> > +_require_quota
> > +_require_scratch
> > +
> > +rm -f $seqres.full
> > +
> > +echo "Format filesystem" | tee -a $seqres.full
> > +_scratch_mkfs > $seqres.full
> > +_qmount_option 'prjquota'
> > +_qmount
> > +_require_prjquota $SCRATCH_DEV
> > +
> > +echo "Run test program"
> > +$XFS_QUOTA_PROG -f -x -c 'report -ap' $SCRATCH_MNT >> $seqres.full
> > +$here/src/chprojid_fail $SCRATCH_MNT/blah >> $seqres.full
> > +res=$?
> > +if [ $res -ne 0 ]; then
> > +	echo "chprojid_fail returned $res, expected 0"
> > +fi
> 
> Can we just use the output from the test program as the golden test
> output?

Hm, I guess so?  Not sure I want to encode the exact failure in the
test, but I don't see why not.

Admittedly it's done that way because originally I was going to wire
this up as a generic test, but then noticed that not all the other
fssetxattr implementations actually error out on nonzero extent size and
decided to pull this back into tests/xfs/.

> > +$XFS_QUOTA_PROG -f -x -c 'report -ap' $SCRATCH_MNT >> $seqres.full
> > +$FILEFRAG_PROG -v $SCRATCH_MNT/blah >> $seqres.full
> > +$FILEFRAG_PROG -v $SCRATCH_MNT/blah 2>&1 | grep -q delalloc || \
> > +	echo "file didn't get delalloc extents?"
> 
> This could technically cause a failure even if the test did the right
> thing if writeback happens to occur, right? I suppose that's exceedingly
> unlikely, but might at least be worth documenting in the event that
> somebody has to deal with that scenario sometime in the future.

Yeah.  I'll add a comment explaining that the delalloc grep is to check
that we even set up the test correctly.

--D

> 
> Brian
> 
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/765.out b/tests/xfs/765.out
> > new file mode 100644
> > index 00000000..f44ba43e
> > --- /dev/null
> > +++ b/tests/xfs/765.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 765
> > +Format filesystem
> > +Run test program
> > diff --git a/tests/xfs/group b/tests/xfs/group
> > index f406a9b9..fb78b0d7 100644
> > --- a/tests/xfs/group
> > +++ b/tests/xfs/group
> > @@ -544,6 +544,7 @@
> >  762 auto quick rw scrub realtime
> >  763 auto quick rw realtime
> >  764 auto quick repair
> > +765 auto quick quota
> >  908 auto quick bigtime
> >  909 auto quick bigtime quota
> >  910 auto quick inobtcount
> > 
>