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

[Zorro Lang <zlang@redhat.com>]

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