Message-ID: <CAGudoHH76NYH2O-TQw6ZPjZF5ht76HgiKtsG=owYdLZarGRwcA@mail.gmail.com>
Date: Tue, 10 Dec 2024 05:48:40 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Christian Brauner <brauner@...nel.org>, jack@...e.cz, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [MEH PATCH] fs: sort out a stale comment about races between fd
 alloc and dup2

On Mon, Dec 9, 2024 at 8:56 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> 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.

Oh huh. I had seen that code before, did not mentally register there
may be repeat file alloc/free calls due to repeat path_openat.

Indeed it would be nice if someone(tm) sorted it out, but I don't see
how this has any relation to installing the file early and thus having
fget worry about it.

Suppose the "embryo"/"larval" file pointer is to be installed early
and populated later. I don't see a benefit but do see a downside: this
requires protection against close() on the fd (on top of dup2 needed
now).
The options that I see are:
- install the file with a refcount of 2, let dup2/close whack it, do a
fput in open to bring back to 1 or get rid of it if it raced (yuck)
(freebsd is doing this)
- dup2 is already special casing to not mess with it, add that to
close as well (also yuck imo)

>From userspace side the only programs which can ever see EBUSY are
buggy or trying to screw the kernel, so not a concern on that front.

Now something amusing, I did not realize I had a super stale copy of
the OpenBSD source code hanging around -- they stopped pre-installing
files in 2018! Instead they install late and do the in dup2 returning
EBUSY, i.e. the same thing as Linux. I do have up to date FreeBSD and
NetBSD though. :)

Christian, would you mind massaging the OS entries in the commit
message (or should i send a v2?):
- OpenBSD installing a larval file -- they moved away from it, file is
installed late and EBUSY is returned on conflict
- FreeBSD returning EBADF -- reworked to install the file early like
OpenBSD used to do
-- 
Mateusz Guzik <mjguzik gmail.com>

Powered by blists - more mailing lists