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
