Skip to content

Commit

Permalink
libqos/virtio.c: init all elems in qvring_indirect_desc_setup()
Browse files Browse the repository at this point in the history
The loop isn't setting the values for the last element. Every other
element is being initialized with addr = 0, flags = VRING_DESC_F_NEXT
and next = i + 1. The last elem is never touched.

This became a problem when enabling a RISC-V 'virt' libqos machine in
the 'indirect' test of virti-blk-test.c. The 'flags' for the last
element will end up being an odd number (since we didn't touch it).
Being an odd number it will be mistaken by VRING_DESC_F_NEXT, which
happens to be 1.

Deep into hw/virt/virtio.c, in virtqueue_split_pop(), into
virtqueue_split_read_next_desc(), a check for VRING_DESC_F_NEXT will be
made to see if we're supposed to chain. The code will keep up chaining
in the last element because the uninitialized value happens to be odd.
We'll error out right after that because desc->next (which is also
uninitialized) will be >= max. A VIRTQUEUE_READ_DESC_ERROR will be
returned, with an error message like this in the stderr:

qemu-system-riscv64: Desc next is 49391

Since we never returned, we'll end up timing out at qvirtio_wait_used_elem():

ERROR:../tests/qtest/libqos/virtio.c:236:qvirtio_wait_used_elem:
    assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)

The root cause is using uninitialized values from guest_alloc() in
qvring_indirect_desc_setup(). There's no guarantee that the memory pages
retrieved will be zeroed, so we can't make assumptions. In fact, commit
5b4f72f ("tests/qtest: properly initialise the vring used idx") fixed a
similar problem stating "It is probably not wise to assume guest memory
is zeroed anyway". I concur.

Initialize all elems in qvring_indirect_desc_setup().

Fixes: f294b02 ("libqos: Added indirect descriptor support to virtio implementation")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240217192607.32565-2-dbarboza@ventanamicro.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
  • Loading branch information
danielhb authored and huth committed Mar 1, 2024
1 parent c0c6a0e commit 2791490
Showing 1 changed file with 19 additions and 6 deletions.
25 changes: 19 additions & 6 deletions tests/qtest/libqos/virtio.c
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,27 @@ QVRingIndirectDesc *qvring_indirect_desc_setup(QTestState *qs, QVirtioDevice *d,
indirect->elem = elem;
indirect->desc = guest_alloc(alloc, sizeof(struct vring_desc) * elem);

for (i = 0; i < elem - 1; ++i) {
for (i = 0; i < elem; ++i) {
/* indirect->desc[i].addr */
qvirtio_writeq(d, qs, indirect->desc + (16 * i), 0);
/* indirect->desc[i].flags */
qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
VRING_DESC_F_NEXT);
/* indirect->desc[i].next */
qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);

/*
* If it's not the last element of the ring, set
* the chain (VRING_DESC_F_NEXT) flag and
* desc->next. Clear the last element - there's
* no guarantee that guest_alloc() will do it.
*/
if (i != elem - 1) {
/* indirect->desc[i].flags */
qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12,
VRING_DESC_F_NEXT);

/* indirect->desc[i].next */
qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, i + 1);
} else {
qvirtio_writew(d, qs, indirect->desc + (16 * i) + 12, 0);
qvirtio_writew(d, qs, indirect->desc + (16 * i) + 14, 0);
}
}

return indirect;
Expand Down

0 comments on commit 2791490

Please sign in to comment.