Re: [PATCH v10 5/5] io_uring: add fgetxattr and getxattr support

[Al Viro <viro@zeniv.linux.org.uk>]

On Thu, Dec 30, 2021 at 01:41:35AM +0000, Al Viro wrote:
> On Wed, Dec 29, 2021 at 12:30:02PM -0800, Stefan Roesch wrote:
> 
> > +static int io_getxattr(struct io_kiocb *req, unsigned int issue_flags)
> > +{
> > +	struct io_xattr *ix = &req->xattr;
> > +	unsigned int lookup_flags = LOOKUP_FOLLOW;
> > +	struct path path;
> > +	int ret;
> > +
> > +	if (issue_flags & IO_URING_F_NONBLOCK)
> > +		return -EAGAIN;
> > +
> > +retry:
> > +	ret = do_user_path_at_empty(AT_FDCWD, ix->filename, lookup_flags, &path);
> > +	if (!ret) {
> > +		ret = do_getxattr(mnt_user_ns(path.mnt),
> > +				path.dentry,
> > +				ix->ctx.kname->name,
> > +				(void __user *)ix->ctx.value,
> > +				ix->ctx.size);
> > +
> > +		path_put(&path);
> > +		if (retry_estale(ret, lookup_flags)) {
> > +			lookup_flags |= LOOKUP_REVAL;
> > +			goto retry;
> > +		}
> > +	}
> > +	putname(ix->filename);
> > +
> > +	__io_getxattr_finish(req, ret);
> > +	return 0;
> > +}
> 
> Looking at that one...  Is there any reason to have that loop (from retry: to
> putname() call) outside of fs/xattr.c?  Come to think of that, why bother
> polluting your struct io_xattr with ->filename?
> 
> Note, BTW, that we already have this:
> static ssize_t path_getxattr(const char __user *pathname,
>                              const char __user *name, void __user *value,
> 			     size_t size, unsigned int lookup_flags)
> {
> 	struct path path;
> 	ssize_t error;
> retry:
> 	error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path);
> 	if (error)
> 		return error;
> 	error = getxattr(mnt_user_ns(path.mnt), path.dentry, name, value, size);
> 	path_put(&path);
> 	if (retry_estale(error, lookup_flags)) {
> 		lookup_flags |= LOOKUP_REVAL;
> 		goto retry;
> 	}
> 	return error;
> }
> in there.  The only potential benefit here would be to avoid repeated getname
> in case of having hit -ESTALE and going to repeat the entire fucking pathwalk
> with maximal paranoia, asking the server(s) involved to revalidate on every
> step, etc.
> 
> If we end up going there, who the hell *cares* about the costs of less than
> a page worth of copy_from_user()?  We are already on a very slow path as it
> is, so what's the point?

BTW, if the answer is along the lines of "we want to copy the name in prep
phase fo $REASONS", I would like to hear what it is that makes getxattr()
different from statx() in that respect.