Re: [PATCH 1/6] libqos/virtio.c: init all elems in qvring_indirect_desc_setup()

[Alistair Francis]

On Wed, Feb 14, 2024 at 5:18 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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 unintialized 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, w'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 unintialized 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
> 5b4f72f5e8 ("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: f294b029aa ("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>

Alistair

> ---
>  tests/qtest/libqos/virtio.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
> index 410513225f..4f39124eba 100644
> --- a/tests/qtest/libqos/virtio.c
> +++ b/tests/qtest/libqos/virtio.c
> @@ -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;
> --
> 2.43.0
>
>