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

[Amir Goldstein <amir73il@gmail.com>]

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.