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

[Brian Foster <bfoster@redhat.com>]

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.

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

> +_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?

> +$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.

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
>