Re: [PATCH 3/3] tools/virtio: add --features-array switch

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The kernel implementation of VHOST_GET_FEATURES_ARRAY zeroes out the returned features instead of the tail, causing incorrect ioctl behavior and causing the newly added test to fail.
--

--- Patch [3]: [PATCH 3/3] tools/virtio: add --features-array switch ---
commit 62c148ff31dfbcede0d6da6c8873dc84602618f3
Author: Yufeng Wang <[email protected]>

tools/virtio: add --features-array switch

This patch adds a --features-array switch to vhost_net_test to exercise the
VHOST_SET_FEATURES_ARRAY and VHOST_GET_FEATURES_ARRAY ioctls. The test verifies
that the kernel correctly copies the features and clears the trailing space when
the requested count exceeds the kernel's defined feature entries.

> diff --git a/tools/virtio/vhost_net_test.c b/tools/virtio/vhost_net_test.c
> index 389d99a6d7c76..77191e60a7a35 100644
> --- a/tools/virtio/vhost_net_test.c
> +++ b/tools/virtio/vhost_net_test.c
[ ... ]
> @@ -467,11 +491,43 @@ static void help(int status)
[ ... ]
> +static void verify_get_features_array(struct vdev_info *dev)
> +{
> +	struct vhost_features_array *clean, *dirty;
> +	int r, i;
> +
> +	clean = calloc(1, struct_size(clean, features, GET_FEATURES_ARRAY_COUNT));
> +	assert(clean);
> +	clean->count = GET_FEATURES_ARRAY_COUNT;
> +	r = ioctl(dev->control, VHOST_GET_FEATURES_ARRAY, clean);
> +	assert(r >= 0);
> +
> +	/* Sentinel-fill so the kernel's clear_user() of the tail is visible. */
> +	dirty = calloc(1, struct_size(dirty, features, GET_FEATURES_ARRAY_COUNT));
> +	assert(dirty);
> +	dirty->count = GET_FEATURES_ARRAY_COUNT;
> +	memset(dirty->features, 0xa5, GET_FEATURES_ARRAY_COUNT * sizeof(dirty->features[0]));
> +	r = ioctl(dev->control, VHOST_GET_FEATURES_ARRAY, dirty);
> +	assert(r >= 0);
> +
> +	/* Must match; a wrong clear_user() start would leave them unequal. */
> +	for (i = 0; i < GET_FEATURES_ARRAY_COUNT; i++)
> +		assert(clean->features[i] == dirty->features[i]);

[Severity: High]
This is a pre-existing issue, but will this assert consistently fail against the
current kernel implementation of VHOST_GET_FEATURES_ARRAY?

Looking at vhost_net_ioctl() in drivers/vhost/net.c, when handling the array
features and zeroing the trailing space, it appears to start at argp:

        /* Zero the trailing space provided by user-space, if any */
        if (clear_user(argp, size_mul(count - copied, sizeof(u64))))
                return -EFAULT;

Because argp is not advanced by copied * sizeof(u64), clear_user() zeroes out
the newly copied features at the beginning of the array instead of the tail.

Since the kernel implementation leaves the actual trailing space uninitialized,
will this cause the test introduced here to always crash via the assertion?

> +
> +	printf("GET_FEATURES_ARRAY: 0x%llx 0x%llx (tail zeroed) OK\n",
> +	       (unsigned long long)clean->features[0],
> +	       (unsigned long long)clean->features[1]);
> +
> +	free(clean);
> +	free(dirty);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3