Re: [PATCH v2 6/6] RFC: io_uring getdents: test returning an EOF flag in CQE

[Dominique Martinet]

Christian Brauner wrote on Tue, May 23, 2023 at 04:30:14PM +0200:
> > index b15ec81c1ed2..f6222b0148ef 100644
> > --- a/io_uring/fs.c
> > +++ b/io_uring/fs.c
> > @@ -322,6 +322,7 @@ int io_getdents(struct io_kiocb *req, unsigned int issue_flags)
> >  {
> >  	struct io_getdents *gd = io_kiocb_to_cmd(req, struct io_getdents);
> >  	unsigned long getdents_flags = 0;
> > +	u32 cqe_flags = 0;
> >  	int ret;
> >  
> >  	if (issue_flags & IO_URING_F_NONBLOCK) {
> > @@ -338,13 +339,16 @@ int io_getdents(struct io_kiocb *req, unsigned int issue_flags)
> >  			goto out;
> >  	}
> >  
> > -	ret = vfs_getdents(req->file, gd->dirent, gd->count, getdents_flags);
> > +	ret = vfs_getdents(req->file, gd->dirent, gd->count, &getdents_flags);
> 
> I don't understand how synchronization and updating of f_pos works here.
> For example, what happens if a concurrent seek happens on the fd while
> io_uring is using vfs_getdents which calls into iterate_dir() and
> updates f_pos?

I don't see how different that is from a user spawning two threads and
calling getdents64 + lseek or two getdents64 in parallel?
(or any two other users of iterate_dir)

As far as I understand you'll either get the old or new pos as
obtained/updated by iterate_dir()?

That iterate_dir probably ought to be using READ_ONCE/WRITE_ONCE or some
atomic read/update wrappers as the shared case only has a read lock
around these, but that's not a new problem; and for all I care
about I'm happy to let users shoot themselves in the foot.
(although I guess that with filesystems not validating the offset as
was pointed out in a previous version comment having non-atomic update
might be a security issue at some point on architectures that don't
guarantee atomic 64bit updates, but if someone manages to abuse it
it's already possible to abuse it with the good old syscalls, so I'd
rather leave that up to someone who understand how atomicity in the
kernel works better than me...)

-- 
Dominique Martinet | Asmadeus