Re: [PATCH] file: always lock position

[Jens Axboe <axboe@kernel.dk>]

On 7/25/23 12:30?PM, Linus Torvalds wrote:
> On Mon, 24 Jul 2023 at 15:57, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 7/24/23 4:25?PM, Linus Torvalds wrote:
>>> This sentence still worries me.
>>>
>>> Those fixed files had better have their own refcounts from being
>>> fixed. So the rules really shouldn't change in any way what-so-ever.
>>> So what exactly are you alluding to?
>>
>> They do, but they only have a single reference, which is what fixes them
>> into the io_uring file table for fixed files. With the patch from the
>> top of this thread, that should then be fine as we don't need to
>> artificially elevator the ref count more than that.
> 
> No.
> 
> The patch from the top of this thread cannot *possibly* matter for a
> io_uring fixed file.
> 
> The fdget_pos() always gets the file pointer from the file table. But
> that means that it is guaranteed to have a refcount of at least one.
> 
> If io_uring fixed file holds a reference (and not holding a reference
> would be a huge bug), that in turn means that the minimum refcount is
> now two.

Right, but what if the original app closes the file descriptor? Now you
have the io_uring file table still holding a reference to it, but it'd
just be 1. Which is enough to keep it alive, but you can still have
multiple IOs inflight against this file.

Obviously using the file position is wonky with async IO to begin with,
exactly because you can have multiple IOs in flight to it at the same
time. You can make it work by specifying ordering constraints, but that
is obviously also totally messy and not really a valid use case. Just
don't use the file position at that point.

Some libraries are limited though and want to use file positions with
async IO, and they generally get to keep both pieces if they do and
don't treat it as sync IO (or serialized, at least) at that point.

> So the code in fdget_pos() is correct, with or without the patch.
> 
> The *only* problem is when something actually violates the refcounting
> rules. Sadly, that's exactly what pidfd_getfd() does, and can
> basically make a private file pointer be non-private without
> synchronizing with the original owner of the fd.
> 
> Now, io_uring may have had its own problems, if it tried to
> re-implement some io_uring-specific version of fdget_pos() for the
> fixed file case, and thought that it could use the file_count() == 1
> trick when it *wasn't* also a file table entry.
>
> But that would be an independent bug from copy-and-pasting code
> without taking the surrounding rules into account.

We never made any assumptions on the file_count() for the file, exactly
because the count of it means nothing to io_uring in terms of whether we
can have concurrent IO to it or not.

-- 
Jens Axboe