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

[Stefan Roesch <shr@fb.com>]

On 12/29/21 6:17 PM, Al Viro wrote:
> On Wed, Dec 29, 2021 at 12:30:01PM -0800, Stefan Roesch wrote:
> 
>> +static int __io_setxattr_prep(struct io_kiocb *req,
>> +			const struct io_uring_sqe *sqe)
>> +{
>> +	struct io_xattr *ix = &req->xattr;
>> +	const char __user *name;
>> +	int 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.kvalue = NULL;
>> +	ix->ctx.size = READ_ONCE(sqe->len);
>> +	ix->ctx.flags = READ_ONCE(sqe->xattr_flags);
>> +
>> +	ix->ctx.kname = kmalloc(sizeof(*ix->ctx.kname), GFP_KERNEL);
>> +	if (!ix->ctx.kname)
>> +		return -ENOMEM;
>> +
>> +	ret = setxattr_copy(name, &ix->ctx);
>> +	if (ret) {
>> +		kfree(ix->ctx.kname);
>> +		return ret;
>> +	}
>> +
>> +	req->flags |= REQ_F_NEED_CLEANUP;
>> +	return 0;
>> +}
> 
> OK, so you
> 	* allocate a buffer for xattr name
> 	* have setxattr_copy() copy the name in *and* memdup the contents
> 	* on failure, you have the buffer for xattr name freed and return
> an error.  memdup'ed stuff is left for cleanup, presumably.
> 
>> +static int io_setxattr_prep(struct io_kiocb *req,
>> +			const struct io_uring_sqe *sqe)
>> +{
>> +	struct io_xattr *ix = &req->xattr;
>> +	const char __user *path;
>> +	int ret;
>> +
>> +	ret = __io_setxattr_prep(req, sqe);
>> +	if (ret)
>> +		return ret;
>> +
>> +	path = u64_to_user_ptr(READ_ONCE(sqe->addr3));
>> +
>> +	ix->filename = getname_flags(path, LOOKUP_FOLLOW, NULL);
>> +	if (IS_ERR(ix->filename)) {
>> +		ret = PTR_ERR(ix->filename);
>> +		ix->filename = NULL;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> ... and here you use it and bring the pathname in.  Should the latter
> step fail, you restore ->filename to NULL and return an error.
> 
> Could you explain what kind of magic could allow the caller to tell
> whether ix->ctx.kname needs to be freed on error?  I don't see any way
> that could possibly work...

At the end of the function __io_setxattr_prep() we set the flag REQ_F_NEED_CLEANUP.

If the processing fails for some reason, the cleanup code in io_clean_op() gets called
and the data structures get de-allocated.

In case the request is processed successfully, the memory gets de-allocated in io_setxattr()
and io_fsetxattr() with the helper function __io_setxattr_finish(). The helper function clears
the flag REQ_F_NEED_CLEANUP, so clean up is not necessary.

This is the general pattern of cleanup in io-uring.

I can certainly add a cleanup function, that is called in all 3 cases:
- io_setxattr,
- io_fsetxattr
- io_clean_op