On Tue, Jul 25, 2023 at 11:30:32AM -0700, Linus Torvalds wrote: > On Mon, 24 Jul 2023 at 15:57, Jens Axboe <axboe@xxxxxxxxx> 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. Yeah, the patch doesn't matter for fixed files. But they copied the logic from fdget_pos() which was the problem. > > 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. > > So the code in fdget_pos() is correct, with or without the patch. Yes. > > 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. Yes, that's what one of the patch versions did and what I pointed out won't work and what ultimately led me to discover the pidfd_getfd() bug. (That's also btw, why my explanation was long.) > > But that would be an independent bug from copy-and-pasting code > without taking the surrounding rules into account. Yes, exactly what I said in the review.
