Re: [PATCH 2/6] io_uring: add IORING_OP_PROVIDE_BUFFERS

[Jens Axboe <axboe@kernel.dk>]

On 3/9/20 11:03 AM, Andres Freund wrote:
> Hi,
> 
> I like this feature quite a bit...
> 
> Sorry for the late response.
> 
> 
> On 2020-02-28 13:30:49 -0700, Jens Axboe wrote:
>> +static int io_provide_buffers_prep(struct io_kiocb *req,
>> +				   const struct io_uring_sqe *sqe)
>> +{
>> +	struct io_provide_buf *p = &req->pbuf;
>> +	u64 tmp;
>> +
>> +	if (sqe->ioprio || sqe->rw_flags)
>> +		return -EINVAL;
>> +
>> +	tmp = READ_ONCE(sqe->fd);
>> +	if (!tmp || tmp > USHRT_MAX)
>> +		return -EINVAL;
> 
> Hm, seems like it'd be less confusing if this didn't use
> io_uring_sqe->fd, but a separate union member?

Sure, not a big deal to me, and we can make this change after the fact
too as it won't change the ABI.

>> +	p->nbufs = tmp;
>> +	p->addr = READ_ONCE(sqe->addr);
>> +	p->len = READ_ONCE(sqe->len);
>> +
>> +	if (!access_ok(u64_to_user_ptr(p->addr), p->len))
>> +		return -EFAULT;
>> +
>> +	p->gid = READ_ONCE(sqe->buf_group);
>> +	tmp = READ_ONCE(sqe->off);
>> +	if (tmp > USHRT_MAX)
>> +		return -EINVAL;
> 
> Would it make sense to return a distinct error for the >= USHRT_MAX
> cases? E2BIG or something roughly in that direction? Seems good to be
> able to recognizably refuse "large" buffer group ids.

Don't feel too strongly, but it does help to separate the error.

>> +static int io_add_buffers(struct io_provide_buf *pbuf, struct list_head *list)
>> +{
>> +	struct io_buffer *buf;
>> +	u64 addr = pbuf->addr;
>> +	int i, bid = pbuf->bid;
>> +
>> +	for (i = 0; i < pbuf->nbufs; i++) {
>> +		buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>> +		if (!buf)
>> +			break;
>> +
>> +		buf->addr = addr;
>> +		buf->len = pbuf->len;
>> +		buf->bid = bid;
>> +		list_add(&buf->list, list);
>> +		addr += pbuf->len;
>> +		bid++;
>> +	}
>> +
>> +	return i;
>> +}
> 
> Hm, aren't you loosing an error here if you kmalloc fails for i > 0?
> Afaict io_provide_buffes() only checks for ret != 0. I think userland
> should know that a PROVIDE_BUFFERS failed, even if just partially (I'd
> just make it fail wholesale).

The above one does have the issue that we're losing the error for i ==
0, current one does:

return i ? i : -ENOMEM;

But this is what most interfaces end up doing, return the number
processed, if any, or error if none of them were added. Like a short
read, for example, and you'd get EIO if you forwarded and tried again.
So I tend to prefer doing it like that, at least to me it seems more
logical than unwinding. The application won't know what buffer caused
the error if you unwind, whereas it's perfectly clear if you asked to
add 128 and we return 64 that the issue is with the 65th buffer.

>> +static int io_provide_buffers(struct io_kiocb *req, struct io_kiocb **nxt,
>> +			      bool force_nonblock)
>> +{
>> +	struct io_provide_buf *p = &req->pbuf;
>> +	struct io_ring_ctx *ctx = req->ctx;
>> +	struct list_head *list;
>> +	int ret = 0;
> 
>> +	list = idr_find(&ctx->io_buffer_idr, p->gid);
> 
> I'd rename gid to bgid or such to distinguish from other types of group
> ids, but whatever.

Noted!

-- 
Jens Axboe