On 09 Mar 2021 at 10:10, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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.
>
Looks good to me.
Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> .gitignore | 1 +
> src/Makefile | 2 +
> src/chprojid_fail.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/xfs/765 | 71 +++++++++++++++++++++++++++++++++++++++
> tests/xfs/765.out | 4 ++
> tests/xfs/group | 1 +
> 6 files changed, 170 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/.gitignore b/.gitignore
> index 03c03be5..3af8e207 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -58,6 +58,7 @@
> /src/bulkstat_null_ocount
> /src/bulkstat_unlink_test
> /src/bulkstat_unlink_test_modified
> +/src/chprojid_fail
> /src/cloner
> /src/dbtest
> /src/devzero
> diff --git a/src/Makefile b/src/Makefile
> index 38ee6718..3d729a34 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -29,7 +29,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \
> dio-invalidate-cache stat_test t_encrypted_d_revalidate \
> attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \
> - fscrypt-crypt-util bulkstat_null_ocount splice-test
> + fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail
>
> SUBDIRS = log-writes perf
>
> diff --git a/src/chprojid_fail.c b/src/chprojid_fail.c
> new file mode 100644
> index 00000000..8c5b5fee
> --- /dev/null
> +++ b/src/chprojid_fail.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Oracle. All Rights Reserved.
> + * Author: Darrick J. Wong <djwong@xxxxxxxxxx>
> + *
> + * 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;
> + }
> +
> + /*
> + * The regression we're trying to test happens when the fsxattr input
> + * validation decides to bail out after the chown quota reservation has
> + * been made on a file containing delalloc extents. Extent size hints
> + * can't be set on non-empty files and we can't check the value until
> + * we've reserved resources and taken the file's ILOCK, so this is a
> + * perfect vector for triggering this condition. In this way we set up
> + * a FSSETXATTR call that will 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..68b89ce3
> --- /dev/null
> +++ b/tests/xfs/765
> @@ -0,0 +1,71 @@
> +#! /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. This is a regression test
> +# for commit 1aecf3734a95 ("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_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
> +
> +# Make sure that a regular buffered write produces delalloc reservations.
> +$XFS_IO_PROG -f -c 'pwrite 0 64k' $SCRATCH_MNT/testy &> /dev/null
> +$FILEFRAG_PROG -v $SCRATCH_MNT/testy 2>&1 | grep -q delalloc || \
> + _notrun "test requires delayed allocation writes"
> +rm -f $SCRATCH_MNT/testy
> +
> +echo "Run test program"
> +$XFS_QUOTA_PROG -f -x -c 'report -ap' $SCRATCH_MNT >> $seqres.full
> +$here/src/chprojid_fail $SCRATCH_MNT/blah
> +
> +# The regression we're testing for is an accounting bug involving delalloc
> +# reservations. FSSETXATTR does not itself cause dirty data writeback, so we
> +# assume that if the file still has delalloc extents, then it must have had
> +# them when chprojid_fail was running, and therefore the test was set up
> +# correctly. There's a slight chance that background writeback can sneak in
> +# and flush the file, but this should be a small enough gap.
> +$FILEFRAG_PROG -v $SCRATCH_MNT/blah 2>&1 | grep -q delalloc || \
> + echo "file didn't get delalloc extents, test invalid?"
> +
> +# Make a note of current quota status for diagnostic purposes
> +$XFS_QUOTA_PROG -f -x -c 'report -ap' $SCRATCH_MNT >> $seqres.full
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/765.out b/tests/xfs/765.out
> new file mode 100644
> index 00000000..d5f8fc11
> --- /dev/null
> +++ b/tests/xfs/765.out
> @@ -0,0 +1,4 @@
> +QA output created by 765
> +Format filesystem
> +Run test program
> +FSSETXATTRR should fail: Invalid argument
> diff --git a/tests/xfs/group b/tests/xfs/group
> index d7aafc94..84468d10 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -505,4 +505,5 @@
> 760 auto quick rw collapse punch insert zero prealloc
> 761 auto quick realtime
> 763 auto quick rw realtime
> +765 auto quick quota
> 915 auto quick quota
--
chandan
