| Message ID | 20230718132112.461218-6-hao.xu@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring getdents | expand |
On Tue, Jul 18, 2023 at 09:21:12PM +0800, Hao Xu wrote: > From: Hao Xu <howeyxu@tencent.com> > > Fixed file for io_uring getdents can trigger race problem. Users can > register a file to be fixed file in io_uring and then remove other > reference so that there are only fixed file reference of that file. > And then they can issue concurrent async getdents requests or both > async and sync getdents requests without holding the f_pos_lock > since there is a f_count == 1 optimization. Afaict, that limitation isn't necessary. This version ow works fine with fixed files. Based on the commit message there seems to be a misunderstanding. Your previous version of the patchset copied the f_count optimization into io_uring's locking which would've caused the race I described in the other thread. There regular system call interface was always safe because as long as the original fd is kept the file count will be greater than 1 and both the fixed file and regular system call interface will acquire the lock. So fixed file's not being usable was entirely causes by copying the f_count optimization into io_uring. Since this patchset now doesn't use that optimization and unconditionally locks things are fine. (And even if, the point is now moot anyway since we dropped that optimization from the regular system call path anyway because of another issue.)
On 7/26/23 22:23, Christian Brauner wrote: > On Tue, Jul 18, 2023 at 09:21:12PM +0800, Hao Xu wrote: >> From: Hao Xu <howeyxu@tencent.com> >> >> Fixed file for io_uring getdents can trigger race problem. Users can >> register a file to be fixed file in io_uring and then remove other >> reference so that there are only fixed file reference of that file. >> And then they can issue concurrent async getdents requests or both >> async and sync getdents requests without holding the f_pos_lock >> since there is a f_count == 1 optimization. > > Afaict, that limitation isn't necessary. This version ow works fine with > fixed files. > > Based on the commit message there seems to be a misunderstanding. > Your previous version of the patchset copied the f_count optimization > into io_uring's locking which would've caused the race I described > in the other thread. > > There regular system call interface was always safe because as long as > the original fd is kept the file count will be greater than 1 and both > the fixed file and regular system call interface will acquire the lock. > > So fixed file's not being usable was entirely causes by copying the > f_count optimization into io_uring. Since this patchset now doesn't use > that optimization and unconditionally locks things are fine. (And even > if, the point is now moot anyway since we dropped that optimization from > the regular system call path anyway because of another issue.) I see, forgot we already remove it from fdtable after close(fd) thus cannot access it by fd any more. I'll fix it in next version. Thanks. Regards, Hao
diff --git a/io_uring/fs.c b/io_uring/fs.c index 480f25677fed..dc74676b1499 100644 --- a/io_uring/fs.c +++ b/io_uring/fs.c @@ -303,6 +303,8 @@ int io_getdents_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { struct io_getdents *gd = io_kiocb_to_cmd(req, struct io_getdents); + if (unlikely(req->flags & REQ_F_FIXED_FILE)) + return -EBADF; if (READ_ONCE(sqe->off) != 0) return -EINVAL;