Re: [Qemu-devel] virtio-blk: Something bizarre with VIRTIO_BLK_T_GET_ID

[Anthony Liguori <aliguori@us.ibm.com>]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> Hi Anthony !
>
> I was looking at virtio-blk.c as an example of some details regarding
> the use of virtio queues. One thing I'm implementing is a
> request/reponse model similar to what it does.
>
> One thing I noticed that sounds off to me but I might have missed
> something is the handling of the "GET_ID" request. Qemu does:
>
>     } else if (type & VIRTIO_BLK_T_GET_ID) {
>         VirtIOBlock *s = req->dev;
>
>         /*
>          * NB: per existing s/n string convention the string is
>          * terminated by '\0' only when shorter than buffer.
>          */
>         strncpy(req->elem.in_sg[0].iov_base,
>                 s->blk->serial ? s->blk->serial : "",
>                 MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
>         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
>         g_free(req);
>     } ...
>
> So it basically writes up to VIRTIO_BLK_ID_BYTES bytes (which is 20)
> into the "in" iov (it doesn't walk the sg list, so it's a bit fishy,
> it assumes the guest is using a single entry here but that's not my
> problem).
>
> However, virtio_blk_req_complete() does:
>
>     virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
>
> So it pushes into the queue req->qiov.size (which is 0) + sizeof(*req->in)
> which is as far as I can tell ... 16.

It's completely bogus.  Non-read/write commands probably need special
handling for push.

Regards,

Anthony Liguori

>
> So we don't push enough bytes out basically for the full 20 bytes allowed
> for the ID.
>
> Or am I missing something ?
>
> Cheers,
> Ben.