Re: [PATCH] file: always lock position

[Christian Brauner <brauner@kernel.org>]

On Mon, Jul 24, 2023 at 11:27:29AM -0700, Linus Torvalds wrote:
> On Mon, 24 Jul 2023 at 11:05, Jens Axboe <axboe@kernel.dk> wrote:
> >
> > io_uring never does that isn't the original user space creator task, or
> > from the io-wq workers that it may create. Those are _always_ normal
> > threads. There's no workqueue/kthread usage for IO or file
> > getting/putting/installing/removing etc.
> 
> That's what I thought. But Christian's comments about the io_uring
> getdents work made me worry.

Sorry, my point was that an earlier version of the patchset _would_ have
introduced a locking scheme that would've violated locking assumptions
because it didn't get the subtle refcount difference right. But that was
caught during review.

It's really just keeping in mind that refcount rules change depending on
whether fds or fixed files are used.

> 
> If io_uring does everything right, then the old "file_count(file) > 1"
> test in __fdget_pos() - now sadly removed - should work just fine for
> any io_uring directory handling.

Yes, but only if you disallow getdents with fixed files when the
reference count rules are different.

> 
> It may not be obvious when you look at just that test in a vacuum, but
> it happens after __fdget_pos() has done the
> 
>         unsigned long v = __fdget(fd);
>         struct file *file = (struct file *)(v & ~3);
> 
> and if it's a threaded app - where io_uring threads count the same -
> then the __fdget() in that sequence will have incremented the file
> count.
> 
> So when it then used to do that
> 
>                 if (file_count(file) > 1) {
> 
> that "> 1" included not just all the references from dup() etc,  but
> for any threaded use where we have multiple references to the file
> table it also that reference we get from __fdget() -> __fget() ->
> __fget_files() -> __fget_files_rcu() -> get_file_rcu() ->
> atomic_long_inc_not_zero(&(x)->f_count)).

Yes, for the regular system call path it's all well and dandy and I said
as much on the getdents thread as well.