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

[Amir Goldstein <amir73il@gmail.com>]

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