| Message ID | 161526485870.1214319.7885928745714445687.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | fstests: test kernel regressions fixed in 5.12 | expand |
On 09 Mar 2021 at 10:10, 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. > Looks good to me. Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > .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@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; > + } > + > + /* > + * 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
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@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; + } + + /* + * 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