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

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

[Benjamin Herrenschmidt]

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.

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.

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

[Stefan Hajnoczi]

On Wed, Aug 1, 2012 at 5:54 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> 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.
>
> So we don't push enough bytes out basically for the full 20 bytes allowed
> for the ID.
>
> Or am I missing something ?

The len field is mostly informational.  The virtio device driver
inside the guest may make use of it.  In many cases it doesn't so an
incorrect len value has no effect.  In
drivers/block/virtio_blk.c:blk_done() the len variable is unused.

QEMU should call cpu_physical_memory_unmap() with the correct size
value in hw/virtio.c:virtqueue_fill() so that the memory dirty bitmap
is kept up-to-date.  This is the only bad side-effect I can see here.

Stefan

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

[Benjamin Herrenschmidt]

On Wed, 2012-08-01 at 11:16 +0100, Stefan Hajnoczi wrote:

> The len field is mostly informational.  The virtio device driver
> inside the guest may make use of it.  In many cases it doesn't so an
> incorrect len value has no effect.  In
> drivers/block/virtio_blk.c:blk_done() the len variable is unused.

Right but arguably the guest reading more than the len passed back into
the descriptor is itself a bug :-)

> QEMU should call cpu_physical_memory_unmap() with the correct size
> value in hw/virtio.c:virtqueue_fill() so that the memory dirty bitmap
> is kept up-to-date.  This is the only bad side-effect I can see here.

With the current guest driver ... another one adhering strictly to the
spec might get bitten :-) Anyway, it's minor, but probably somebody
should fix. I don't have time right now, but if you don't beat me to it
I might try to give it a spin tomorrow.

Cheers,
Ben.

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

[Stefan Hajnoczi]

On Wed, Aug 1, 2012 at 11:27 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2012-08-01 at 11:16 +0100, Stefan Hajnoczi wrote:
>
>> The len field is mostly informational.  The virtio device driver
>> inside the guest may make use of it.  In many cases it doesn't so an
>> incorrect len value has no effect.  In
>> drivers/block/virtio_blk.c:blk_done() the len variable is unused.
>
> Right but arguably the guest reading more than the len passed back into
> the descriptor is itself a bug :-)
>
>> QEMU should call cpu_physical_memory_unmap() with the correct size
>> value in hw/virtio.c:virtqueue_fill() so that the memory dirty bitmap
>> is kept up-to-date.  This is the only bad side-effect I can see here.
>
> With the current guest driver ... another one adhering strictly to the
> spec might get bitten :-) Anyway, it's minor, but probably somebody
> should fix. I don't have time right now, but if you don't beat me to it
> I might try to give it a spin tomorrow.

Added to my TODO list but I probably won't get a chance before
tomorrow either because I need to focus on QEMU 1.2 soft freeze.

Stefan

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

[Anthony Liguori]

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.