Re: [PATCH] file: always lock position

[Christian Brauner <brauner@kernel.org>]

On Mon, Jul 24, 2023 at 08:53:32AM -0700, Linus Torvalds wrote:
> So this was a case of "too much explanations make the explanation much
> harder to follow".
> 
> I tend to enjoy your pull request explanations, but this one was just
> *way* too much.
> 
> Please try to make the point you are making a bit more salient, so
> that it's a lot easier to follow.

Sure. The thing is though that I'm not doing this because it's fun to
write a lot of text. It's simply because I want to remember how I
arrived at that conclusion. I can keep this shorter for sure. The
problem is often just what can I assume the reader knows and what they
don't.

> 
> On Mon, 24 Jul 2023 at 08:01, Christian Brauner <brauner@kernel.org> wrote:
> >
> >     [..] the
> > file_count(file) greater than one optimization was already broken and
> > that concurrent read/write/getdents/seek calls are possible in the
> > regular system call api.
> >
> > The pidfd_getfd() system call allows a caller with ptrace_may_access()
> > abilities on another process to steal a file descriptor from this
> > process.
> 
> I think the above is all you need to actually explain the problem and
> boil down the cause of the bug, and it means that the reader doesn't
> have to wade through a lot of other verbiage to figure it out.
> 
> >         if (file && (file->f_mode & FMODE_ATOMIC_POS)) {
> > -               if (file_count(file) > 1) {
> > -                       v |= FDPUT_POS_UNLOCK;
> > -                       mutex_lock(&file->f_pos_lock);
> > -               }
> > +               v |= FDPUT_POS_UNLOCK;
> > +               mutex_lock(&file->f_pos_lock);
> >         }
> 
> Ho humm. The patch is obviously correct.
> 
> At the same time this is actually very annoying, because I played this
> very issue with the plain /proc/<pid>/fd/<xyz> interface long long
> ago, where it would just re-use the 'struct file' directly, and it was
> such a sh*t-show that I know it's much better to actually open a new
> file descriptor.
> 
> I'm not sure that "share actual 'struct file' ever was part of a
> mainline kernel". I remember having it, but it was a "last century"
> kind of thing.
> 
> The /proc interface hack was actually somewhat useful exactly because
> you'd see the file position change, but it really caused problems.
> 
> The fact that pidfd_getfd() re-introduced that garbage and I never
> realized this just annoys me no end.

SCM_RIGHTS which have existed since 2.1 or sm allow you to do the same
thing just cooperatively. If you receive a bunch of fds from another
task including sockets and so on they refer to the same struct file.

In recent kernels we also have the seccomp notifier addfd ioctl which
let's you add a file descriptor into another process which can also be
used to create shared struct files.