Re: [PATCH v4 4/5] io_uring: add fsetxattr and setxattr support

[Pavel Begunkov <asml.silence@gmail.com>]

On 12/15/21 22:17, Stefan Roesch wrote:
> This adds support to io_uring for the fsetxattr and setxattr API.

Apart from potentially putname comment,

Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>

> 
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>   fs/io_uring.c                 | 171 ++++++++++++++++++++++++++++++++++
>   include/uapi/linux/io_uring.h |   6 +-
>   2 files changed, 176 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 5092dfe56da6..fc2239635342 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
[...]> +static int __io_setxattr_prep(struct io_kiocb *req,
> +			const struct io_uring_sqe *sqe,
> +			struct user_namespace *user_ns)
> +{
> +	struct io_xattr *ix = &req->xattr;
> +	const char __user *name;
> +	void *ret;
> +
> +	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
> +		return -EINVAL;
> +	if (unlikely(sqe->ioprio))
> +		return -EINVAL;
> +	if (unlikely(req->flags & REQ_F_FIXED_FILE))
> +		return -EBADF;
> +
> +	ix->filename = NULL;
> +	name = u64_to_user_ptr(READ_ONCE(sqe->addr));
> +	ix->ctx.value = u64_to_user_ptr(READ_ONCE(sqe->addr2));
> +	ix->ctx.size = READ_ONCE(sqe->len);
> +	ix->ctx.flags = READ_ONCE(sqe->xattr_flags);
> +
> +	ix->ctx.kname = kmalloc(XATTR_NAME_MAX + 1, GFP_KERNEL);
> +	if (!ix->ctx.kname)
> +		return -ENOMEM;> +	ix->ctx.kname_sz = XATTR_NAME_MAX + 1;

Might make sense to let the userspace specify kname size, but
depends what is the average name length and how much of free
space we have in io_xattr

[...]
> +static int io_setxattr(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);
> +	putname(ix->filename);

It putname() multiple times on retry, how does it work?


> +	if (!ret) {
> +		ret = __io_setxattr(req, issue_flags, &path);
> +		path_put(&path);
> +		if (retry_estale(ret, lookup_flags)) {
> +			lookup_flags |= LOOKUP_REVAL;
> +			goto retry;
> +		}
> +	}
> +
> +	req->flags &= ~REQ_F_NEED_CLEANUP;
> +	kfree(ix->ctx.kname);
> +
> +	if (ix->value)
> +		kvfree(ix->value);
> +	if (ret < 0)
> +		req_set_fail(req);
> +
> +	io_req_complete(req, ret);
> +	return 0;
> +}
> +
[...]
>   
>   	printk_once(KERN_WARNING "io_uring: unhandled opcode %d\n",
> @@ -6715,6 +6870,15 @@ static void io_clean_op(struct io_kiocb *req)
>   			putname(req->hardlink.oldpath);
>   			putname(req->hardlink.newpath);
>   			break;
> +		case IORING_OP_SETXATTR:
> +			if (req->xattr.filename)
> +				putname(req->xattr.filename);
> +			fallthrough;
> +		case IORING_OP_FSETXATTR:
> +			kfree(req->xattr.ctx.kname);
> +			if (req->xattr.value)
> +				kvfree(req->xattr.value);

nit: it's slow path and kvfree() handles NULLs, so we don't
really need NULL checks here.

-- 
Pavel Begunkov