On Tue, Jul 25, 2023 at 11:30:32AM -0700, 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.
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.