[PATCHSET 0/4] fstests: new tests for kernel 5.18

[PATCHSET 0/4] fstests: new tests for kernel 5.18

[Darrick J. Wong]

Hi all,

Add new tests for bugfixes merged during 5.18.  Specifically, we now
check quota enforcement when linking and renaming into a directory.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-merge-5.18

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=xfs-merge-5.18
---
 tests/generic/832     |   67 ++++++++++++++++++++++++++
 tests/generic/832.out |    3 +
 tests/generic/833     |   71 +++++++++++++++++++++++++++
 tests/generic/833.out |    3 +
 tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/834.out |   33 +++++++++++++
 tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/835.out |   33 +++++++++++++
 tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/836.out |   33 +++++++++++++
 tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/837.out |   33 +++++++++++++
 tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/838.out |   33 +++++++++++++
 tests/generic/839     |   77 ++++++++++++++++++++++++++++++
 tests/generic/839.out |   13 +++++
 tests/xfs/839         |   42 ++++++++++++++++
 tests/xfs/839.out     |    2 +
 18 files changed, 1078 insertions(+)
 create mode 100755 tests/generic/832
 create mode 100644 tests/generic/832.out
 create mode 100755 tests/generic/833
 create mode 100644 tests/generic/833.out
 create mode 100755 tests/generic/834
 create mode 100644 tests/generic/834.out
 create mode 100755 tests/generic/835
 create mode 100644 tests/generic/835.out
 create mode 100755 tests/generic/836
 create mode 100644 tests/generic/836.out
 create mode 100755 tests/generic/837
 create mode 100644 tests/generic/837.out
 create mode 100755 tests/generic/838
 create mode 100644 tests/generic/838.out
 create mode 100755 tests/generic/839
 create mode 100755 tests/generic/839.out
 create mode 100755 tests/xfs/839
 create mode 100644 tests/xfs/839.out

[PATCH 1/4] xfs: make sure syncfs(2) passes back super_operations.sync_fs errors

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This is a regression test to make sure that nonzero error returns from
a filesystem's ->sync_fs implementation are actually passed back to
userspace when the call stack involves syncfs(2).

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/839     |   42 ++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/839.out |    2 ++
 2 files changed, 44 insertions(+)
 create mode 100755 tests/xfs/839
 create mode 100644 tests/xfs/839.out


diff --git a/tests/xfs/839 b/tests/xfs/839
new file mode 100755
index 00000000..9bfe93ef
--- /dev/null
+++ b/tests/xfs/839
@@ -0,0 +1,42 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 839
+#
+# Regression test for kernel commits:
+#
+# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
+# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs")
+#
+# During a code inspection, I noticed that sync_filesystem ignores the return
+# value of the ->sync_fs calls that it makes.  sync_filesystem, in turn is used
+# by the syncfs(2) syscall to persist filesystem changes to disk.  This means
+# that syncfs(2) does not capture internal filesystem errors that are neither
+# visible from the block device (e.g. media error) nor recorded in s_wb_err.
+# XFS historically returned 0 from ->sync_fs even if there were log failures,
+# so that had to be corrected as well.
+#
+# The kernel commits above fix this problem, so this test tries to trigger the
+# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the
+# hope that the EIO generated as a result of the filesystem being shut down is
+# only visible via ->sync_fs.
+#
+. ./common/preamble
+_begin_fstest auto quick shutdown
+
+# real QA test starts here
+
+# Modify as appropriate.
+_require_xfs_io_command syncfs
+_require_scratch_nocheck
+_require_scratch_shutdown
+
+# Reuse the fs formatted when we checked for the shutdown ioctl, and don't
+# bother checking the filesystem afterwards since we never wrote anything.
+_scratch_mount
+$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/839.out b/tests/xfs/839.out
new file mode 100644
index 00000000..f275cdcc
--- /dev/null
+++ b/tests/xfs/839.out
@@ -0,0 +1,2 @@
+QA output created by 839
+syncfs: Input/output error

Re: [PATCH 1/4] xfs: make sure syncfs(2) passes back super_operations.sync_fs errors

[Zorro Lang]

On Mon, Apr 11, 2022 at 03:54:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> This is a regression test to make sure that nonzero error returns from
> a filesystem's ->sync_fs implementation are actually passed back to
> userspace when the call stack involves syncfs(2).
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/839     |   42 ++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/839.out |    2 ++
>  2 files changed, 44 insertions(+)
>  create mode 100755 tests/xfs/839
>  create mode 100644 tests/xfs/839.out
> 
> 
> diff --git a/tests/xfs/839 b/tests/xfs/839

This case looks good to me. Just one question, is it possible to be a generic
case? From the code logic, it doesn't use xfs specified operations, but I'm
not sure if other filesystems would like to treat sync_fs return value as XFS.

> new file mode 100755
> index 00000000..9bfe93ef
> --- /dev/null
> +++ b/tests/xfs/839
> @@ -0,0 +1,42 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 839
> +#
> +# Regression test for kernel commits:
> +#
> +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
> +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs")

BTW, after this change, now can I assume that sync(2) flushes all data and metadata
to underlying disk, if it returns 0. Sorry, really confused on what these sync things
really guarantee :)

Thanks,
Zorro

> +#
> +# During a code inspection, I noticed that sync_filesystem ignores the return
> +# value of the ->sync_fs calls that it makes.  sync_filesystem, in turn is used
> +# by the syncfs(2) syscall to persist filesystem changes to disk.  This means
> +# that syncfs(2) does not capture internal filesystem errors that are neither
> +# visible from the block device (e.g. media error) nor recorded in s_wb_err.
> +# XFS historically returned 0 from ->sync_fs even if there were log failures,
> +# so that had to be corrected as well.
> +#
> +# The kernel commits above fix this problem, so this test tries to trigger the
> +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the
> +# hope that the EIO generated as a result of the filesystem being shut down is
> +# only visible via ->sync_fs.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick shutdown
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_require_xfs_io_command syncfs
> +_require_scratch_nocheck
> +_require_scratch_shutdown
> +
> +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't
> +# bother checking the filesystem afterwards since we never wrote anything.
> +_scratch_mount
> +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/839.out b/tests/xfs/839.out
> new file mode 100644
> index 00000000..f275cdcc
> --- /dev/null
> +++ b/tests/xfs/839.out
> @@ -0,0 +1,2 @@
> +QA output created by 839
> +syncfs: Input/output error
> 

Re: [PATCH 1/4] xfs: make sure syncfs(2) passes back super_operations.sync_fs errors

[Darrick J. Wong]

On Tue, Apr 12, 2022 at 05:37:27PM +0800, Zorro Lang wrote:
> On Mon, Apr 11, 2022 at 03:54:37PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > This is a regression test to make sure that nonzero error returns from
> > a filesystem's ->sync_fs implementation are actually passed back to
> > userspace when the call stack involves syncfs(2).
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/xfs/839     |   42 ++++++++++++++++++++++++++++++++++++++++++
> >  tests/xfs/839.out |    2 ++
> >  2 files changed, 44 insertions(+)
> >  create mode 100755 tests/xfs/839
> >  create mode 100644 tests/xfs/839.out
> > 
> > 
> > diff --git a/tests/xfs/839 b/tests/xfs/839
> 
> This case looks good to me. Just one question, is it possible to be a generic
> case? From the code logic, it doesn't use xfs specified operations, but I'm
> not sure if other filesystems would like to treat sync_fs return value as XFS.

Other filesystems (ext4 in particular) haven't been fixed to make
->sync_fs return error codes when the fs has been shut down via
FS_IOC_SHUTDOWN.  We'll get there eventually, but for now I'd like to
get this under test for XFS since we've applied those fixes.

> > new file mode 100755
> > index 00000000..9bfe93ef
> > --- /dev/null
> > +++ b/tests/xfs/839
> > @@ -0,0 +1,42 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 839
> > +#
> > +# Regression test for kernel commits:
> > +#
> > +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
> > +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs")
> 
> BTW, after this change, now can I assume that sync(2) flushes all data and metadata
> to underlying disk, if it returns 0.

Yes.

> Sorry, really confused on what these sync things
> really guarantee :)

No worries -- the history of the sync variants has been very messy and
confusing even to people on fsdevel.

--D

> 
> Thanks,
> Zorro
> 
> > +#
> > +# During a code inspection, I noticed that sync_filesystem ignores the return
> > +# value of the ->sync_fs calls that it makes.  sync_filesystem, in turn is used
> > +# by the syncfs(2) syscall to persist filesystem changes to disk.  This means
> > +# that syncfs(2) does not capture internal filesystem errors that are neither
> > +# visible from the block device (e.g. media error) nor recorded in s_wb_err.
> > +# XFS historically returned 0 from ->sync_fs even if there were log failures,
> > +# so that had to be corrected as well.
> > +#
> > +# The kernel commits above fix this problem, so this test tries to trigger the
> > +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the
> > +# hope that the EIO generated as a result of the filesystem being shut down is
> > +# only visible via ->sync_fs.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick shutdown
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_require_xfs_io_command syncfs
> > +_require_scratch_nocheck
> > +_require_scratch_shutdown
> > +
> > +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't
> > +# bother checking the filesystem afterwards since we never wrote anything.
> > +_scratch_mount
> > +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/xfs/839.out b/tests/xfs/839.out
> > new file mode 100644
> > index 00000000..f275cdcc
> > --- /dev/null
> > +++ b/tests/xfs/839.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 839
> > +syncfs: Input/output error
> > 
> 

Re: [PATCH 1/4] xfs: make sure syncfs(2) passes back super_operations.sync_fs errors

[Zorro Lang]

On Tue, Apr 12, 2022 at 10:28:53AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 12, 2022 at 05:37:27PM +0800, Zorro Lang wrote:
> > On Mon, Apr 11, 2022 at 03:54:37PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > This is a regression test to make sure that nonzero error returns from
> > > a filesystem's ->sync_fs implementation are actually passed back to
> > > userspace when the call stack involves syncfs(2).
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/xfs/839     |   42 ++++++++++++++++++++++++++++++++++++++++++
> > >  tests/xfs/839.out |    2 ++
> > >  2 files changed, 44 insertions(+)
> > >  create mode 100755 tests/xfs/839
> > >  create mode 100644 tests/xfs/839.out
> > > 
> > > 
> > > diff --git a/tests/xfs/839 b/tests/xfs/839
> > 
> > This case looks good to me. Just one question, is it possible to be a generic
> > case? From the code logic, it doesn't use xfs specified operations, but I'm
> > not sure if other filesystems would like to treat sync_fs return value as XFS.
> 
> Other filesystems (ext4 in particular) haven't been fixed to make
> ->sync_fs return error codes when the fs has been shut down via
> FS_IOC_SHUTDOWN.  We'll get there eventually, but for now I'd like to
> get this under test for XFS since we've applied those fixes.

If other filesystems intend to do that, how about using a generic case failure to
remind them they haven't done that :) If they don't intend that, keep this case
under xfs is good to me.

> 
> > > new file mode 100755
> > > index 00000000..9bfe93ef
> > > --- /dev/null
> > > +++ b/tests/xfs/839
> > > @@ -0,0 +1,42 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 839
> > > +#
> > > +# Regression test for kernel commits:
> > > +#
> > > +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
> > > +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs")
> > 
> > BTW, after this change, now can I assume that sync(2) flushes all data and metadata
> > to underlying disk, if it returns 0.
> 
> Yes.
> 
> > Sorry, really confused on what these sync things
> > really guarantee :)
> 
> No worries -- the history of the sync variants has been very messy and
> confusing even to people on fsdevel.
> 
> --D
> 
> > 
> > Thanks,
> > Zorro
> > 
> > > +#
> > > +# During a code inspection, I noticed that sync_filesystem ignores the return
> > > +# value of the ->sync_fs calls that it makes.  sync_filesystem, in turn is used
> > > +# by the syncfs(2) syscall to persist filesystem changes to disk.  This means
> > > +# that syncfs(2) does not capture internal filesystem errors that are neither
> > > +# visible from the block device (e.g. media error) nor recorded in s_wb_err.
> > > +# XFS historically returned 0 from ->sync_fs even if there were log failures,
> > > +# so that had to be corrected as well.
> > > +#
> > > +# The kernel commits above fix this problem, so this test tries to trigger the
> > > +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the
> > > +# hope that the EIO generated as a result of the filesystem being shut down is
> > > +# only visible via ->sync_fs.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick shutdown
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_require_xfs_io_command syncfs
> > > +_require_scratch_nocheck
> > > +_require_scratch_shutdown
> > > +
> > > +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't
> > > +# bother checking the filesystem afterwards since we never wrote anything.
> > > +_scratch_mount
> > > +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/xfs/839.out b/tests/xfs/839.out
> > > new file mode 100644
> > > index 00000000..f275cdcc
> > > --- /dev/null
> > > +++ b/tests/xfs/839.out
> > > @@ -0,0 +1,2 @@
> > > +QA output created by 839
> > > +syncfs: Input/output error
> > > 
> > 
> 

Re: [PATCH 1/4] xfs: make sure syncfs(2) passes back super_operations.sync_fs errors

[Darrick J. Wong]

On Thu, Apr 14, 2022 at 10:43:30PM +0800, Zorro Lang wrote:
> On Tue, Apr 12, 2022 at 10:28:53AM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 12, 2022 at 05:37:27PM +0800, Zorro Lang wrote:
> > > On Mon, Apr 11, 2022 at 03:54:37PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > This is a regression test to make sure that nonzero error returns from
> > > > a filesystem's ->sync_fs implementation are actually passed back to
> > > > userspace when the call stack involves syncfs(2).
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  tests/xfs/839     |   42 ++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/xfs/839.out |    2 ++
> > > >  2 files changed, 44 insertions(+)
> > > >  create mode 100755 tests/xfs/839
> > > >  create mode 100644 tests/xfs/839.out
> > > > 
> > > > 
> > > > diff --git a/tests/xfs/839 b/tests/xfs/839
> > > 
> > > This case looks good to me. Just one question, is it possible to be a generic
> > > case? From the code logic, it doesn't use xfs specified operations, but I'm
> > > not sure if other filesystems would like to treat sync_fs return value as XFS.
> > 
> > Other filesystems (ext4 in particular) haven't been fixed to make
> > ->sync_fs return error codes when the fs has been shut down via
> > FS_IOC_SHUTDOWN.  We'll get there eventually, but for now I'd like to
> > get this under test for XFS since we've applied those fixes.
> 
> If other filesystems intend to do that, how about using a generic case failure to
> remind them they haven't done that :) If they don't intend that, keep this case
> under xfs is good to me.

<shrug> I don't know if they do or not; I've been so strapped for time
trying to get all these fixes out that I haven't had time to ask the
ext4 or btrfs communities, let alone propose patches.

At the moment I'd really like to get as many patches out of djwong-dev
as I can without people asking for more side projects.  As it stands
today, landing the online fsck patchset is going to involve getting 185
kernel patches, 95 xfsprogs patches, and 87 fstests patches all through
review.

--D

> > 
> > > > new file mode 100755
> > > > index 00000000..9bfe93ef
> > > > --- /dev/null
> > > > +++ b/tests/xfs/839
> > > > @@ -0,0 +1,42 @@
> > > > +#! /bin/bash
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > > +#
> > > > +# FS QA Test No. 839
> > > > +#
> > > > +# Regression test for kernel commits:
> > > > +#
> > > > +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
> > > > +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs")
> > > 
> > > BTW, after this change, now can I assume that sync(2) flushes all data and metadata
> > > to underlying disk, if it returns 0.
> > 
> > Yes.
> > 
> > > Sorry, really confused on what these sync things
> > > really guarantee :)
> > 
> > No worries -- the history of the sync variants has been very messy and
> > confusing even to people on fsdevel.
> > 
> > --D
> > 
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > > +#
> > > > +# During a code inspection, I noticed that sync_filesystem ignores the return
> > > > +# value of the ->sync_fs calls that it makes.  sync_filesystem, in turn is used
> > > > +# by the syncfs(2) syscall to persist filesystem changes to disk.  This means
> > > > +# that syncfs(2) does not capture internal filesystem errors that are neither
> > > > +# visible from the block device (e.g. media error) nor recorded in s_wb_err.
> > > > +# XFS historically returned 0 from ->sync_fs even if there were log failures,
> > > > +# so that had to be corrected as well.
> > > > +#
> > > > +# The kernel commits above fix this problem, so this test tries to trigger the
> > > > +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the
> > > > +# hope that the EIO generated as a result of the filesystem being shut down is
> > > > +# only visible via ->sync_fs.
> > > > +#
> > > > +. ./common/preamble
> > > > +_begin_fstest auto quick shutdown
> > > > +
> > > > +# real QA test starts here
> > > > +
> > > > +# Modify as appropriate.
> > > > +_require_xfs_io_command syncfs
> > > > +_require_scratch_nocheck
> > > > +_require_scratch_shutdown
> > > > +
> > > > +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't
> > > > +# bother checking the filesystem afterwards since we never wrote anything.
> > > > +_scratch_mount
> > > > +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
> > > > +
> > > > +# success, all done
> > > > +status=0
> > > > +exit
> > > > diff --git a/tests/xfs/839.out b/tests/xfs/839.out
> > > > new file mode 100644
> > > > index 00000000..f275cdcc
> > > > --- /dev/null
> > > > +++ b/tests/xfs/839.out
> > > > @@ -0,0 +1,2 @@
> > > > +QA output created by 839
> > > > +syncfs: Input/output error
> > > > 
> > > 
> > 
> 

Re: [PATCH 1/4] xfs: make sure syncfs(2) passes back super_operations.sync_fs errors

[Zorro Lang]

On Thu, Apr 14, 2022 at 08:42:34AM -0700, Darrick J. Wong wrote:
> On Thu, Apr 14, 2022 at 10:43:30PM +0800, Zorro Lang wrote:
> > On Tue, Apr 12, 2022 at 10:28:53AM -0700, Darrick J. Wong wrote:
> > > On Tue, Apr 12, 2022 at 05:37:27PM +0800, Zorro Lang wrote:
> > > > On Mon, Apr 11, 2022 at 03:54:37PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > 
> > > > > This is a regression test to make sure that nonzero error returns from
> > > > > a filesystem's ->sync_fs implementation are actually passed back to
> > > > > userspace when the call stack involves syncfs(2).
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > >  tests/xfs/839     |   42 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/xfs/839.out |    2 ++
> > > > >  2 files changed, 44 insertions(+)
> > > > >  create mode 100755 tests/xfs/839
> > > > >  create mode 100644 tests/xfs/839.out
> > > > > 
> > > > > 
> > > > > diff --git a/tests/xfs/839 b/tests/xfs/839
> > > > 
> > > > This case looks good to me. Just one question, is it possible to be a generic
> > > > case? From the code logic, it doesn't use xfs specified operations, but I'm
> > > > not sure if other filesystems would like to treat sync_fs return value as XFS.
> > > 
> > > Other filesystems (ext4 in particular) haven't been fixed to make
> > > ->sync_fs return error codes when the fs has been shut down via
> > > FS_IOC_SHUTDOWN.  We'll get there eventually, but for now I'd like to
> > > get this under test for XFS since we've applied those fixes.
> > 
> > If other filesystems intend to do that, how about using a generic case failure to
> > remind them they haven't done that :) If they don't intend that, keep this case
> > under xfs is good to me.
> 
> <shrug> I don't know if they do or not; I've been so strapped for time
> trying to get all these fixes out that I haven't had time to ask the
> ext4 or btrfs communities, let alone propose patches.
> 
> At the moment I'd really like to get as many patches out of djwong-dev
> as I can without people asking for more side projects.  As it stands
> today, landing the online fsck patchset is going to involve getting 185
> kernel patches, 95 xfsprogs patches, and 87 fstests patches all through
> review.

Sure, this case can be a xfs specified case at first. We'll see if need to
change it to be generic case later.

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
> --D
> 
> > > 
> > > > > new file mode 100755
> > > > > index 00000000..9bfe93ef
> > > > > --- /dev/null
> > > > > +++ b/tests/xfs/839
> > > > > @@ -0,0 +1,42 @@
> > > > > +#! /bin/bash
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > > > +#
> > > > > +# FS QA Test No. 839
> > > > > +#
> > > > > +# Regression test for kernel commits:
> > > > > +#
> > > > > +# 5679897eb104 ("vfs: make sync_filesystem return errors from ->sync_fs")
> > > > > +# 2d86293c7075 ("xfs: return errors in xfs_fs_sync_fs")
> > > > 
> > > > BTW, after this change, now can I assume that sync(2) flushes all data and metadata
> > > > to underlying disk, if it returns 0.
> > > 
> > > Yes.
> > > 
> > > > Sorry, really confused on what these sync things
> > > > really guarantee :)
> > > 
> > > No worries -- the history of the sync variants has been very messy and
> > > confusing even to people on fsdevel.
> > > 
> > > --D
> > > 
> > > > 
> > > > Thanks,
> > > > Zorro
> > > > 
> > > > > +#
> > > > > +# During a code inspection, I noticed that sync_filesystem ignores the return
> > > > > +# value of the ->sync_fs calls that it makes.  sync_filesystem, in turn is used
> > > > > +# by the syncfs(2) syscall to persist filesystem changes to disk.  This means
> > > > > +# that syncfs(2) does not capture internal filesystem errors that are neither
> > > > > +# visible from the block device (e.g. media error) nor recorded in s_wb_err.
> > > > > +# XFS historically returned 0 from ->sync_fs even if there were log failures,
> > > > > +# so that had to be corrected as well.
> > > > > +#
> > > > > +# The kernel commits above fix this problem, so this test tries to trigger the
> > > > > +# bug by using the shutdown ioctl on a clean, freshly mounted filesystem in the
> > > > > +# hope that the EIO generated as a result of the filesystem being shut down is
> > > > > +# only visible via ->sync_fs.
> > > > > +#
> > > > > +. ./common/preamble
> > > > > +_begin_fstest auto quick shutdown
> > > > > +
> > > > > +# real QA test starts here
> > > > > +
> > > > > +# Modify as appropriate.
> > > > > +_require_xfs_io_command syncfs
> > > > > +_require_scratch_nocheck
> > > > > +_require_scratch_shutdown
> > > > > +
> > > > > +# Reuse the fs formatted when we checked for the shutdown ioctl, and don't
> > > > > +# bother checking the filesystem afterwards since we never wrote anything.
> > > > > +_scratch_mount
> > > > > +$XFS_IO_PROG -x -c 'shutdown -f ' -c syncfs $SCRATCH_MNT
> > > > > +
> > > > > +# success, all done
> > > > > +status=0
> > > > > +exit
> > > > > diff --git a/tests/xfs/839.out b/tests/xfs/839.out
> > > > > new file mode 100644
> > > > > index 00000000..f275cdcc
> > > > > --- /dev/null
> > > > > +++ b/tests/xfs/839.out
> > > > > @@ -0,0 +1,2 @@
> > > > > +QA output created by 839
> > > > > +syncfs: Input/output error
> > > > > 
> > > > 
> > > 
> > 
> 

[PATCH 2/4] generic: ensure we drop suid after fallocate

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

fallocate changes file contents, so make sure that we drop privileges
and file capabilities after each fallocate operation.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/834.out |   33 +++++++++++++
 tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/835.out |   33 +++++++++++++
 tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/836.out |   33 +++++++++++++
 tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/837.out |   33 +++++++++++++
 tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/838.out |   33 +++++++++++++
 tests/generic/839     |   77 ++++++++++++++++++++++++++++++
 tests/generic/839.out |   13 +++++
 12 files changed, 890 insertions(+)
 create mode 100755 tests/generic/834
 create mode 100644 tests/generic/834.out
 create mode 100755 tests/generic/835
 create mode 100644 tests/generic/835.out
 create mode 100755 tests/generic/836
 create mode 100644 tests/generic/836.out
 create mode 100755 tests/generic/837
 create mode 100644 tests/generic/837.out
 create mode 100755 tests/generic/838
 create mode 100644 tests/generic/838.out
 create mode 100755 tests/generic/839
 create mode 100755 tests/generic/839.out


diff --git a/tests/generic/834 b/tests/generic/834
new file mode 100755
index 00000000..9302137b
--- /dev/null
+++ b/tests/generic/834
@@ -0,0 +1,127 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 834
+#
+# Functional test for dropping suid and sgid bits as part of a fallocate.
+#
+. ./common/preamble
+_begin_fstest auto clone quick
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.* $junk_dir
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/reflink
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs btrfs ext4
+_require_user
+_require_test
+verb=falloc
+_require_xfs_io_command $verb
+
+junk_dir=$TEST_DIR/$seq
+junk_file=$junk_dir/a
+mkdir -p $junk_dir/
+chmod a+rw $junk_dir/
+
+setup_testfile() {
+	rm -f $junk_file
+	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
+	sync
+}
+
+commit_and_check() {
+	local user="$1"
+	local command="$2"
+	local start="$3"
+	local end="$4"
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
+	if [ -n "$user" ]; then
+		su - "$user" -c "$cmd" >> $seqres.full
+	else
+		$SHELL -c "$cmd" >> $seqres.full
+	fi
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	# Blank line in output
+	echo
+}
+
+nr=0
+# Commit to a non-exec file by an unprivileged user clears suid but
+# leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a group-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a user-exec file by an unprivileged user clears suid but
+# not sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a all-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a non-exec file by root clears suid but leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a group-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a user-exec file by root clears suid but not sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a all-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/834.out b/tests/generic/834.out
new file mode 100644
index 00000000..2226eea6
--- /dev/null
+++ b/tests/generic/834.out
@@ -0,0 +1,33 @@
+QA output created by 834
+Test 1 - qa_user, non-exec file falloc
+6666 -rwSrwSrw- TEST_DIR/834/a
+666 -rw-rw-rw- TEST_DIR/834/a
+
+Test 2 - qa_user, group-exec file falloc
+6676 -rwSrwsrw- TEST_DIR/834/a
+676 -rw-rwxrw- TEST_DIR/834/a
+
+Test 3 - qa_user, user-exec file falloc
+6766 -rwsrwSrw- TEST_DIR/834/a
+766 -rwxrw-rw- TEST_DIR/834/a
+
+Test 4 - qa_user, all-exec file falloc
+6777 -rwsrwsrwx TEST_DIR/834/a
+777 -rwxrwxrwx TEST_DIR/834/a
+
+Test 5 - root, non-exec file falloc
+6666 -rwSrwSrw- TEST_DIR/834/a
+6666 -rwSrwSrw- TEST_DIR/834/a
+
+Test 6 - root, group-exec file falloc
+6676 -rwSrwsrw- TEST_DIR/834/a
+6676 -rwSrwsrw- TEST_DIR/834/a
+
+Test 7 - root, user-exec file falloc
+6766 -rwsrwSrw- TEST_DIR/834/a
+6766 -rwsrwSrw- TEST_DIR/834/a
+
+Test 8 - root, all-exec file falloc
+6777 -rwsrwsrwx TEST_DIR/834/a
+6777 -rwsrwsrwx TEST_DIR/834/a
+
diff --git a/tests/generic/835 b/tests/generic/835
new file mode 100755
index 00000000..b0dc9cc3
--- /dev/null
+++ b/tests/generic/835
@@ -0,0 +1,127 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 835
+#
+# Functional test for dropping suid and sgid bits as part of a fpunch.
+#
+. ./common/preamble
+_begin_fstest auto clone quick
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.* $junk_dir
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/reflink
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs btrfs ext4
+_require_user
+_require_test
+verb=fpunch
+_require_xfs_io_command $verb
+
+junk_dir=$TEST_DIR/$seq
+junk_file=$junk_dir/a
+mkdir -p $junk_dir/
+chmod a+rw $junk_dir/
+
+setup_testfile() {
+	rm -f $junk_file
+	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
+	sync
+}
+
+commit_and_check() {
+	local user="$1"
+	local command="$2"
+	local start="$3"
+	local end="$4"
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
+	if [ -n "$user" ]; then
+		su - "$user" -c "$cmd" >> $seqres.full
+	else
+		$SHELL -c "$cmd" >> $seqres.full
+	fi
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	# Blank line in output
+	echo
+}
+
+nr=0
+# Commit to a non-exec file by an unprivileged user clears suid but
+# leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a group-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a user-exec file by an unprivileged user clears suid but
+# not sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a all-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a non-exec file by root clears suid but leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a group-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a user-exec file by root clears suid but not sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a all-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/835.out b/tests/generic/835.out
new file mode 100644
index 00000000..186d7da4
--- /dev/null
+++ b/tests/generic/835.out
@@ -0,0 +1,33 @@
+QA output created by 835
+Test 1 - qa_user, non-exec file fpunch
+6666 -rwSrwSrw- TEST_DIR/835/a
+666 -rw-rw-rw- TEST_DIR/835/a
+
+Test 2 - qa_user, group-exec file fpunch
+6676 -rwSrwsrw- TEST_DIR/835/a
+676 -rw-rwxrw- TEST_DIR/835/a
+
+Test 3 - qa_user, user-exec file fpunch
+6766 -rwsrwSrw- TEST_DIR/835/a
+766 -rwxrw-rw- TEST_DIR/835/a
+
+Test 4 - qa_user, all-exec file fpunch
+6777 -rwsrwsrwx TEST_DIR/835/a
+777 -rwxrwxrwx TEST_DIR/835/a
+
+Test 5 - root, non-exec file fpunch
+6666 -rwSrwSrw- TEST_DIR/835/a
+6666 -rwSrwSrw- TEST_DIR/835/a
+
+Test 6 - root, group-exec file fpunch
+6676 -rwSrwsrw- TEST_DIR/835/a
+6676 -rwSrwsrw- TEST_DIR/835/a
+
+Test 7 - root, user-exec file fpunch
+6766 -rwsrwSrw- TEST_DIR/835/a
+6766 -rwsrwSrw- TEST_DIR/835/a
+
+Test 8 - root, all-exec file fpunch
+6777 -rwsrwsrwx TEST_DIR/835/a
+6777 -rwsrwsrwx TEST_DIR/835/a
+
diff --git a/tests/generic/836 b/tests/generic/836
new file mode 100755
index 00000000..ff0cf092
--- /dev/null
+++ b/tests/generic/836
@@ -0,0 +1,127 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 836
+#
+# Functional test for dropping suid and sgid bits as part of a fzero.
+#
+. ./common/preamble
+_begin_fstest auto clone quick
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.* $junk_dir
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/reflink
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs btrfs ext4
+_require_user
+_require_test
+verb=fzero
+_require_xfs_io_command $verb
+
+junk_dir=$TEST_DIR/$seq
+junk_file=$junk_dir/a
+mkdir -p $junk_dir/
+chmod a+rw $junk_dir/
+
+setup_testfile() {
+	rm -f $junk_file
+	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
+	sync
+}
+
+commit_and_check() {
+	local user="$1"
+	local command="$2"
+	local start="$3"
+	local end="$4"
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
+	if [ -n "$user" ]; then
+		su - "$user" -c "$cmd" >> $seqres.full
+	else
+		$SHELL -c "$cmd" >> $seqres.full
+	fi
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	# Blank line in output
+	echo
+}
+
+nr=0
+# Commit to a non-exec file by an unprivileged user clears suid but
+# leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a group-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a user-exec file by an unprivileged user clears suid but
+# not sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a all-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a non-exec file by root clears suid but leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a group-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a user-exec file by root clears suid but not sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a all-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/836.out b/tests/generic/836.out
new file mode 100644
index 00000000..9f9f5f12
--- /dev/null
+++ b/tests/generic/836.out
@@ -0,0 +1,33 @@
+QA output created by 836
+Test 1 - qa_user, non-exec file fzero
+6666 -rwSrwSrw- TEST_DIR/836/a
+666 -rw-rw-rw- TEST_DIR/836/a
+
+Test 2 - qa_user, group-exec file fzero
+6676 -rwSrwsrw- TEST_DIR/836/a
+676 -rw-rwxrw- TEST_DIR/836/a
+
+Test 3 - qa_user, user-exec file fzero
+6766 -rwsrwSrw- TEST_DIR/836/a
+766 -rwxrw-rw- TEST_DIR/836/a
+
+Test 4 - qa_user, all-exec file fzero
+6777 -rwsrwsrwx TEST_DIR/836/a
+777 -rwxrwxrwx TEST_DIR/836/a
+
+Test 5 - root, non-exec file fzero
+6666 -rwSrwSrw- TEST_DIR/836/a
+6666 -rwSrwSrw- TEST_DIR/836/a
+
+Test 6 - root, group-exec file fzero
+6676 -rwSrwsrw- TEST_DIR/836/a
+6676 -rwSrwsrw- TEST_DIR/836/a
+
+Test 7 - root, user-exec file fzero
+6766 -rwsrwSrw- TEST_DIR/836/a
+6766 -rwsrwSrw- TEST_DIR/836/a
+
+Test 8 - root, all-exec file fzero
+6777 -rwsrwsrwx TEST_DIR/836/a
+6777 -rwsrwsrwx TEST_DIR/836/a
+
diff --git a/tests/generic/837 b/tests/generic/837
new file mode 100755
index 00000000..477b8f21
--- /dev/null
+++ b/tests/generic/837
@@ -0,0 +1,127 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 837
+#
+# Functional test for dropping suid and sgid bits as part of a finsert.
+#
+. ./common/preamble
+_begin_fstest auto clone quick
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.* $junk_dir
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/reflink
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs btrfs ext4
+_require_user
+_require_test
+verb=finsert
+_require_xfs_io_command $verb
+
+junk_dir=$TEST_DIR/$seq
+junk_file=$junk_dir/a
+mkdir -p $junk_dir/
+chmod a+rw $junk_dir/
+
+setup_testfile() {
+	rm -f $junk_file
+	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
+	sync
+}
+
+commit_and_check() {
+	local user="$1"
+	local command="$2"
+	local start="$3"
+	local end="$4"
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
+	if [ -n "$user" ]; then
+		su - "$user" -c "$cmd" >> $seqres.full
+	else
+		$SHELL -c "$cmd" >> $seqres.full
+	fi
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	# Blank line in output
+	echo
+}
+
+nr=0
+# Commit to a non-exec file by an unprivileged user clears suid but
+# leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a group-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a user-exec file by an unprivileged user clears suid but
+# not sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a all-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a non-exec file by root clears suid but leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a group-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a user-exec file by root clears suid but not sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a all-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/837.out b/tests/generic/837.out
new file mode 100644
index 00000000..686b806e
--- /dev/null
+++ b/tests/generic/837.out
@@ -0,0 +1,33 @@
+QA output created by 837
+Test 1 - qa_user, non-exec file finsert
+6666 -rwSrwSrw- TEST_DIR/837/a
+666 -rw-rw-rw- TEST_DIR/837/a
+
+Test 2 - qa_user, group-exec file finsert
+6676 -rwSrwsrw- TEST_DIR/837/a
+676 -rw-rwxrw- TEST_DIR/837/a
+
+Test 3 - qa_user, user-exec file finsert
+6766 -rwsrwSrw- TEST_DIR/837/a
+766 -rwxrw-rw- TEST_DIR/837/a
+
+Test 4 - qa_user, all-exec file finsert
+6777 -rwsrwsrwx TEST_DIR/837/a
+777 -rwxrwxrwx TEST_DIR/837/a
+
+Test 5 - root, non-exec file finsert
+6666 -rwSrwSrw- TEST_DIR/837/a
+6666 -rwSrwSrw- TEST_DIR/837/a
+
+Test 6 - root, group-exec file finsert
+6676 -rwSrwsrw- TEST_DIR/837/a
+6676 -rwSrwsrw- TEST_DIR/837/a
+
+Test 7 - root, user-exec file finsert
+6766 -rwsrwSrw- TEST_DIR/837/a
+6766 -rwsrwSrw- TEST_DIR/837/a
+
+Test 8 - root, all-exec file finsert
+6777 -rwsrwsrwx TEST_DIR/837/a
+6777 -rwsrwsrwx TEST_DIR/837/a
+
diff --git a/tests/generic/838 b/tests/generic/838
new file mode 100755
index 00000000..d7c7e5a8
--- /dev/null
+++ b/tests/generic/838
@@ -0,0 +1,127 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 838
+#
+# Functional test for dropping suid and sgid bits as part of a fcollapse.
+#
+. ./common/preamble
+_begin_fstest auto clone quick
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.* $junk_dir
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/reflink
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs btrfs ext4
+_require_user
+_require_test
+verb=fcollapse
+_require_xfs_io_command $verb
+
+junk_dir=$TEST_DIR/$seq
+junk_file=$junk_dir/a
+mkdir -p $junk_dir/
+chmod a+rw $junk_dir/
+
+setup_testfile() {
+	rm -f $junk_file
+	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
+	sync
+}
+
+commit_and_check() {
+	local user="$1"
+	local command="$2"
+	local start="$3"
+	local end="$4"
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
+	if [ -n "$user" ]; then
+		su - "$user" -c "$cmd" >> $seqres.full
+	else
+		$SHELL -c "$cmd" >> $seqres.full
+	fi
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+
+	# Blank line in output
+	echo
+}
+
+nr=0
+# Commit to a non-exec file by an unprivileged user clears suid but
+# leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a group-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a user-exec file by an unprivileged user clears suid but
+# not sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a all-exec file by an unprivileged user clears suid and
+# sgid.
+nr=$((nr + 1))
+echo "Test $nr - qa_user, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "$qa_user" "$verb" 64k 64k
+
+# Commit to a non-exec file by root clears suid but leaves sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, non-exec file $verb"
+setup_testfile
+chmod a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a group-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, group-exec file $verb"
+setup_testfile
+chmod g+x,a+rws $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a user-exec file by root clears suid but not sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, user-exec file $verb"
+setup_testfile
+chmod u+x,a+rws,g-x $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# Commit to a all-exec file by root clears suid and sgid.
+nr=$((nr + 1))
+echo "Test $nr - root, all-exec file $verb"
+setup_testfile
+chmod a+rwxs $junk_file
+commit_and_check "" "$verb" 64k 64k
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/838.out b/tests/generic/838.out
new file mode 100644
index 00000000..cdc29f4b
--- /dev/null
+++ b/tests/generic/838.out
@@ -0,0 +1,33 @@
+QA output created by 838
+Test 1 - qa_user, non-exec file fcollapse
+6666 -rwSrwSrw- TEST_DIR/838/a
+666 -rw-rw-rw- TEST_DIR/838/a
+
+Test 2 - qa_user, group-exec file fcollapse
+6676 -rwSrwsrw- TEST_DIR/838/a
+676 -rw-rwxrw- TEST_DIR/838/a
+
+Test 3 - qa_user, user-exec file fcollapse
+6766 -rwsrwSrw- TEST_DIR/838/a
+766 -rwxrw-rw- TEST_DIR/838/a
+
+Test 4 - qa_user, all-exec file fcollapse
+6777 -rwsrwsrwx TEST_DIR/838/a
+777 -rwxrwxrwx TEST_DIR/838/a
+
+Test 5 - root, non-exec file fcollapse
+6666 -rwSrwSrw- TEST_DIR/838/a
+6666 -rwSrwSrw- TEST_DIR/838/a
+
+Test 6 - root, group-exec file fcollapse
+6676 -rwSrwsrw- TEST_DIR/838/a
+6676 -rwSrwsrw- TEST_DIR/838/a
+
+Test 7 - root, user-exec file fcollapse
+6766 -rwsrwSrw- TEST_DIR/838/a
+6766 -rwsrwSrw- TEST_DIR/838/a
+
+Test 8 - root, all-exec file fcollapse
+6777 -rwsrwsrwx TEST_DIR/838/a
+6777 -rwsrwsrwx TEST_DIR/838/a
+
diff --git a/tests/generic/839 b/tests/generic/839
new file mode 100755
index 00000000..70bef5fc
--- /dev/null
+++ b/tests/generic/839
@@ -0,0 +1,77 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 839
+#
+# Functional test for dropping capability bits as part of an fallocate.
+#
+. ./common/preamble
+_begin_fstest auto prealloc quick
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.* $junk_dir
+}
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs xfs btrfs ext4
+_require_user
+_require_command "$GETCAP_PROG" getcap
+_require_command "$SETCAP_PROG" setcap
+_require_xfs_io_command falloc
+_require_test
+
+junk_dir=$TEST_DIR/$seq
+junk_file=$junk_dir/a
+mkdir -p $junk_dir/
+chmod a+rw $junk_dir/
+
+setup_testfile() {
+	rm -f $junk_file
+	touch $junk_file
+	chmod a+rwx $junk_file
+	$SETCAP_PROG cap_setgid,cap_setuid+ep $junk_file
+	sync
+}
+
+commit_and_check() {
+	local user="$1"
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+	_getcap -v $junk_file | _filter_test_dir
+
+	local cmd="$XFS_IO_PROG -c 'falloc 0 64k' $junk_file"
+	if [ -n "$user" ]; then
+		su - "$user" -c "$cmd" >> $seqres.full
+	else
+		$SHELL -c "$cmd" >> $seqres.full
+	fi
+
+	stat -c '%a %A %n' $junk_file | _filter_test_dir
+	_getcap -v $junk_file | _filter_test_dir
+
+	# Blank line in output
+	echo
+}
+
+# Commit by an unprivileged user clears capability bits.
+echo "Test 1 - qa_user"
+setup_testfile
+commit_and_check "$qa_user"
+
+# Commit by root leaves capability bits.
+echo "Test 2 - root"
+setup_testfile
+commit_and_check
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/839.out b/tests/generic/839.out
new file mode 100755
index 00000000..f571cd26
--- /dev/null
+++ b/tests/generic/839.out
@@ -0,0 +1,13 @@
+QA output created by 839
+Test 1 - qa_user
+777 -rwxrwxrwx TEST_DIR/839/a
+TEST_DIR/839/a cap_setgid,cap_setuid=ep
+777 -rwxrwxrwx TEST_DIR/839/a
+TEST_DIR/839/a
+
+Test 2 - root
+777 -rwxrwxrwx TEST_DIR/839/a
+TEST_DIR/839/a cap_setgid,cap_setuid=ep
+777 -rwxrwxrwx TEST_DIR/839/a
+TEST_DIR/839/a
+

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Zorro Lang]

On Mon, Apr 11, 2022 at 03:54:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> fallocate changes file contents, so make sure that we drop privileges
> and file capabilities after each fallocate operation.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/834.out |   33 +++++++++++++
>  tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/835.out |   33 +++++++++++++
>  tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/836.out |   33 +++++++++++++
>  tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/837.out |   33 +++++++++++++
>  tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/838.out |   33 +++++++++++++
>  tests/generic/839     |   77 ++++++++++++++++++++++++++++++
>  tests/generic/839.out |   13 +++++
>  12 files changed, 890 insertions(+)
>  create mode 100755 tests/generic/834
>  create mode 100644 tests/generic/834.out
>  create mode 100755 tests/generic/835
>  create mode 100644 tests/generic/835.out
>  create mode 100755 tests/generic/836
>  create mode 100644 tests/generic/836.out
>  create mode 100755 tests/generic/837
>  create mode 100644 tests/generic/837.out
>  create mode 100755 tests/generic/838
>  create mode 100644 tests/generic/838.out
>  create mode 100755 tests/generic/839
>  create mode 100755 tests/generic/839.out
> 
> 
> diff --git a/tests/generic/834 b/tests/generic/834
> new file mode 100755
> index 00000000..9302137b
> --- /dev/null
> +++ b/tests/generic/834
> @@ -0,0 +1,127 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 834
> +#
> +# Functional test for dropping suid and sgid bits as part of a fallocate.
> +#
> +. ./common/preamble
> +_begin_fstest auto clone quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.* $junk_dir
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/reflink
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs btrfs ext4

So we have more cases will break downstream XFS testing :) All cases
looks good, but according to the custom, all generic cases use
"_supported_fs generic", if you have 1+ specified filesystems, maybe
"tests/shared/*" is better?

> +_require_user
> +_require_test
> +verb=falloc
> +_require_xfs_io_command $verb
> +
> +junk_dir=$TEST_DIR/$seq
> +junk_file=$junk_dir/a
> +mkdir -p $junk_dir/
> +chmod a+rw $junk_dir/
> +
> +setup_testfile() {
> +	rm -f $junk_file
> +	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
> +	sync
> +}
> +
> +commit_and_check() {
> +	local user="$1"
> +	local command="$2"
> +	local start="$3"
> +	local end="$4"
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
> +	if [ -n "$user" ]; then
> +		su - "$user" -c "$cmd" >> $seqres.full
> +	else
> +		$SHELL -c "$cmd" >> $seqres.full
> +	fi
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	# Blank line in output
> +	echo
> +}
> +
> +nr=0
> +# Commit to a non-exec file by an unprivileged user clears suid but
> +# leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by an unprivileged user clears suid but
> +# not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a non-exec file by root clears suid but leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by root clears suid but not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/834.out b/tests/generic/834.out
> new file mode 100644
> index 00000000..2226eea6
> --- /dev/null
> +++ b/tests/generic/834.out
> @@ -0,0 +1,33 @@
> +QA output created by 834
> +Test 1 - qa_user, non-exec file falloc
> +6666 -rwSrwSrw- TEST_DIR/834/a
> +666 -rw-rw-rw- TEST_DIR/834/a
> +
> +Test 2 - qa_user, group-exec file falloc
> +6676 -rwSrwsrw- TEST_DIR/834/a
> +676 -rw-rwxrw- TEST_DIR/834/a
> +
> +Test 3 - qa_user, user-exec file falloc
> +6766 -rwsrwSrw- TEST_DIR/834/a
> +766 -rwxrw-rw- TEST_DIR/834/a
> +
> +Test 4 - qa_user, all-exec file falloc
> +6777 -rwsrwsrwx TEST_DIR/834/a
> +777 -rwxrwxrwx TEST_DIR/834/a
> +
> +Test 5 - root, non-exec file falloc
> +6666 -rwSrwSrw- TEST_DIR/834/a
> +6666 -rwSrwSrw- TEST_DIR/834/a
> +
> +Test 6 - root, group-exec file falloc
> +6676 -rwSrwsrw- TEST_DIR/834/a
> +6676 -rwSrwsrw- TEST_DIR/834/a
> +
> +Test 7 - root, user-exec file falloc
> +6766 -rwsrwSrw- TEST_DIR/834/a
> +6766 -rwsrwSrw- TEST_DIR/834/a
> +
> +Test 8 - root, all-exec file falloc
> +6777 -rwsrwsrwx TEST_DIR/834/a
> +6777 -rwsrwsrwx TEST_DIR/834/a
> +
> diff --git a/tests/generic/835 b/tests/generic/835
> new file mode 100755
> index 00000000..b0dc9cc3
> --- /dev/null
> +++ b/tests/generic/835
> @@ -0,0 +1,127 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 835
> +#
> +# Functional test for dropping suid and sgid bits as part of a fpunch.
> +#
> +. ./common/preamble
> +_begin_fstest auto clone quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.* $junk_dir
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/reflink
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs btrfs ext4
> +_require_user
> +_require_test
> +verb=fpunch
> +_require_xfs_io_command $verb
> +
> +junk_dir=$TEST_DIR/$seq
> +junk_file=$junk_dir/a
> +mkdir -p $junk_dir/
> +chmod a+rw $junk_dir/
> +
> +setup_testfile() {
> +	rm -f $junk_file
> +	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
> +	sync
> +}
> +
> +commit_and_check() {
> +	local user="$1"
> +	local command="$2"
> +	local start="$3"
> +	local end="$4"
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
> +	if [ -n "$user" ]; then
> +		su - "$user" -c "$cmd" >> $seqres.full
> +	else
> +		$SHELL -c "$cmd" >> $seqres.full
> +	fi
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	# Blank line in output
> +	echo
> +}
> +
> +nr=0
> +# Commit to a non-exec file by an unprivileged user clears suid but
> +# leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by an unprivileged user clears suid but
> +# not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a non-exec file by root clears suid but leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by root clears suid but not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/835.out b/tests/generic/835.out
> new file mode 100644
> index 00000000..186d7da4
> --- /dev/null
> +++ b/tests/generic/835.out
> @@ -0,0 +1,33 @@
> +QA output created by 835
> +Test 1 - qa_user, non-exec file fpunch
> +6666 -rwSrwSrw- TEST_DIR/835/a
> +666 -rw-rw-rw- TEST_DIR/835/a
> +
> +Test 2 - qa_user, group-exec file fpunch
> +6676 -rwSrwsrw- TEST_DIR/835/a
> +676 -rw-rwxrw- TEST_DIR/835/a
> +
> +Test 3 - qa_user, user-exec file fpunch
> +6766 -rwsrwSrw- TEST_DIR/835/a
> +766 -rwxrw-rw- TEST_DIR/835/a
> +
> +Test 4 - qa_user, all-exec file fpunch
> +6777 -rwsrwsrwx TEST_DIR/835/a
> +777 -rwxrwxrwx TEST_DIR/835/a
> +
> +Test 5 - root, non-exec file fpunch
> +6666 -rwSrwSrw- TEST_DIR/835/a
> +6666 -rwSrwSrw- TEST_DIR/835/a
> +
> +Test 6 - root, group-exec file fpunch
> +6676 -rwSrwsrw- TEST_DIR/835/a
> +6676 -rwSrwsrw- TEST_DIR/835/a
> +
> +Test 7 - root, user-exec file fpunch
> +6766 -rwsrwSrw- TEST_DIR/835/a
> +6766 -rwsrwSrw- TEST_DIR/835/a
> +
> +Test 8 - root, all-exec file fpunch
> +6777 -rwsrwsrwx TEST_DIR/835/a
> +6777 -rwsrwsrwx TEST_DIR/835/a
> +
> diff --git a/tests/generic/836 b/tests/generic/836
> new file mode 100755
> index 00000000..ff0cf092
> --- /dev/null
> +++ b/tests/generic/836
> @@ -0,0 +1,127 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 836
> +#
> +# Functional test for dropping suid and sgid bits as part of a fzero.
> +#
> +. ./common/preamble
> +_begin_fstest auto clone quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.* $junk_dir
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/reflink
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs btrfs ext4
> +_require_user
> +_require_test
> +verb=fzero
> +_require_xfs_io_command $verb
> +
> +junk_dir=$TEST_DIR/$seq
> +junk_file=$junk_dir/a
> +mkdir -p $junk_dir/
> +chmod a+rw $junk_dir/
> +
> +setup_testfile() {
> +	rm -f $junk_file
> +	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
> +	sync
> +}
> +
> +commit_and_check() {
> +	local user="$1"
> +	local command="$2"
> +	local start="$3"
> +	local end="$4"
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
> +	if [ -n "$user" ]; then
> +		su - "$user" -c "$cmd" >> $seqres.full
> +	else
> +		$SHELL -c "$cmd" >> $seqres.full
> +	fi
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	# Blank line in output
> +	echo
> +}
> +
> +nr=0
> +# Commit to a non-exec file by an unprivileged user clears suid but
> +# leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by an unprivileged user clears suid but
> +# not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a non-exec file by root clears suid but leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by root clears suid but not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/836.out b/tests/generic/836.out
> new file mode 100644
> index 00000000..9f9f5f12
> --- /dev/null
> +++ b/tests/generic/836.out
> @@ -0,0 +1,33 @@
> +QA output created by 836
> +Test 1 - qa_user, non-exec file fzero
> +6666 -rwSrwSrw- TEST_DIR/836/a
> +666 -rw-rw-rw- TEST_DIR/836/a
> +
> +Test 2 - qa_user, group-exec file fzero
> +6676 -rwSrwsrw- TEST_DIR/836/a
> +676 -rw-rwxrw- TEST_DIR/836/a
> +
> +Test 3 - qa_user, user-exec file fzero
> +6766 -rwsrwSrw- TEST_DIR/836/a
> +766 -rwxrw-rw- TEST_DIR/836/a
> +
> +Test 4 - qa_user, all-exec file fzero
> +6777 -rwsrwsrwx TEST_DIR/836/a
> +777 -rwxrwxrwx TEST_DIR/836/a
> +
> +Test 5 - root, non-exec file fzero
> +6666 -rwSrwSrw- TEST_DIR/836/a
> +6666 -rwSrwSrw- TEST_DIR/836/a
> +
> +Test 6 - root, group-exec file fzero
> +6676 -rwSrwsrw- TEST_DIR/836/a
> +6676 -rwSrwsrw- TEST_DIR/836/a
> +
> +Test 7 - root, user-exec file fzero
> +6766 -rwsrwSrw- TEST_DIR/836/a
> +6766 -rwsrwSrw- TEST_DIR/836/a
> +
> +Test 8 - root, all-exec file fzero
> +6777 -rwsrwsrwx TEST_DIR/836/a
> +6777 -rwsrwsrwx TEST_DIR/836/a
> +
> diff --git a/tests/generic/837 b/tests/generic/837
> new file mode 100755
> index 00000000..477b8f21
> --- /dev/null
> +++ b/tests/generic/837
> @@ -0,0 +1,127 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 837
> +#
> +# Functional test for dropping suid and sgid bits as part of a finsert.
> +#
> +. ./common/preamble
> +_begin_fstest auto clone quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.* $junk_dir
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/reflink
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs btrfs ext4
> +_require_user
> +_require_test
> +verb=finsert
> +_require_xfs_io_command $verb
> +
> +junk_dir=$TEST_DIR/$seq
> +junk_file=$junk_dir/a
> +mkdir -p $junk_dir/
> +chmod a+rw $junk_dir/
> +
> +setup_testfile() {
> +	rm -f $junk_file
> +	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
> +	sync
> +}
> +
> +commit_and_check() {
> +	local user="$1"
> +	local command="$2"
> +	local start="$3"
> +	local end="$4"
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
> +	if [ -n "$user" ]; then
> +		su - "$user" -c "$cmd" >> $seqres.full
> +	else
> +		$SHELL -c "$cmd" >> $seqres.full
> +	fi
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	# Blank line in output
> +	echo
> +}
> +
> +nr=0
> +# Commit to a non-exec file by an unprivileged user clears suid but
> +# leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by an unprivileged user clears suid but
> +# not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a non-exec file by root clears suid but leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by root clears suid but not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/837.out b/tests/generic/837.out
> new file mode 100644
> index 00000000..686b806e
> --- /dev/null
> +++ b/tests/generic/837.out
> @@ -0,0 +1,33 @@
> +QA output created by 837
> +Test 1 - qa_user, non-exec file finsert
> +6666 -rwSrwSrw- TEST_DIR/837/a
> +666 -rw-rw-rw- TEST_DIR/837/a
> +
> +Test 2 - qa_user, group-exec file finsert
> +6676 -rwSrwsrw- TEST_DIR/837/a
> +676 -rw-rwxrw- TEST_DIR/837/a
> +
> +Test 3 - qa_user, user-exec file finsert
> +6766 -rwsrwSrw- TEST_DIR/837/a
> +766 -rwxrw-rw- TEST_DIR/837/a
> +
> +Test 4 - qa_user, all-exec file finsert
> +6777 -rwsrwsrwx TEST_DIR/837/a
> +777 -rwxrwxrwx TEST_DIR/837/a
> +
> +Test 5 - root, non-exec file finsert
> +6666 -rwSrwSrw- TEST_DIR/837/a
> +6666 -rwSrwSrw- TEST_DIR/837/a
> +
> +Test 6 - root, group-exec file finsert
> +6676 -rwSrwsrw- TEST_DIR/837/a
> +6676 -rwSrwsrw- TEST_DIR/837/a
> +
> +Test 7 - root, user-exec file finsert
> +6766 -rwsrwSrw- TEST_DIR/837/a
> +6766 -rwsrwSrw- TEST_DIR/837/a
> +
> +Test 8 - root, all-exec file finsert
> +6777 -rwsrwsrwx TEST_DIR/837/a
> +6777 -rwsrwsrwx TEST_DIR/837/a
> +
> diff --git a/tests/generic/838 b/tests/generic/838
> new file mode 100755
> index 00000000..d7c7e5a8
> --- /dev/null
> +++ b/tests/generic/838
> @@ -0,0 +1,127 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 838
> +#
> +# Functional test for dropping suid and sgid bits as part of a fcollapse.
> +#
> +. ./common/preamble
> +_begin_fstest auto clone quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.* $junk_dir
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/reflink
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs btrfs ext4
> +_require_user
> +_require_test
> +verb=fcollapse
> +_require_xfs_io_command $verb
> +
> +junk_dir=$TEST_DIR/$seq
> +junk_file=$junk_dir/a
> +mkdir -p $junk_dir/
> +chmod a+rw $junk_dir/
> +
> +setup_testfile() {
> +	rm -f $junk_file
> +	_pwrite_byte 0x58 0 192k $junk_file >> $seqres.full
> +	sync
> +}
> +
> +commit_and_check() {
> +	local user="$1"
> +	local command="$2"
> +	local start="$3"
> +	local end="$4"
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
> +	if [ -n "$user" ]; then
> +		su - "$user" -c "$cmd" >> $seqres.full
> +	else
> +		$SHELL -c "$cmd" >> $seqres.full
> +	fi
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +
> +	# Blank line in output
> +	echo
> +}
> +
> +nr=0
> +# Commit to a non-exec file by an unprivileged user clears suid but
> +# leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by an unprivileged user clears suid but
> +# not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by an unprivileged user clears suid and
> +# sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - qa_user, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "$qa_user" "$verb" 64k 64k
> +
> +# Commit to a non-exec file by root clears suid but leaves sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, non-exec file $verb"
> +setup_testfile
> +chmod a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a group-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, group-exec file $verb"
> +setup_testfile
> +chmod g+x,a+rws $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a user-exec file by root clears suid but not sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, user-exec file $verb"
> +setup_testfile
> +chmod u+x,a+rws,g-x $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# Commit to a all-exec file by root clears suid and sgid.
> +nr=$((nr + 1))
> +echo "Test $nr - root, all-exec file $verb"
> +setup_testfile
> +chmod a+rwxs $junk_file
> +commit_and_check "" "$verb" 64k 64k
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/838.out b/tests/generic/838.out
> new file mode 100644
> index 00000000..cdc29f4b
> --- /dev/null
> +++ b/tests/generic/838.out
> @@ -0,0 +1,33 @@
> +QA output created by 838
> +Test 1 - qa_user, non-exec file fcollapse
> +6666 -rwSrwSrw- TEST_DIR/838/a
> +666 -rw-rw-rw- TEST_DIR/838/a
> +
> +Test 2 - qa_user, group-exec file fcollapse
> +6676 -rwSrwsrw- TEST_DIR/838/a
> +676 -rw-rwxrw- TEST_DIR/838/a
> +
> +Test 3 - qa_user, user-exec file fcollapse
> +6766 -rwsrwSrw- TEST_DIR/838/a
> +766 -rwxrw-rw- TEST_DIR/838/a
> +
> +Test 4 - qa_user, all-exec file fcollapse
> +6777 -rwsrwsrwx TEST_DIR/838/a
> +777 -rwxrwxrwx TEST_DIR/838/a
> +
> +Test 5 - root, non-exec file fcollapse
> +6666 -rwSrwSrw- TEST_DIR/838/a
> +6666 -rwSrwSrw- TEST_DIR/838/a
> +
> +Test 6 - root, group-exec file fcollapse
> +6676 -rwSrwsrw- TEST_DIR/838/a
> +6676 -rwSrwsrw- TEST_DIR/838/a
> +
> +Test 7 - root, user-exec file fcollapse
> +6766 -rwsrwSrw- TEST_DIR/838/a
> +6766 -rwsrwSrw- TEST_DIR/838/a
> +
> +Test 8 - root, all-exec file fcollapse
> +6777 -rwsrwsrwx TEST_DIR/838/a
> +6777 -rwsrwsrwx TEST_DIR/838/a
> +
> diff --git a/tests/generic/839 b/tests/generic/839
> new file mode 100755
> index 00000000..70bef5fc
> --- /dev/null
> +++ b/tests/generic/839
> @@ -0,0 +1,77 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 839
> +#
> +# Functional test for dropping capability bits as part of an fallocate.
> +#
> +. ./common/preamble
> +_begin_fstest auto prealloc quick
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	rm -r -f $tmp.* $junk_dir
> +}
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs xfs btrfs ext4
> +_require_user
> +_require_command "$GETCAP_PROG" getcap
> +_require_command "$SETCAP_PROG" setcap
> +_require_xfs_io_command falloc
> +_require_test
> +
> +junk_dir=$TEST_DIR/$seq
> +junk_file=$junk_dir/a
> +mkdir -p $junk_dir/
> +chmod a+rw $junk_dir/
> +
> +setup_testfile() {
> +	rm -f $junk_file
> +	touch $junk_file
> +	chmod a+rwx $junk_file
> +	$SETCAP_PROG cap_setgid,cap_setuid+ep $junk_file
> +	sync
> +}
> +
> +commit_and_check() {
> +	local user="$1"
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +	_getcap -v $junk_file | _filter_test_dir
> +
> +	local cmd="$XFS_IO_PROG -c 'falloc 0 64k' $junk_file"
> +	if [ -n "$user" ]; then
> +		su - "$user" -c "$cmd" >> $seqres.full
> +	else
> +		$SHELL -c "$cmd" >> $seqres.full
> +	fi
> +
> +	stat -c '%a %A %n' $junk_file | _filter_test_dir
> +	_getcap -v $junk_file | _filter_test_dir
> +
> +	# Blank line in output
> +	echo
> +}
> +
> +# Commit by an unprivileged user clears capability bits.
> +echo "Test 1 - qa_user"
> +setup_testfile
> +commit_and_check "$qa_user"
> +
> +# Commit by root leaves capability bits.
> +echo "Test 2 - root"
> +setup_testfile
> +commit_and_check
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/839.out b/tests/generic/839.out
> new file mode 100755
> index 00000000..f571cd26
> --- /dev/null
> +++ b/tests/generic/839.out
> @@ -0,0 +1,13 @@
> +QA output created by 839
> +Test 1 - qa_user
> +777 -rwxrwxrwx TEST_DIR/839/a
> +TEST_DIR/839/a cap_setgid,cap_setuid=ep
> +777 -rwxrwxrwx TEST_DIR/839/a
> +TEST_DIR/839/a
> +
> +Test 2 - root
> +777 -rwxrwxrwx TEST_DIR/839/a
> +TEST_DIR/839/a cap_setgid,cap_setuid=ep
> +777 -rwxrwxrwx TEST_DIR/839/a
> +TEST_DIR/839/a
> +
> 

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Amir Goldstein]

On Wed, Apr 13, 2022 at 1:18 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Mon, Apr 11, 2022 at 03:54:42PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > fallocate changes file contents, so make sure that we drop privileges
> > and file capabilities after each fallocate operation.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/834.out |   33 +++++++++++++
> >  tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/835.out |   33 +++++++++++++
> >  tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/836.out |   33 +++++++++++++
> >  tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/837.out |   33 +++++++++++++
> >  tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/838.out |   33 +++++++++++++
> >  tests/generic/839     |   77 ++++++++++++++++++++++++++++++
> >  tests/generic/839.out |   13 +++++
> >  12 files changed, 890 insertions(+)
> >  create mode 100755 tests/generic/834
> >  create mode 100644 tests/generic/834.out
> >  create mode 100755 tests/generic/835
> >  create mode 100644 tests/generic/835.out
> >  create mode 100755 tests/generic/836
> >  create mode 100644 tests/generic/836.out
> >  create mode 100755 tests/generic/837
> >  create mode 100644 tests/generic/837.out
> >  create mode 100755 tests/generic/838
> >  create mode 100644 tests/generic/838.out
> >  create mode 100755 tests/generic/839
> >  create mode 100755 tests/generic/839.out
> >
> >
> > diff --git a/tests/generic/834 b/tests/generic/834
> > new file mode 100755
> > index 00000000..9302137b
> > --- /dev/null
> > +++ b/tests/generic/834
> > @@ -0,0 +1,127 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 834
> > +#
> > +# Functional test for dropping suid and sgid bits as part of a fallocate.
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto clone quick
> > +
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +     cd /
> > +     rm -r -f $tmp.* $junk_dir
> > +}
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/reflink
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs xfs btrfs ext4
>
> So we have more cases will break downstream XFS testing :)

Funny you should mention that.
I was going to propose an RFC for something like:

_fixed_by_kernel_commit fbe7e5200365 "xfs: fallocate() should call
file_modified()"

The first thing that could be done with this standard annotation is print a
hint on failure, like LTP does:

HINT: You _MAY_ be missing kernel fixes:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbe7e5200365

The second thing to be done is that downstream testers could use a script
to auto-generate an expunge list for their test kernel, if they don't care about
testing known issues, only regressions.

I hope that with the new maintainship you will also take the opportunity
to make fstests more friendly to downstream kernel testers.

> All cases looks good, but according to the custom, all generic cases use
> "_supported_fs generic", if you have 1+ specified filesystems, maybe
> "tests/shared/*" is better?
>

I think we should stay away from tests/shared for as much as possible and
use it only for very specific fs behaviors.

What in the behavior of fallocate() and setgid makes it so special that it needs
to be restricted to "xfs btrfs ext4" and not treated as a bug for other fs?
I suspect that it might be difficult or impossible to change that behavior in
network filesystems?

When facing a similar dilemma in the past we ended up with a whitelist
_fstyp_has_non_default_seek_data_hole(), but not sure we need to resort to that.

Thanks,
Amir.

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Zorro Lang]

On Wed, Apr 13, 2022 at 10:58:41AM +0300, Amir Goldstein wrote:
> On Wed, Apr 13, 2022 at 1:18 AM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Mon, Apr 11, 2022 at 03:54:42PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > fallocate changes file contents, so make sure that we drop privileges
> > > and file capabilities after each fallocate operation.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/834.out |   33 +++++++++++++
> > >  tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/835.out |   33 +++++++++++++
> > >  tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/836.out |   33 +++++++++++++
> > >  tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/837.out |   33 +++++++++++++
> > >  tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/838.out |   33 +++++++++++++
> > >  tests/generic/839     |   77 ++++++++++++++++++++++++++++++
> > >  tests/generic/839.out |   13 +++++
> > >  12 files changed, 890 insertions(+)
> > >  create mode 100755 tests/generic/834
> > >  create mode 100644 tests/generic/834.out
> > >  create mode 100755 tests/generic/835
> > >  create mode 100644 tests/generic/835.out
> > >  create mode 100755 tests/generic/836
> > >  create mode 100644 tests/generic/836.out
> > >  create mode 100755 tests/generic/837
> > >  create mode 100644 tests/generic/837.out
> > >  create mode 100755 tests/generic/838
> > >  create mode 100644 tests/generic/838.out
> > >  create mode 100755 tests/generic/839
> > >  create mode 100755 tests/generic/839.out
> > >
> > >
> > > diff --git a/tests/generic/834 b/tests/generic/834
> > > new file mode 100755
> > > index 00000000..9302137b
> > > --- /dev/null
> > > +++ b/tests/generic/834
> > > @@ -0,0 +1,127 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 834
> > > +#
> > > +# Functional test for dropping suid and sgid bits as part of a fallocate.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto clone quick
> > > +
> > > +# Override the default cleanup function.
> > > +_cleanup()
> > > +{
> > > +     cd /
> > > +     rm -r -f $tmp.* $junk_dir
> > > +}
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +. ./common/reflink
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs xfs btrfs ext4
> >
> > So we have more cases will break downstream XFS testing :)
> 
> Funny you should mention that.
> I was going to propose an RFC for something like:
> 
> _fixed_by_kernel_commit fbe7e5200365 "xfs: fallocate() should call
> file_modified()"
> 
> The first thing that could be done with this standard annotation is print a
> hint on failure, like LTP does:
> 
> HINT: You _MAY_ be missing kernel fixes:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbe7e5200365

I think it's not difficult to implement this behavior in xfstests. Generally if
a case covers a known bug, we record the patch commit in case description.

As my habit, if a test case fails, I'd like to read the case source code
directly, to get more details about the failure, and check if there's a known
issue(commit id) covered by that. If there is, check if the kernel I'm testing
contains this commit.

From my experience, if a case fails as it's expect, that's easy to find out,
if the comments is good. Print a hint will help, but won't help much I think,
due to the hint is just a guess, we still need to read source code or do more
testing to make sure that, when we hit a failure first time. But most of time
we always hit unexpected failures, that takes longer time to check.

> 
> The second thing to be done is that downstream testers could use a script
> to auto-generate an expunge list for their test kernel, if they don't care about
> testing known issues, only regressions.

In my testing on RHEL (downstream), I record and update known issues, include known
failures and panic/hang issues (need to skip) for each RHEL release. Before running
xfstests, I try to get a skip list for a specified RHEL/kernel version. Then compare
with its known failures after testing done, to decide if a failure is known/unknown.
Also I created version tags for my redhat internal xfstests repo, for some downstream
of downstream kernel testing (likes Z-stream testing) can use fixed xfstests version.

Some known issue format I record as below[1], a bash script will help to parse it and
compare with testing results. It's only for our internal use, due to I think it's too
crude to be shared :-P

[1]
$ cat known_results/$distro/xfs/145.json 
[
    {
        "DESCRIPTION": "bz19483*** XFS: Assertion failed: dqp->q_res_bcount >= be64_to_cpu(dqp->q_core.d_bcount)",
        "FS": ["xfs"],
        "DMESG": "Assertion failed: dqp->q_res_bcount >= be64_to_cpu\\(dqp->q_core.d_bcount\\)",
        "FIXED": true
    }
]
$ cat known_results/$distro/generic/417.json 
[
    {
        "DESCRIPTION": "bz16255*** (<1%): XFS corruption attribute entry #0 in attr block 0, inode 674 is INCOMPLETE",
        "FS": ["xfs"],
        "ARCH": ["ppc64le"],
        "OUTBAD": "_check_xfs_filesystem.*inconsistent",
        "FULL": "attribute entry.*in attr block.*, inode.*is INCOMPLETE"
    }
]

> 
> I hope that with the new maintainship you will also take the opportunity
> to make fstests more friendly to downstream kernel testers.
> 
> > All cases looks good, but according to the custom, all generic cases use
> > "_supported_fs generic", if you have 1+ specified filesystems, maybe
> > "tests/shared/*" is better?
> >
> 
> I think we should stay away from tests/shared for as much as possible and
> use it only for very specific fs behaviors.

I prefer generic testing too :)

> 
> What in the behavior of fallocate() and setgid makes it so special that it needs
> to be restricted to "xfs btrfs ext4" and not treated as a bug for other fs?
> I suspect that it might be difficult or impossible to change that behavior in
> network filesystems?

I'm not sure what other filesystems think about this behavior. If this's a standard
or most common behavior, I hope it can be a generic test (then let other fs maintainers
worry about their new testing failure:-P). Likes generic/673 was written for XFS,
then btrfs found failure, then btrfs said XFS should follow VFS as btrfs does :)

> 
> When facing a similar dilemma in the past we ended up with a whitelist
> _fstyp_has_non_default_seek_data_hole(), but not sure we need to resort to that.
> 
> Thanks,
> Amir.
> 

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Darrick J. Wong]

On Wed, Apr 13, 2022 at 11:44:01PM +0800, Zorro Lang wrote:
> On Wed, Apr 13, 2022 at 10:58:41AM +0300, Amir Goldstein wrote:
> > On Wed, Apr 13, 2022 at 1:18 AM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Mon, Apr 11, 2022 at 03:54:42PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > >
> > > > fallocate changes file contents, so make sure that we drop privileges
> > > > and file capabilities after each fallocate operation.
> > > >
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/generic/834.out |   33 +++++++++++++
> > > >  tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/generic/835.out |   33 +++++++++++++
> > > >  tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/generic/836.out |   33 +++++++++++++
> > > >  tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/generic/837.out |   33 +++++++++++++
> > > >  tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  tests/generic/838.out |   33 +++++++++++++
> > > >  tests/generic/839     |   77 ++++++++++++++++++++++++++++++
> > > >  tests/generic/839.out |   13 +++++
> > > >  12 files changed, 890 insertions(+)
> > > >  create mode 100755 tests/generic/834
> > > >  create mode 100644 tests/generic/834.out
> > > >  create mode 100755 tests/generic/835
> > > >  create mode 100644 tests/generic/835.out
> > > >  create mode 100755 tests/generic/836
> > > >  create mode 100644 tests/generic/836.out
> > > >  create mode 100755 tests/generic/837
> > > >  create mode 100644 tests/generic/837.out
> > > >  create mode 100755 tests/generic/838
> > > >  create mode 100644 tests/generic/838.out
> > > >  create mode 100755 tests/generic/839
> > > >  create mode 100755 tests/generic/839.out
> > > >
> > > >
> > > > diff --git a/tests/generic/834 b/tests/generic/834
> > > > new file mode 100755
> > > > index 00000000..9302137b
> > > > --- /dev/null
> > > > +++ b/tests/generic/834
> > > > @@ -0,0 +1,127 @@
> > > > +#! /bin/bash
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > > +#
> > > > +# FS QA Test No. 834
> > > > +#
> > > > +# Functional test for dropping suid and sgid bits as part of a fallocate.
> > > > +#
> > > > +. ./common/preamble
> > > > +_begin_fstest auto clone quick
> > > > +
> > > > +# Override the default cleanup function.
> > > > +_cleanup()
> > > > +{
> > > > +     cd /
> > > > +     rm -r -f $tmp.* $junk_dir
> > > > +}
> > > > +
> > > > +# Import common functions.
> > > > +. ./common/filter
> > > > +. ./common/reflink
> > > > +
> > > > +# real QA test starts here
> > > > +
> > > > +# Modify as appropriate.
> > > > +_supported_fs xfs btrfs ext4
> > >
> > > So we have more cases will break downstream XFS testing :)
> > 
> > Funny you should mention that.
> > I was going to propose an RFC for something like:
> > 
> > _fixed_by_kernel_commit fbe7e5200365 "xfs: fallocate() should call
> > file_modified()"
> > 
> > The first thing that could be done with this standard annotation is print a
> > hint on failure, like LTP does:
> > 
> > HINT: You _MAY_ be missing kernel fixes:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbe7e5200365
> 
> I think it's not difficult to implement this behavior in xfstests. Generally if
> a case covers a known bug, we record the patch commit in case description.

It's not hard, but it's a treewide change to identify all the fstests
that are regression fixes (or at least mention a commit hash) and well
beyond the scope of adding tests for a new fallocate security behavior.

In fact, it's an *entirely new project*.  One that I don't have time to
take on myself as a condition for getting *this* patch merged.

> As my habit, if a test case fails, I'd like to read the case source code
> directly, to get more details about the failure, and check if there's a known
> issue(commit id) covered by that. If there is, check if the kernel I'm testing
> contains this commit.
> 
> From my experience, if a case fails as it's expect, that's easy to find out,
> if the comments is good. Print a hint will help, but won't help much I think,
> due to the hint is just a guess, we still need to read source code or do more
> testing to make sure that, when we hit a failure first time. But most of time
> we always hit unexpected failures, that takes longer time to check.
> 
> > 
> > The second thing to be done is that downstream testers could use a script
> > to auto-generate an expunge list for their test kernel, if they don't care about
> > testing known issues, only regressions.
> 
> In my testing on RHEL (downstream), I record and update known issues, include known
> failures and panic/hang issues (need to skip) for each RHEL release. Before running
> xfstests, I try to get a skip list for a specified RHEL/kernel version. Then compare
> with its known failures after testing done, to decide if a failure is known/unknown.
> Also I created version tags for my redhat internal xfstests repo, for some downstream
> of downstream kernel testing (likes Z-stream testing) can use fixed xfstests version.
> 
> Some known issue format I record as below[1], a bash script will help to parse it and
> compare with testing results. It's only for our internal use, due to I think it's too
> crude to be shared :-P
> 
> [1]
> $ cat known_results/$distro/xfs/145.json 
> [
>     {
>         "DESCRIPTION": "bz19483*** XFS: Assertion failed: dqp->q_res_bcount >= be64_to_cpu(dqp->q_core.d_bcount)",
>         "FS": ["xfs"],
>         "DMESG": "Assertion failed: dqp->q_res_bcount >= be64_to_cpu\\(dqp->q_core.d_bcount\\)",
>         "FIXED": true
>     }
> ]
> $ cat known_results/$distro/generic/417.json 
> [
>     {
>         "DESCRIPTION": "bz16255*** (<1%): XFS corruption attribute entry #0 in attr block 0, inode 674 is INCOMPLETE",
>         "FS": ["xfs"],
>         "ARCH": ["ppc64le"],
>         "OUTBAD": "_check_xfs_filesystem.*inconsistent",
>         "FULL": "attribute entry.*in attr block.*, inode.*is INCOMPLETE"
>     }
> ]
> 
> > 
> > I hope that with the new maintainship you will also take the opportunity
> > to make fstests more friendly to downstream kernel testers.
> > 
> > > All cases looks good, but according to the custom, all generic cases use
> > > "_supported_fs generic", if you have 1+ specified filesystems, maybe
> > > "tests/shared/*" is better?
> > >
> > 
> > I think we should stay away from tests/shared for as much as possible and
> > use it only for very specific fs behaviors.
> 
> I prefer generic testing too :)
> 
> > 
> > What in the behavior of fallocate() and setgid makes it so special that it needs
> > to be restricted to "xfs btrfs ext4" and not treated as a bug for other fs?
> > I suspect that it might be difficult or impossible to change that behavior in
> > network filesystems?
> 
> I'm not sure what other filesystems think about this behavior. If this's a standard
> or most common behavior, I hope it can be a generic test (then let other fs maintainers
> worry about their new testing failure:-P). Likes generic/673 was written for XFS,
> then btrfs found failure, then btrfs said XFS should follow VFS as btrfs does :)

It will *become* a new behavior, but I haven't spread it to any other
filesystems other than the three listed above.  Overlayfs, for example,
doesn't clear set.id bits or drop file capabilities, nor do things like
f2fs and fat.  I'll get to them eventually, but I think I'll have an
easier time persuading the other maintainers of this new behavior if I
can tell them "Here is a change, and this is an existing fstest that
checks the behavior for correctness."

--D

> 
> > 
> > When facing a similar dilemma in the past we ended up with a whitelist
> > _fstyp_has_non_default_seek_data_hole(), but not sure we need to resort to that.
> > 
> > Thanks,
> > Amir.
> > 
> 

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Zorro Lang]

On Thu, Apr 14, 2022 at 08:50:07AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 13, 2022 at 11:44:01PM +0800, Zorro Lang wrote:
> > On Wed, Apr 13, 2022 at 10:58:41AM +0300, Amir Goldstein wrote:
> > > On Wed, Apr 13, 2022 at 1:18 AM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Mon, Apr 11, 2022 at 03:54:42PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > >
> > > > > fallocate changes file contents, so make sure that we drop privileges
> > > > > and file capabilities after each fallocate operation.
> > > > >
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > >  tests/generic/834     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/834.out |   33 +++++++++++++
> > > > >  tests/generic/835     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/835.out |   33 +++++++++++++
> > > > >  tests/generic/836     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/836.out |   33 +++++++++++++
> > > > >  tests/generic/837     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/837.out |   33 +++++++++++++
> > > > >  tests/generic/838     |  127 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  tests/generic/838.out |   33 +++++++++++++
> > > > >  tests/generic/839     |   77 ++++++++++++++++++++++++++++++
> > > > >  tests/generic/839.out |   13 +++++
> > > > >  12 files changed, 890 insertions(+)
> > > > >  create mode 100755 tests/generic/834
> > > > >  create mode 100644 tests/generic/834.out
> > > > >  create mode 100755 tests/generic/835
> > > > >  create mode 100644 tests/generic/835.out
> > > > >  create mode 100755 tests/generic/836
> > > > >  create mode 100644 tests/generic/836.out
> > > > >  create mode 100755 tests/generic/837
> > > > >  create mode 100644 tests/generic/837.out
> > > > >  create mode 100755 tests/generic/838
> > > > >  create mode 100644 tests/generic/838.out
> > > > >  create mode 100755 tests/generic/839
> > > > >  create mode 100755 tests/generic/839.out
> > > > >
> > > > >
> > > > > diff --git a/tests/generic/834 b/tests/generic/834
> > > > > new file mode 100755
> > > > > index 00000000..9302137b
> > > > > --- /dev/null
> > > > > +++ b/tests/generic/834
> > > > > @@ -0,0 +1,127 @@
> > > > > +#! /bin/bash
> > > > > +# SPDX-License-Identifier: GPL-2.0
> > > > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > > > +#
> > > > > +# FS QA Test No. 834
> > > > > +#
> > > > > +# Functional test for dropping suid and sgid bits as part of a fallocate.
> > > > > +#
> > > > > +. ./common/preamble
> > > > > +_begin_fstest auto clone quick
> > > > > +
> > > > > +# Override the default cleanup function.
> > > > > +_cleanup()
> > > > > +{
> > > > > +     cd /
> > > > > +     rm -r -f $tmp.* $junk_dir
> > > > > +}
> > > > > +
> > > > > +# Import common functions.
> > > > > +. ./common/filter
> > > > > +. ./common/reflink
> > > > > +
> > > > > +# real QA test starts here
> > > > > +
> > > > > +# Modify as appropriate.
> > > > > +_supported_fs xfs btrfs ext4
> > > >
> > > > So we have more cases will break downstream XFS testing :)
> > > 
> > > Funny you should mention that.
> > > I was going to propose an RFC for something like:
> > > 
> > > _fixed_by_kernel_commit fbe7e5200365 "xfs: fallocate() should call
> > > file_modified()"
> > > 
> > > The first thing that could be done with this standard annotation is print a
> > > hint on failure, like LTP does:
> > > 
> > > HINT: You _MAY_ be missing kernel fixes:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbe7e5200365
> > 
> > I think it's not difficult to implement this behavior in xfstests. Generally if
> > a case covers a known bug, we record the patch commit in case description.
> 
> It's not hard, but it's a treewide change to identify all the fstests
> that are regression fixes (or at least mention a commit hash) and well
> beyond the scope of adding tests for a new fallocate security behavior.
> 
> In fact, it's an *entirely new project*.  One that I don't have time to
> take on myself as a condition for getting *this* patch merged.

Hi Darrick, that's another story, you don't need to worry about that in this case :)
I'd like to ack this patch, but hope to move it from generic/ to shared/ . Maybe
Eryu can help to move it, or I can do that after I get the push permission.

The reason why I intend moving it to shared is:
Although we are trying to get rid of tests/shared/, but the tests/shared/ still help to
remind us what cases are still not real generic cases. We'll try to help all shared
cases to be generic. When the time is ready, I'd like to move this case to generic/
and change _supported_fs from "xfs btrfs ext4" to "generic".

Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks,
Zorro

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Amir Goldstein]

> Hi Darrick, that's another story, you don't need to worry about that in this case :)
> I'd like to ack this patch, but hope to move it from generic/ to shared/ . Maybe
> Eryu can help to move it, or I can do that after I get the push permission.
>
> The reason why I intend moving it to shared is:
> Although we are trying to get rid of tests/shared/, but the tests/shared/ still help to
> remind us what cases are still not real generic cases. We'll try to help all shared
> cases to be generic. When the time is ready, I'd like to move this case to generic/
> and change _supported_fs from "xfs btrfs ext4" to "generic".
>

Sorry, but I have to object to this move.
I do not think that is what tests/shared should be used for.

My preferences are:
1. _suppoted_fs generic && _require_xfs_io_command "finsert"
2. _suppoted_fs generic
3. _supported_fs xfs btrfs ext4 (without moving to tests/shared)

Thanks,
Amir.

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Zorro Lang]

On Fri, Apr 15, 2022 at 04:42:33PM +0300, Amir Goldstein wrote:
> > Hi Darrick, that's another story, you don't need to worry about that in this case :)
> > I'd like to ack this patch, but hope to move it from generic/ to shared/ . Maybe
> > Eryu can help to move it, or I can do that after I get the push permission.
> >
> > The reason why I intend moving it to shared is:
> > Although we are trying to get rid of tests/shared/, but the tests/shared/ still help to
> > remind us what cases are still not real generic cases. We'll try to help all shared
> > cases to be generic. When the time is ready, I'd like to move this case to generic/
> > and change _supported_fs from "xfs btrfs ext4" to "generic".
> >
> 
> Sorry, but I have to object to this move.
> I do not think that is what tests/shared should be used for.
> 
> My preferences are:
> 1. _suppoted_fs generic && _require_xfs_io_command "finsert"

There is:
"verb=finsert
 _require_xfs_io_command $verb"

This patch has not only one case, different cases test different mode of fallocate,
and I think Darrick has given them different _require_xfs_io_command.

> 2. _suppoted_fs generic
> 3. _supported_fs xfs btrfs ext4 (without moving to tests/shared)

There's not any generic cases write like this, only shared cases like that. My personal
opinion is *(2)* or make it shared if it insists "_supported_fs xfs btrfs ext4" (then
will move it back to generic and "_suppoted_fs generic" when Darrick think it's time).

Thanks,
Zorro

> 
> Thanks,
> Amir.
> 

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Amir Goldstein]

On Sat, Apr 16, 2022 at 5:02 PM Zorro Lang <zlang@redhat.com> wrote:
>
> On Fri, Apr 15, 2022 at 04:42:33PM +0300, Amir Goldstein wrote:
> > > Hi Darrick, that's another story, you don't need to worry about that in this case :)
> > > I'd like to ack this patch, but hope to move it from generic/ to shared/ . Maybe
> > > Eryu can help to move it, or I can do that after I get the push permission.
> > >
> > > The reason why I intend moving it to shared is:
> > > Although we are trying to get rid of tests/shared/, but the tests/shared/ still help to
> > > remind us what cases are still not real generic cases. We'll try to help all shared
> > > cases to be generic. When the time is ready, I'd like to move this case to generic/
> > > and change _supported_fs from "xfs btrfs ext4" to "generic".
> > >
> >
> > Sorry, but I have to object to this move.
> > I do not think that is what tests/shared should be used for.
> >
> > My preferences are:
> > 1. _suppoted_fs generic && _require_xfs_io_command "finsert"
>
> There is:
> "verb=finsert
>  _require_xfs_io_command $verb"
>
> This patch has not only one case, different cases test different mode of fallocate,
> and I think Darrick has given them different _require_xfs_io_command.
>

I know. I meant that the tests for verbs finsert/fcollapse can definitely use
generic as there are very few fs that support those verbs and those fs
should be fixed, not hide the failure.

cifs maintainer btw is using whitelists of tests for CI, so new
failing tests  are
not likely to affect cifs testing.

> > 2. _suppoted_fs generic
> > 3. _supported_fs xfs btrfs ext4 (without moving to tests/shared)
>
> There's not any generic cases write like this, only shared cases like that. My personal
> opinion is *(2)* or make it shared if it insists "_supported_fs xfs btrfs ext4" (then
> will move it back to generic and "_suppoted_fs generic" when Darrick think it's time).
>

Let's wait to hear what Darrick has to say.
I just don't understand the incentive to hide test failures from buggy fs even
if this is a change of long time behavior.

BTW, here is untested draft of what I started working on:
https://github.com/amir73il/xfstests/commits/hints

With the new helpers, this test could also be classified as:

_suppoted_fs generic
_known_issue_on_fs ^xfs ^btrfs ^ext4

Meaning that the test runs on all fs, unless tester opts-out with
-x known_issues
and if test is run an fails an hint is printed:
"You _MAY_ be hit by a known issue on $FSTYP."

I'll probably be done with testing those patches and post them tomorrow.

Thanks,
Amir.

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Eryu Guan]

On Fri, Apr 15, 2022 at 04:42:33PM +0300, Amir Goldstein wrote:
> > Hi Darrick, that's another story, you don't need to worry about that in this case :)
> > I'd like to ack this patch, but hope to move it from generic/ to shared/ . Maybe
> > Eryu can help to move it, or I can do that after I get the push permission.
> >
> > The reason why I intend moving it to shared is:
> > Although we are trying to get rid of tests/shared/, but the tests/shared/ still help to
> > remind us what cases are still not real generic cases. We'll try to help all shared
> > cases to be generic. When the time is ready, I'd like to move this case to generic/
> > and change _supported_fs from "xfs btrfs ext4" to "generic".
> >
> 
> Sorry, but I have to object to this move.
> I do not think that is what tests/shared should be used for.

After reading all the discussions, I prefer option 2 here as well, it's
testing for a security bug, and all affected filesystems should be fixed,
and a new failure will remind people there's something to be fixed.

> 
> My preferences are:
> 1. _suppoted_fs generic && _require_xfs_io_command "finsert"

As btrfs doesn't support "finsert", so the falloc/fpunch tests won't run
on btrfs, and we miss test coverage there.

> 2. _suppoted_fs generic
> 3. _supported_fs xfs btrfs ext4 (without moving to tests/shared)

This is weired. And if we really want to restrict the new behavior
within xfs, btrfs and ext4 for now, then I can live with a whitelist
_require rule, and a good comment on it.

Thanks,
Eryu

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Darrick J. Wong]

On Sun, Apr 17, 2022 at 11:40:32PM +0800, Eryu Guan wrote:
> On Fri, Apr 15, 2022 at 04:42:33PM +0300, Amir Goldstein wrote:
> > > Hi Darrick, that's another story, you don't need to worry about that in this case :)
> > > I'd like to ack this patch, but hope to move it from generic/ to shared/ . Maybe
> > > Eryu can help to move it, or I can do that after I get the push permission.
> > >
> > > The reason why I intend moving it to shared is:
> > > Although we are trying to get rid of tests/shared/, but the tests/shared/ still help to
> > > remind us what cases are still not real generic cases. We'll try to help all shared
> > > cases to be generic. When the time is ready, I'd like to move this case to generic/
> > > and change _supported_fs from "xfs btrfs ext4" to "generic".
> > >
> > 
> > Sorry, but I have to object to this move.
> > I do not think that is what tests/shared should be used for.
> 
> After reading all the discussions, I prefer option 2 here as well, it's
> testing for a security bug, and all affected filesystems should be fixed,
> and a new failure will remind people there's something to be fixed.

Ok.  I'll put it back to _supported_fs generic and leave the tests in
tests/generic/.  Thank you for making a decision. :)

--D

> > 
> > My preferences are:
> > 1. _suppoted_fs generic && _require_xfs_io_command "finsert"
> 
> As btrfs doesn't support "finsert", so the falloc/fpunch tests won't run
> on btrfs, and we miss test coverage there.
> 
> > 2. _suppoted_fs generic
> > 3. _supported_fs xfs btrfs ext4 (without moving to tests/shared)
> 
> This is weired. And if we really want to restrict the new behavior
> within xfs, btrfs and ext4 for now, then I can live with a whitelist
> _require rule, and a good comment on it.
> 
> Thanks,
> Eryu

Re: [PATCH 2/4] generic: ensure we drop suid after fallocate

[Amir Goldstein]

> > > > > +# Modify as appropriate.
> > > > > +_supported_fs xfs btrfs ext4
> > > >
> > > > So we have more cases will break downstream XFS testing :)
> > >
> > > Funny you should mention that.
> > > I was going to propose an RFC for something like:
> > >
> > > _fixed_by_kernel_commit fbe7e5200365 "xfs: fallocate() should call
> > > file_modified()"
> > >
> > > The first thing that could be done with this standard annotation is print a
> > > hint on failure, like LTP does:
> > >
> > > HINT: You _MAY_ be missing kernel fixes:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fbe7e5200365
> >
> > I think it's not difficult to implement this behavior in xfstests. Generally if
> > a case covers a known bug, we record the patch commit in case description.
>
> It's not hard, but it's a treewide change to identify all the fstests
> that are regression fixes (or at least mention a commit hash) and well
> beyond the scope of adding tests for a new fallocate security behavior.
>
> In fact, it's an *entirely new project*.  One that I don't have time to
> take on myself as a condition for getting *this* patch merged.
>

To be clear, my comment had no intention to serve as a condition for
merging your patch and not for suggesting that you should do anything really.
My comment was that "I was going to propose an RFC" meaning that
I was going to send patches but didn't get to it yet.
It's not a treewide project either, it's a simple optional annotation per test,
as is the case with LTP's optional .tags array.

My plan was to start with annotating the overlay tests and the xfs tests
that I collected during my work on v5.10..v.5.17 xfs backports, as they say
The change starts with me ;)

[...]

> > >
> > > What in the behavior of fallocate() and setgid makes it so special that it needs
> > > to be restricted to "xfs btrfs ext4" and not treated as a bug for other fs?
> > > I suspect that it might be difficult or impossible to change that behavior in
> > > network filesystems?
> >
> > I'm not sure what other filesystems think about this behavior. If this's a standard
> > or most common behavior, I hope it can be a generic test (then let other fs maintainers
> > worry about their new testing failure:-P). Likes generic/673 was written for XFS,
> > then btrfs found failure, then btrfs said XFS should follow VFS as btrfs does :)
>
> It will *become* a new behavior, but I haven't spread it to any other
> filesystems other than the three listed above.  Overlayfs, for example,
> doesn't clear set.id bits or drop file capabilities, nor do things like
> f2fs and fat.  I'll get to them eventually, but I think I'll have an
> easier time persuading the other maintainers of this new behavior if I
> can tell them "Here is a change, and this is an existing fstest that
> checks the behavior for correctness."
>

TBH, I am a bit surprised that you sign yourself up to fixing all those
other filesystems. It sounds like you got plenty on your plate already.
My intention is not to talk you out of doing community work, but to ask
what is the best way for developers that find a bug which they do NOT
intend to fix, to annotate the test for that bug.

I ran into that dilema with overlay/061 which tests for some non-standard
behavior regarding mmap that has been there since day 1 and is sufficiently
hard to fix. I ended up leaving this test out of "auto" group and in
"posix" group.
I might have left it in "known" or "broken" group just the same.

Regarding *this* patch, why not leave it _supported_fs generic?
What are the downsides?
Other fs would fail the test as they should.
It's not like that new behavior is debatable, it's a proper security issue.
And especially for fs that support FALLOC_FL_COLLAPSE_RANGE
and FALLOC_FL_INSERT_RANGE, so my other suggestion if you do
not wish to inflict this new failure on all other fs is to make the test require
finsert and document in a comment why that was done.

I know that the test won't run on btrfs with this requirement, but that is not
your problem to solve and also I am quite surprised that btrfs does not
support finsert/fcollapse, so maybe that support will be added some day
and then the security thread that comes with it could be avoided.

Thanks,
Amir.

[PATCH 3/4] generic: test that linking into a directory fails with EDQUOT

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Add a regression test to make sure that unprivileged userspace linking
into a directory fails with EDQUOT when the directory quota limits have
been exceeded.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/832     |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/832.out |    3 ++
 2 files changed, 70 insertions(+)
 create mode 100755 tests/generic/832
 create mode 100644 tests/generic/832.out


diff --git a/tests/generic/832 b/tests/generic/832
new file mode 100755
index 00000000..1190b795
--- /dev/null
+++ b/tests/generic/832
@@ -0,0 +1,67 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 832
+#
+# Ensure that unprivileged userspace hits EDQUOT while linking files into a
+# directory when the directory's quota limits have been exceeded.
+#
+# Regression test for commit:
+#
+# 871b9316e7a7 ("xfs: reserve quota for dir expansion when linking/unlinking files")
+#
+. ./common/preamble
+_begin_fstest auto quick quota
+
+# Import common functions.
+. ./common/filter
+. ./common/quota
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_quota
+_require_user
+_require_scratch
+
+_scratch_mkfs > "$seqres.full" 2>&1
+_qmount_option usrquota
+_qmount
+
+blocksize=$(_get_block_size $SCRATCH_MNT)
+scratchdir=$SCRATCH_MNT/dir
+scratchfile=$SCRATCH_MNT/file
+mkdir $scratchdir
+touch $scratchfile
+
+# Create a 2-block directory for our 1-block quota limit
+total_size=$((blocksize * 2))
+dirents=$((total_size / 255))
+
+for ((i = 0; i < dirents; i++)); do
+	name=$(printf "x%0254d" $i)
+	ln $scratchfile $scratchdir/$name
+done
+
+# Set a low quota hardlimit for an unprivileged uid and chown the files to it
+echo "set up quota" >> $seqres.full
+setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
+chown $qa_user $scratchdir $scratchfile
+repquota -upn $SCRATCH_MNT >> $seqres.full
+
+# Fail at appending the directory as qa_user to ensure quota enforcement works
+echo "fail quota" >> $seqres.full
+for ((i = 0; i < dirents; i++)); do
+	name=$(printf "y%0254d" $i)
+	su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
+		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
+	test "${PIPESTATUS[0]}" -ne 0 && break
+done
+repquota -upn $SCRATCH_MNT >> $seqres.full
+
+# success, all done
+echo Silence is golden
+status=0
+exit
diff --git a/tests/generic/832.out b/tests/generic/832.out
new file mode 100644
index 00000000..593afe8b
--- /dev/null
+++ b/tests/generic/832.out
@@ -0,0 +1,3 @@
+QA output created by 832
+ln: failed to create hard link 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
+Silence is golden

Re: [PATCH 3/4] generic: test that linking into a directory fails with EDQUOT

[Zorro Lang]

On Mon, Apr 11, 2022 at 03:54:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add a regression test to make sure that unprivileged userspace linking
> into a directory fails with EDQUOT when the directory quota limits have
> been exceeded.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/generic/832     |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/832.out |    3 ++
>  2 files changed, 70 insertions(+)
>  create mode 100755 tests/generic/832
>  create mode 100644 tests/generic/832.out
> 
> 
> diff --git a/tests/generic/832 b/tests/generic/832
> new file mode 100755
> index 00000000..1190b795
> --- /dev/null
> +++ b/tests/generic/832
> @@ -0,0 +1,67 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 832
> +#
> +# Ensure that unprivileged userspace hits EDQUOT while linking files into a
> +# directory when the directory's quota limits have been exceeded.
> +#
> +# Regression test for commit:
> +#
> +# 871b9316e7a7 ("xfs: reserve quota for dir expansion when linking/unlinking files")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick quota
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/quota
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_quota
> +_require_user
> +_require_scratch
> +
> +_scratch_mkfs > "$seqres.full" 2>&1
> +_qmount_option usrquota
> +_qmount
> +
> +blocksize=$(_get_block_size $SCRATCH_MNT)
> +scratchdir=$SCRATCH_MNT/dir
> +scratchfile=$SCRATCH_MNT/file
> +mkdir $scratchdir
> +touch $scratchfile
> +
> +# Create a 2-block directory for our 1-block quota limit
> +total_size=$((blocksize * 2))
> +dirents=$((total_size / 255))
> +
> +for ((i = 0; i < dirents; i++)); do
> +	name=$(printf "x%0254d" $i)
> +	ln $scratchfile $scratchdir/$name
> +done
> +
> +# Set a low quota hardlimit for an unprivileged uid and chown the files to it
> +echo "set up quota" >> $seqres.full
> +setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
> +chown $qa_user $scratchdir $scratchfile
> +repquota -upn $SCRATCH_MNT >> $seqres.full
> +
> +# Fail at appending the directory as qa_user to ensure quota enforcement works
> +echo "fail quota" >> $seqres.full
> +for ((i = 0; i < dirents; i++)); do
> +	name=$(printf "y%0254d" $i)
> +	su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \

All looks good to me. Only one question about this "su -". Is the "-" necessary?
I checked all cases in fstests, no one use "--login" when try to su to $qa_user.
I'm not sure if "login $qa_user" will affect the testing, I just know it affect
environment variables.

Thanks,
Zorro

> +		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
> +	test "${PIPESTATUS[0]}" -ne 0 && break
> +done
> +repquota -upn $SCRATCH_MNT >> $seqres.full
> +
> +# success, all done
> +echo Silence is golden
> +status=0
> +exit
> diff --git a/tests/generic/832.out b/tests/generic/832.out
> new file mode 100644
> index 00000000..593afe8b
> --- /dev/null
> +++ b/tests/generic/832.out
> @@ -0,0 +1,3 @@
> +QA output created by 832
> +ln: failed to create hard link 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
> +Silence is golden
> 

Re: [PATCH 3/4] generic: test that linking into a directory fails with EDQUOT

[Darrick J. Wong]

On Wed, Apr 13, 2022 at 01:17:23AM +0800, Zorro Lang wrote:
> On Mon, Apr 11, 2022 at 03:54:48PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Add a regression test to make sure that unprivileged userspace linking
> > into a directory fails with EDQUOT when the directory quota limits have
> > been exceeded.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/generic/832     |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/832.out |    3 ++
> >  2 files changed, 70 insertions(+)
> >  create mode 100755 tests/generic/832
> >  create mode 100644 tests/generic/832.out
> > 
> > 
> > diff --git a/tests/generic/832 b/tests/generic/832
> > new file mode 100755
> > index 00000000..1190b795
> > --- /dev/null
> > +++ b/tests/generic/832
> > @@ -0,0 +1,67 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 832
> > +#
> > +# Ensure that unprivileged userspace hits EDQUOT while linking files into a
> > +# directory when the directory's quota limits have been exceeded.
> > +#
> > +# Regression test for commit:
> > +#
> > +# 871b9316e7a7 ("xfs: reserve quota for dir expansion when linking/unlinking files")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick quota
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/quota
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_require_quota
> > +_require_user
> > +_require_scratch
> > +
> > +_scratch_mkfs > "$seqres.full" 2>&1
> > +_qmount_option usrquota
> > +_qmount
> > +
> > +blocksize=$(_get_block_size $SCRATCH_MNT)
> > +scratchdir=$SCRATCH_MNT/dir
> > +scratchfile=$SCRATCH_MNT/file
> > +mkdir $scratchdir
> > +touch $scratchfile
> > +
> > +# Create a 2-block directory for our 1-block quota limit
> > +total_size=$((blocksize * 2))
> > +dirents=$((total_size / 255))
> > +
> > +for ((i = 0; i < dirents; i++)); do
> > +	name=$(printf "x%0254d" $i)
> > +	ln $scratchfile $scratchdir/$name
> > +done
> > +
> > +# Set a low quota hardlimit for an unprivileged uid and chown the files to it
> > +echo "set up quota" >> $seqres.full
> > +setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
> > +chown $qa_user $scratchdir $scratchfile
> > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > +
> > +# Fail at appending the directory as qa_user to ensure quota enforcement works
> > +echo "fail quota" >> $seqres.full
> > +for ((i = 0; i < dirents; i++)); do
> > +	name=$(printf "y%0254d" $i)
> > +	su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
> 
> All looks good to me. Only one question about this "su -". Is the "-" necessary?
> I checked all cases in fstests, no one use "--login" when try to su to $qa_user.
> I'm not sure if "login $qa_user" will affect the testing, I just know it affect
> environment variables.

It's not strictly necessary since it's unlikely that qa_user="-luser",
but it seems like a Good Idea to prevent su cli option injection
attacks.

--D

> Thanks,
> Zorro
> 
> > +		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
> > +	test "${PIPESTATUS[0]}" -ne 0 && break
> > +done
> > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > +
> > +# success, all done
> > +echo Silence is golden
> > +status=0
> > +exit
> > diff --git a/tests/generic/832.out b/tests/generic/832.out
> > new file mode 100644
> > index 00000000..593afe8b
> > --- /dev/null
> > +++ b/tests/generic/832.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 832
> > +ln: failed to create hard link 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
> > +Silence is golden
> > 
> 

Re: [PATCH 3/4] generic: test that linking into a directory fails with EDQUOT

[Zorro Lang]

On Tue, Apr 12, 2022 at 10:52:56AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 13, 2022 at 01:17:23AM +0800, Zorro Lang wrote:
> > On Mon, Apr 11, 2022 at 03:54:48PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Add a regression test to make sure that unprivileged userspace linking
> > > into a directory fails with EDQUOT when the directory quota limits have
> > > been exceeded.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/generic/832     |   67 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/832.out |    3 ++
> > >  2 files changed, 70 insertions(+)
> > >  create mode 100755 tests/generic/832
> > >  create mode 100644 tests/generic/832.out
> > > 
> > > 
> > > diff --git a/tests/generic/832 b/tests/generic/832
> > > new file mode 100755
> > > index 00000000..1190b795
> > > --- /dev/null
> > > +++ b/tests/generic/832
> > > @@ -0,0 +1,67 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 832
> > > +#
> > > +# Ensure that unprivileged userspace hits EDQUOT while linking files into a
> > > +# directory when the directory's quota limits have been exceeded.
> > > +#
> > > +# Regression test for commit:
> > > +#
> > > +# 871b9316e7a7 ("xfs: reserve quota for dir expansion when linking/unlinking files")
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick quota
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +. ./common/quota
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs generic
> > > +_require_quota
> > > +_require_user
> > > +_require_scratch
> > > +
> > > +_scratch_mkfs > "$seqres.full" 2>&1
> > > +_qmount_option usrquota
> > > +_qmount
> > > +
> > > +blocksize=$(_get_block_size $SCRATCH_MNT)
> > > +scratchdir=$SCRATCH_MNT/dir
> > > +scratchfile=$SCRATCH_MNT/file
> > > +mkdir $scratchdir
> > > +touch $scratchfile
> > > +
> > > +# Create a 2-block directory for our 1-block quota limit
> > > +total_size=$((blocksize * 2))
> > > +dirents=$((total_size / 255))
> > > +
> > > +for ((i = 0; i < dirents; i++)); do
> > > +	name=$(printf "x%0254d" $i)
> > > +	ln $scratchfile $scratchdir/$name
> > > +done
> > > +
> > > +# Set a low quota hardlimit for an unprivileged uid and chown the files to it
> > > +echo "set up quota" >> $seqres.full
> > > +setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
> > > +chown $qa_user $scratchdir $scratchfile
> > > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > > +
> > > +# Fail at appending the directory as qa_user to ensure quota enforcement works
> > > +echo "fail quota" >> $seqres.full
> > > +for ((i = 0; i < dirents; i++)); do
> > > +	name=$(printf "y%0254d" $i)
> > > +	su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
> > 
> > All looks good to me. Only one question about this "su -". Is the "-" necessary?
> > I checked all cases in fstests, no one use "--login" when try to su to $qa_user.
> > I'm not sure if "login $qa_user" will affect the testing, I just know it affect
> > environment variables.
> 
> It's not strictly necessary since it's unlikely that qa_user="-luser",
> but it seems like a Good Idea to prevent su cli option injection
> attacks.

Thanks for your understanding :) Eryu of me (after I get push permission) will help
to remove the little "-" when merge it.

Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > +		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
> > > +	test "${PIPESTATUS[0]}" -ne 0 && break
> > > +done
> > > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > > +
> > > +# success, all done
> > > +echo Silence is golden
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/832.out b/tests/generic/832.out
> > > new file mode 100644
> > > index 00000000..593afe8b
> > > --- /dev/null
> > > +++ b/tests/generic/832.out
> > > @@ -0,0 +1,3 @@
> > > +QA output created by 832
> > > +ln: failed to create hard link 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
> > > +Silence is golden
> > > 
> > 
> 

[PATCH 4/4] generic: test that renaming into a directory fails with EDQUOT

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Add a regression test to make sure that unprivileged userspace renaming
within a directory fails with EDQUOT when the directory quota limits have
been exceeded.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/833     |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/833.out |    3 ++
 2 files changed, 74 insertions(+)
 create mode 100755 tests/generic/833
 create mode 100644 tests/generic/833.out


diff --git a/tests/generic/833 b/tests/generic/833
new file mode 100755
index 00000000..a1b3cbc0
--- /dev/null
+++ b/tests/generic/833
@@ -0,0 +1,71 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test No. 833
+#
+# Ensure that unprivileged userspace hits EDQUOT while moving files into a
+# directory when the directory's quota limits have been exceeded.
+#
+# Regression test for commit:
+#
+# 41667260bc84 ("xfs: reserve quota for target dir expansion when renaming files")
+#
+. ./common/preamble
+_begin_fstest auto quick quota
+
+# Import common functions.
+. ./common/filter
+. ./common/quota
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_quota
+_require_user
+_require_scratch
+
+_scratch_mkfs > "$seqres.full" 2>&1
+_qmount_option usrquota
+_qmount
+
+blocksize=$(_get_block_size $SCRATCH_MNT)
+scratchdir=$SCRATCH_MNT/dir
+scratchfile=$SCRATCH_MNT/file
+stagedir=$SCRATCH_MNT/staging
+mkdir $scratchdir $stagedir
+touch $scratchfile
+
+# Create a 2-block directory for our 1-block quota limit
+total_size=$((blocksize * 2))
+dirents=$((total_size / 255))
+
+for ((i = 0; i < dirents; i++)); do
+	name=$(printf "x%0254d" $i)
+	ln $scratchfile $scratchdir/$name
+done
+
+# Set a low quota hardlimit for an unprivileged uid and chown the files to it
+echo "set up quota" >> $seqres.full
+setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
+chown $qa_user $scratchdir $scratchfile
+repquota -upn $SCRATCH_MNT >> $seqres.full
+
+# Fail at renaming into the directory as qa_user to ensure quota enforcement
+# works
+chmod a+rwx $stagedir
+echo "fail quota" >> $seqres.full
+for ((i = 0; i < dirents; i++)); do
+	name=$(printf "y%0254d" $i)
+	ln $scratchfile $stagedir/$name
+	su - "$qa_user" -c "mv $stagedir/$name $scratchdir/$name" 2>&1 | \
+		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
+	test "${PIPESTATUS[0]}" -ne 0 && break
+done
+repquota -upn $SCRATCH_MNT >> $seqres.full
+
+# success, all done
+echo Silence is golden
+status=0
+exit
diff --git a/tests/generic/833.out b/tests/generic/833.out
new file mode 100644
index 00000000..d100fa07
--- /dev/null
+++ b/tests/generic/833.out
@@ -0,0 +1,3 @@
+QA output created by 833
+mv: cannot move 'SCRATCH_MNT/staging/yXXX' to 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
+Silence is golden

Re: [PATCH 4/4] generic: test that renaming into a directory fails with EDQUOT

[Zorro Lang]

On Mon, Apr 11, 2022 at 03:54:54PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Add a regression test to make sure that unprivileged userspace renaming
> within a directory fails with EDQUOT when the directory quota limits have
> been exceeded.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/generic/833     |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/833.out |    3 ++
>  2 files changed, 74 insertions(+)
>  create mode 100755 tests/generic/833
>  create mode 100644 tests/generic/833.out
> 
> 
> diff --git a/tests/generic/833 b/tests/generic/833
> new file mode 100755
> index 00000000..a1b3cbc0
> --- /dev/null
> +++ b/tests/generic/833
> @@ -0,0 +1,71 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test No. 833
> +#
> +# Ensure that unprivileged userspace hits EDQUOT while moving files into a
> +# directory when the directory's quota limits have been exceeded.
> +#
> +# Regression test for commit:
> +#
> +# 41667260bc84 ("xfs: reserve quota for target dir expansion when renaming files")
> +#
> +. ./common/preamble
> +_begin_fstest auto quick quota
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/quota
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_quota
> +_require_user
> +_require_scratch
> +
> +_scratch_mkfs > "$seqres.full" 2>&1
> +_qmount_option usrquota
> +_qmount
> +
> +blocksize=$(_get_block_size $SCRATCH_MNT)
> +scratchdir=$SCRATCH_MNT/dir
> +scratchfile=$SCRATCH_MNT/file
> +stagedir=$SCRATCH_MNT/staging
> +mkdir $scratchdir $stagedir
> +touch $scratchfile
> +
> +# Create a 2-block directory for our 1-block quota limit
> +total_size=$((blocksize * 2))
> +dirents=$((total_size / 255))
> +
> +for ((i = 0; i < dirents; i++)); do
> +	name=$(printf "x%0254d" $i)
> +	ln $scratchfile $scratchdir/$name
> +done
> +
> +# Set a low quota hardlimit for an unprivileged uid and chown the files to it
> +echo "set up quota" >> $seqres.full
> +setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
> +chown $qa_user $scratchdir $scratchfile
> +repquota -upn $SCRATCH_MNT >> $seqres.full
> +
> +# Fail at renaming into the directory as qa_user to ensure quota enforcement
> +# works
> +chmod a+rwx $stagedir
> +echo "fail quota" >> $seqres.full
> +for ((i = 0; i < dirents; i++)); do
> +	name=$(printf "y%0254d" $i)
> +	ln $scratchfile $stagedir/$name
> +	su - "$qa_user" -c "mv $stagedir/$name $scratchdir/$name" 2>&1 | \

Same as [PATCH 3/4], do we need "--login"?
Oh, I just found there's only one case generic/128 use this option too. Anyway I
have no reason to object it, just speak out for review:)

Thanks,
Zorro

> +		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
> +	test "${PIPESTATUS[0]}" -ne 0 && break
> +done
> +repquota -upn $SCRATCH_MNT >> $seqres.full
> +
> +# success, all done
> +echo Silence is golden
> +status=0
> +exit
> diff --git a/tests/generic/833.out b/tests/generic/833.out
> new file mode 100644
> index 00000000..d100fa07
> --- /dev/null
> +++ b/tests/generic/833.out
> @@ -0,0 +1,3 @@
> +QA output created by 833
> +mv: cannot move 'SCRATCH_MNT/staging/yXXX' to 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
> +Silence is golden
> 

Re: [PATCH 4/4] generic: test that renaming into a directory fails with EDQUOT

[Darrick J. Wong]

On Wed, Apr 13, 2022 at 01:29:30AM +0800, Zorro Lang wrote:
> On Mon, Apr 11, 2022 at 03:54:54PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Add a regression test to make sure that unprivileged userspace renaming
> > within a directory fails with EDQUOT when the directory quota limits have
> > been exceeded.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/generic/833     |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/833.out |    3 ++
> >  2 files changed, 74 insertions(+)
> >  create mode 100755 tests/generic/833
> >  create mode 100644 tests/generic/833.out
> > 
> > 
> > diff --git a/tests/generic/833 b/tests/generic/833
> > new file mode 100755
> > index 00000000..a1b3cbc0
> > --- /dev/null
> > +++ b/tests/generic/833
> > @@ -0,0 +1,71 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > +#
> > +# FS QA Test No. 833
> > +#
> > +# Ensure that unprivileged userspace hits EDQUOT while moving files into a
> > +# directory when the directory's quota limits have been exceeded.
> > +#
> > +# Regression test for commit:
> > +#
> > +# 41667260bc84 ("xfs: reserve quota for target dir expansion when renaming files")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick quota
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +. ./common/quota
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_require_quota
> > +_require_user
> > +_require_scratch
> > +
> > +_scratch_mkfs > "$seqres.full" 2>&1
> > +_qmount_option usrquota
> > +_qmount
> > +
> > +blocksize=$(_get_block_size $SCRATCH_MNT)
> > +scratchdir=$SCRATCH_MNT/dir
> > +scratchfile=$SCRATCH_MNT/file
> > +stagedir=$SCRATCH_MNT/staging
> > +mkdir $scratchdir $stagedir
> > +touch $scratchfile
> > +
> > +# Create a 2-block directory for our 1-block quota limit
> > +total_size=$((blocksize * 2))
> > +dirents=$((total_size / 255))
> > +
> > +for ((i = 0; i < dirents; i++)); do
> > +	name=$(printf "x%0254d" $i)
> > +	ln $scratchfile $scratchdir/$name
> > +done
> > +
> > +# Set a low quota hardlimit for an unprivileged uid and chown the files to it
> > +echo "set up quota" >> $seqres.full
> > +setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
> > +chown $qa_user $scratchdir $scratchfile
> > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > +
> > +# Fail at renaming into the directory as qa_user to ensure quota enforcement
> > +# works
> > +chmod a+rwx $stagedir
> > +echo "fail quota" >> $seqres.full
> > +for ((i = 0; i < dirents; i++)); do
> > +	name=$(printf "y%0254d" $i)
> > +	ln $scratchfile $stagedir/$name
> > +	su - "$qa_user" -c "mv $stagedir/$name $scratchdir/$name" 2>&1 | \
> 
> Same as [PATCH 3/4], do we need "--login"?
> Oh, I just found there's only one case generic/128 use this option too. Anyway I
> have no reason to object it, just speak out for review:)

<nod> I have the same response as the previous patch. ;)

--D

> Thanks,
> Zorro
> 
> > +		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
> > +	test "${PIPESTATUS[0]}" -ne 0 && break
> > +done
> > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > +
> > +# success, all done
> > +echo Silence is golden
> > +status=0
> > +exit
> > diff --git a/tests/generic/833.out b/tests/generic/833.out
> > new file mode 100644
> > index 00000000..d100fa07
> > --- /dev/null
> > +++ b/tests/generic/833.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 833
> > +mv: cannot move 'SCRATCH_MNT/staging/yXXX' to 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
> > +Silence is golden
> > 
> 

Re: [PATCH 4/4] generic: test that renaming into a directory fails with EDQUOT

[Zorro Lang]

On Tue, Apr 12, 2022 at 10:58:27AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 13, 2022 at 01:29:30AM +0800, Zorro Lang wrote:
> > On Mon, Apr 11, 2022 at 03:54:54PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Add a regression test to make sure that unprivileged userspace renaming
> > > within a directory fails with EDQUOT when the directory quota limits have
> > > been exceeded.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  tests/generic/833     |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/833.out |    3 ++
> > >  2 files changed, 74 insertions(+)
> > >  create mode 100755 tests/generic/833
> > >  create mode 100644 tests/generic/833.out
> > > 
> > > 
> > > diff --git a/tests/generic/833 b/tests/generic/833
> > > new file mode 100755
> > > index 00000000..a1b3cbc0
> > > --- /dev/null
> > > +++ b/tests/generic/833
> > > @@ -0,0 +1,71 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 833
> > > +#
> > > +# Ensure that unprivileged userspace hits EDQUOT while moving files into a
> > > +# directory when the directory's quota limits have been exceeded.
> > > +#
> > > +# Regression test for commit:
> > > +#
> > > +# 41667260bc84 ("xfs: reserve quota for target dir expansion when renaming files")
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick quota
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +. ./common/quota
> > > +
> > > +# real QA test starts here
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs generic
> > > +_require_quota
> > > +_require_user
> > > +_require_scratch
> > > +
> > > +_scratch_mkfs > "$seqres.full" 2>&1
> > > +_qmount_option usrquota
> > > +_qmount
> > > +
> > > +blocksize=$(_get_block_size $SCRATCH_MNT)
> > > +scratchdir=$SCRATCH_MNT/dir
> > > +scratchfile=$SCRATCH_MNT/file
> > > +stagedir=$SCRATCH_MNT/staging
> > > +mkdir $scratchdir $stagedir
> > > +touch $scratchfile
> > > +
> > > +# Create a 2-block directory for our 1-block quota limit
> > > +total_size=$((blocksize * 2))
> > > +dirents=$((total_size / 255))
> > > +
> > > +for ((i = 0; i < dirents; i++)); do
> > > +	name=$(printf "x%0254d" $i)
> > > +	ln $scratchfile $scratchdir/$name
> > > +done
> > > +
> > > +# Set a low quota hardlimit for an unprivileged uid and chown the files to it
> > > +echo "set up quota" >> $seqres.full
> > > +setquota -u $qa_user 0 "$((blocksize / 1024))" 0 0 $SCRATCH_MNT
> > > +chown $qa_user $scratchdir $scratchfile
> > > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > > +
> > > +# Fail at renaming into the directory as qa_user to ensure quota enforcement
> > > +# works
> > > +chmod a+rwx $stagedir
> > > +echo "fail quota" >> $seqres.full
> > > +for ((i = 0; i < dirents; i++)); do
> > > +	name=$(printf "y%0254d" $i)
> > > +	ln $scratchfile $stagedir/$name
> > > +	su - "$qa_user" -c "mv $stagedir/$name $scratchdir/$name" 2>&1 | \
> > 
> > Same as [PATCH 3/4], do we need "--login"?
> > Oh, I just found there's only one case generic/128 use this option too. Anyway I
> > have no reason to object it, just speak out for review:)
> 
> <nod> I have the same response as the previous patch. ;)

Same as [PATCH 3/4], we'll help to deal with the "-".

Reviewed-by: Zorro Lang <zlang@redhat.com>

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > > +		_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
> > > +	test "${PIPESTATUS[0]}" -ne 0 && break
> > > +done
> > > +repquota -upn $SCRATCH_MNT >> $seqres.full
> > > +
> > > +# success, all done
> > > +echo Silence is golden
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/833.out b/tests/generic/833.out
> > > new file mode 100644
> > > index 00000000..d100fa07
> > > --- /dev/null
> > > +++ b/tests/generic/833.out
> > > @@ -0,0 +1,3 @@
> > > +QA output created by 833
> > > +mv: cannot move 'SCRATCH_MNT/staging/yXXX' to 'SCRATCH_MNT/dir/yXXX': Disk quota exceeded
> > > +Silence is golden
> > > 
> > 
>