Re: [MEH PATCH] fs: sort out a stale comment about races between fd alloc and dup2

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Dec 06, 2024 at 01:13:47PM +0100, Christian Brauner wrote:
> On Thu, 05 Dec 2024 16:47:43 +0100, Mateusz Guzik wrote:
> > It claims the issue is only relevant for shared descriptor tables which
> > is of no concern for POSIX (but then is POSIX of concern to anyone
> > today?), which I presume predates standarized threading.
> > 
> > The comment also mentions the following systems:
> > - OpenBSD installing a larval file -- this remains accurate
> > - FreeBSD returning EBADF -- not accurate, the system uses the same
> >   idea as OpenBSD
> > - NetBSD "deadlocks in amusing ways" -- their solution looks
> >   Solaris-inspired (not a compliment) and I would not be particularly
> >   surprised if it indeed deadlocked, in amusing ways or otherwise

FWIW, the note about OpenBSD approach is potentially still interesting,
but probably not in that place...

These days "not an embryo" indicator would probably map to FMODE_OPENED,
so fget side would be fairly easy (especially if we invert that bit -
then the same logics we have for "don't accept FMODE_PATH" would apply
verbatim).

IIRC, the last time I looked into it the main obstacle to untangle had
boiled down to "how do we guarantee that do_dentry_open() failing with
-ESTALE will leave struct file in pristine state" and _that_ got boggled
down in S&M shite - ->file_open() is not idempotent and has no reverse
operation - ->file_free_security() takes care of everything.

If that gets solved, we could lift alloc_empty_file() out of path_openat()
into do_filp_open()/do_file_open_root() - it would require a non-trivial
amount of massage, but a couple of years ago that appeared to have been
plausible; would need to recheck that...  It would reduce the amount of
free/reallocate cycles for struct file in those, which might make it
worthwhile on its own.

Lifting it further would require some massage in the callers, but that
would be on per-caller basis; used to look plausible...

Hell knows - right now I'm ears-deep in ->d_revalidate() crap, but once
that settles down a bit...  Might be worth looking into that again.

LSM ->file_open() behaviour changes would need to be discussed with LSM
crowd, obviously.  Ideally we want it idempotent, so that calling it
twice in a row would have the second call work in the same way as if
the first one hadn't happened.  In-tree instances seem to be trivial
to massage to that (in the worst case you'd need to clear some fields
if the first call hadn't taken a fast path out, but the second one
had), but that really needs a buy-in from maintainers.