Re: [PATCH] file: always lock position

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

On Mon, Jul 24, 2023 at 09:51:05AM -0700, Linus Torvalds wrote:
> On Mon, 24 Jul 2023 at 09:36, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > There are magic rules with "total_refs == inflight_refs", and that
> > total_refs thing is very much the file count, ie
> >
> >                 total_refs = file_count(u->sk.sk_socket->file);
> >
> > where we had some nasty bugs with files coming back to life.
> 
> Ok, I don't think this is an issue here. It really is that "only
> in-flight refs remaining" that is a special case, and even
> pidfd_getfd() shouldn't be able to change that.
> 
> But the magic code is all in fget_task(), and those need to be checked.
> 
> You can see how proc does things properly: it does do "fget_task()",
> but then it only uses it to copy the path part, and just does fput()
> afterwards.
> 
> The bpf code does something like that too, and seems ok (ie it gets
> the file in order to copy data from it, not to install it).

Aside of fget_task() use, it has this:
        rcu_read_lock();
        for (;; curr_fd++) {
                struct file *f;
                f = task_lookup_next_fd_rcu(curr_task, &curr_fd);
                if (!f)
                        break;
                if (!get_file_rcu(f))
                        continue;

                /* set info->fd */
                info->fd = curr_fd;
                rcu_read_unlock();
                return f;
        }

curr_task is not cached current here - it can be an arbitrary thread.
And what we do to the file reference we get here is

        ctx.meta = &meta;
        ctx.task = info->task;
        ctx.fd = info->fd;
        ctx.file = file;
        return bpf_iter_run_prog(prog, &ctx);

I think it can't be used to shove it into any descriptor table, but
then there's forming an SCM_RIGHTS datagram and sending it, etc. -
it might be worth looking into.