On Tue, Jul 25, 2023 at 01:51:37PM -0700, Linus Torvalds wrote: > On Tue, 25 Jul 2023 at 13:41, Jens Axboe <axboe@xxxxxxxxx> wrote: > > > > 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. > > Note that fdget_pos() fundamentally only works on file descriptors > that are there - it's literally looking them up in the file table as > it goes along. And it looks at the count of the file description as it > is looked up. So that refcount is guaranteed to exist. > > If the file has been closed, fdget_pos() will just fail because it > doesn't find it. > > And if it's then closed *afterwards*, that's fine and doesn't affect > anything, because the locking has been done and we saved away the > status bit as FDPUT_POS_UNLOCK, so the code knows to unlock even if > the file descriptor in the meantime has turned back to having just a > single refcount. Yes, and to summarize which I tried in my description for the commit. The getdents support patchset would have introduced a bug because the patchset copied the fdget_pos() file_count(file) > 1 optimization into io_uring. That works fine as long as the original file descriptor used to register the fixed file is kept. The locking will work correctly as file_count(file) > 1 and no races are possible neither via getdent calls using the original file descriptor nor via io_uring using the fixed file or even mixing both. But as soon as the original file descriptor is closed the f_count for the file drops back to 1 but continues to be usable from io_uring via the fixed file. Now the optimization that the patchset wanted to copy over would cause bugs as multiple racing getdent requests would be possible using the fixed file. The simple thing ofc is that io_uring just always grabs the position lock and never does this optimization. The authors were just unaware because it wasn't obvious to them that fixed files would not work with the optimization. I caught that during review but then immediately realized that the file_count(file) > 1 optimization was unfortunately broken in another way, which ultimately led to this commit.
