Re: [PATCH 2/2] tools/virtio: vringh_test: check that a split ring must be a power of two

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] tools/virtio: memory leak of virtqueue in main() [1]
--

commit dacd77c2e0af2c3d6ebdce34137950d56c8f8e41
Author: Stjepan Poljak <stjepan.poljak@gmail.com>

tools/virtio: vringh_test: check that a split ring must be a power of two

This commit adds a check in vringh_test to verify that
vring_new_virtqueue() rejects a non-power-of-two split ring size.

> diff --git a/tools/virtio/vringh_test.c b/tools/virtio/vringh_test.c
> index 2a5d7f7d2d6ce..75c66385502de 100644
> --- a/tools/virtio/vringh_test.c
> +++ b/tools/virtio/vringh_test.c
> @@ -494,6 +494,17 @@ int main(int argc, char *argv[])
>  	__user_addr_max = __user_addr_min + USER_MEM;
>  	memset(__user_addr_min, 0, vring_size(RINGSIZE, ALIGN));
>  
> +	/*
> +	 * The split ring indexes its rings with num - 1 as a mask, so a
> +	 * size that is not a power of two must be refused.
> +	 */
> +	vq = vring_new_virtqueue(0, RINGSIZE - 1, ALIGN, &vdev, true, false,
> +				 __user_addr_min,
> +				 never_notify_host, never_callback_guest,
> +				 "guest vq");
> +	if (vq)
> +		errx(1, "vring_new_virtqueue accepted size %u", RINGSIZE - 1);
> +
>  	/* Set up guest side. */
>  	vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true, false,
>  				 __user_addr_min,

[Severity: Medium]
This problem wasn't introduced by this patch, but does main() leak vq
[1] when running with --indirect?

Later in main(), the VIRTIO_RING_F_INDIRECT_DESC test branch reassigns vq
without calling vring_del_virtqueue(vq) first:

tools/virtio/vringh_test.c:main() {
	...
	if (__virtio_test_bit(&vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
		...
		__virtio_clear_bit(&vdev, VIRTIO_RING_F_INDIRECT_DESC);
		vq = vring_new_virtqueue(0, RINGSIZE, ALIGN, &vdev, true,
					 false, __user_addr_min,
					 never_notify_host,
					 never_callback_guest,
					 "guest vq");
	...
}

This leaks the initial virtqueue and leaves it linked on vdev.vqs when the
cleanup path at the end of main() only deletes the second vq.

[1] https://sashiko.dev/#/bug/linux-e99ea9bc-10be-4c6a-b7f2-1998001de64b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111172947.3579982-1-stjepan.poljak@gmail.com?part=2