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?
> + 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.
> +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).
> +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.
Greetings,
Andres Freund
