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

[Stefan Roesch <shr@fb.com>]

On 12/29/21 5:41 PM, 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?

I think Jens already answered this why we capture the parameters during the prep
step. From Jens:

"

- The prep of it, this happens inline from the system call where the
  request, or requests, are submitted. The prep phase should ensure that
  argument structs are stable. Hence a caller can prep a request and
  have memory on stack, as long as it submits before it becomes invalid.
  An example of that are iovecs for readv/writev. The caller does not
  need to have them stable for the duration of the request, just across
  submit. That's the io_${cmd}_prep() helpers.

- The execution of it. May be separate from prep and from an async
  worker. Where the lower layers don't support a nonblocking attempt,
  they are always done async. The statx stuff is an example of that.

Hence prep needs to copy from userland on the prep side always for the
statx family, as execution will happen out-of-line from the submission.

"

Otherwise we need to copy the path value the user passed in, storing a filename struct
seems to be the better choice.