Re: [PATCH hci-8.1 2/2] linux-headers: drop VHOST_SET_NWORKERS

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vhost-blk: vhost_blk_start() leaks owner and workers on worker setup failure
- [Medium] vhost-blk: ids[] stack overflow in vhost_blk_set_workers() when num-queues/num-threads > 32
--

commit c5f15a317fc4cac3e42d0133296a66fbe1a1ca7d
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

linux-headers: drop VHOST_SET_NWORKERS

This commit removes the VHOST_SET_NWORKERS ioctl definition from
linux-headers/linux/vhost.h, since vhost-blk no longer uses it and the
kernel drops the ioctl.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-107537

> diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
> index baad06412d451..b6eb628d97389 100644
> --- a/linux-headers/linux/vhost.h
> +++ b/linux-headers/linux/vhost.h
> @@ -102,15 +102,6 @@
>  #define VHOST_GET_VRING_WORKER _IOWR(VHOST_VIRTIO, 0x16,		\
>  				     struct vhost_vring_worker)
>  
> -/* Set number of vhost workers
> - * Currently number of vhost workers can only be increased.
> - * All workers are freed upon reset.
> - * If the value is too big it is silently truncated to the maximum number of
> - * supported vhost workers
> - * Even if the error is returned it is possible that some workers were created
> - */
> -#define VHOST_SET_NWORKERS _IOW(VHOST_VIRTIO, 0x1F, int)
> -
>  /* The following ioctls use eventfd file descriptors to signal and poll
>   * for events. */

[Severity: Medium]
The two comments below are not about the lines removed by this header
change. They concern hw/block/vhost-blk.c as modified by the preceding
commit "vhost-blk: create kernel threads with the mainstream worker
ioctls", which added vhost_blk_set_workers(). This patch does not touch
that code, and no later commit in the series fixes either point.

Can vhost_blk_start() leak the vhost owner and workers when worker setup
fails?

hw/block/vhost-blk.c:vhost_blk_start() {
    ...
    if (ioctl(s->vhostfd, VHOST_SET_OWNER, NULL)) {
        error_report("vhost-blk: unable to set owner");
        return -ENOSYS;
    }

    if (nworkers > 1) {
        ret = vhost_blk_set_workers(s, nworkers);
        if (ret < 0) {
            return ret;
        }
    }
    ...
}

If VHOST_NEW_WORKER fails in vhost_blk_set_workers() (for example with
-ENOMEM, a thread limit, or -ENOTTY), or a mid-loop
VHOST_ATTACH_VRING_WORKER fails, the error is returned with the owner set
and some workers already created or attached. Neither the helper nor
vhost_blk_start() issues VHOST_RESET_OWNER or VHOST_FREE_WORKER.

s->vhost_started is still false at that point, so vhost_blk_stop() returns
early:

hw/block/vhost-blk.c:vhost_blk_stop() {
    if (!s->vhost_started) {
        return;
    }
    ...
}

A retry of vhost_blk_start() before a reset would then fail, because
VHOST_SET_OWNER on an already owned device returns an error that is
reported as -ENOSYS.

The resources are not permanently leaked. vhost_blk_reset() issues
VHOST_RESET_OWNER on guest device reset and on the PAUSED and
FINISH_MIGRATE handlers, and closing vhostfd in vhost_dev_cleanup() at
unrealize frees everything.

The serial-failure path (return -EINVAL after VHOST_SET_OWNER) and the
set_guest_notifiers failure path have the same missing-reset pattern.
That pattern predates the preceding commit, as did the old
VHOST_SET_NWORKERS failure return.

[Severity: Medium]
Can vhost_blk_set_workers() overflow the ids[] array on the stack?

hw/block/vhost-blk.c:vhost_blk_set_workers() {
    uint32_t ids[VHOST_BLK_MAX_QUEUES];
    ...
    for (i = 1; i < nworkers; i++) {
        ...
        ids[i] = worker.worker_id;
    }
    ...
}

ids[] has VHOST_BLK_MAX_QUEUES (32) entries, and the loop writes ids[i]
for every i below nworkers with no bound check. The only limit on
nworkers is in vhost_blk_start():

    nworkers = MIN(s->conf.num_threads, s->dev.nvqs);

Neither input is capped at 32. In vhost_blk_device_realize() the
VHOST_BLK_MAX_QUEUES cap is applied only in the
VHOST_BLK_AUTO_NUM_QUEUES branch:

    if (conf->num_queues == VHOST_BLK_AUTO_NUM_QUEUES) {
        conf->num_queues = MIN(virtio_pci_optimal_num_queues(0),
                               VHOST_BLK_MAX_QUEUES);
    }

An explicit num-queues above 32 is only checked for being non-zero and
then flows into s->dev.nvqs. num-threads is a plain DEFINE_PROP_UINT16
in vhost_blk_properties with no upper bound.

With an explicit num-queues=N (33 or more) and num-threads=M (33 or
more), nworkers can exceed 32, and the writes to ids[i] go past the end of
the array. The later attach loop reads ids[i % nworkers] and can also read
out of bounds.

This needs QEMU command line or QMP device_add access, so it is not
reachable from the guest. Would it make sense to clamp nworkers, and
reject num-queues above VHOST_BLK_MAX_QUEUES, in
vhost_blk_device_realize() for explicit values as well?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261027113857.2928048-1-alexander.ivanov@virtuozzo.com?part=2