Christian Brauner wrote on Wed, May 24, 2023 at 03:52:45PM +0200: > The main objection in [1] was to allow specifying an arbitrary offset > from userspace. What [3] did was to implement a pread() variant for > directories, i.e., pgetdents(). That can't work in principle/is > prohibitively complex. Which is what your series avoids by not allowing > any offsets to be specified. Yes. > However, there's still a problem here. Updates to f_pos happen under an > approriate lock to guarantee consistency of the position between calls > that move the cursor position. In the normal read-write path io_uring > doesn't concern itself with f_pos as it keeps local state in > kiocb->ki_pos. > > But then it still does end up running into f_pos consistency problems > for read-write because it does allow operating on the current f_pos if > the offset if struct io_rw is set to -1. > > In that case it does retrieve and update f_pos which should take > f_pos_lock and a patchset for this was posted but it didn't go anywhere. > It should probably hold that lock. See Jann's comments in the other > thread how that currently can lead to issues. Assuming that is this mail: https://lore.kernel.org/io-uring/CAG48ez1O9VxSuWuLXBjke23YxUA8EhMP+6RCHo5PNQBf3B0pDQ@xxxxxxxxxxxxxx/ So, ok, I didn't realize fdget_pos() actually acted as a lock on the file's f_pos_lock (apparently only if FMODE_ATMOIC_POS is set? but it looks set on most if not all directories through finish_open(), that looks called consistently enough) What was confusing is that default_llseek updates f_pos under the inode_lock (write), and getdents also takes that lock (for read only in shared implem), so I assumed getdents also was just protected by this read lock, but I guess that was a bad assumption (as I kept pointing out, a shared read lock isn't good enough, we definitely agree there) In practice, in the non-registered file case io_uring is also calling fdget, so the lock is held exactly the same as the syscall and I wasn't so far off -- but we need to figure something for the registered file case. openat variants don't allow working with the registered variant at all on the parent fd, so as far as I'm concerned I'd be happy setting the same limitation for getdents: just say it acts on fd and not files, and call it a day... It'd also be possible to check if REQ_F_FIXED_FILE flag was set and manually take the lock somehow but we don't have any primitive that takes f_pos_lock from a file (the only place that takes it is fdget which requires a fd), so I'd rather not add such a new exception. I assume the other patch you mentioned about adding that lock was this one: https://lore.kernel.org/all/20220222105504.3331010-1-dylany@xxxxxx/T/#m3609dc8057d0bc8e41ceab643e4d630f7b91bde6 and it just atkes the lock, but __fdget_pos also checks for FMODE_ATOMIC_OPS and file_count and I'm not sure I understand how it sets (f.flags & FDPUT_POS_UNLOCK) (for fdput_pos) so I'd rather not add such a code path at this point.. So, ok, what do you think about just forbidding registered files? I can't see where that wouldn't suffice but I might be missing something else. > For getdents() not protecting f_pos is equally bad or worse. The patch > doesn't hold f_pos_lock and just updates f_pos prior _and_ post > iterate_dir() arguing that this race is fine. But again, f_version and > f_pos are consistent after each system call invocation. > > But without that you can have a concurrent seek running and can end up > with an inconsistent f_pos position within the same system call. IOW, > you're breaking f_pos being in a well-known state. And you're not doing > that just for io_uring you're doing it for the regular system call > interface as well as both can be used on the same fd simultaneously. > So that's a no go imho. (so seek is fine, but I agree two concurrent getdents on registered files won't have the required lock) -- Dominique Martinet | Asmadeus
