Re: [PATCH hci-8.1 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] vhost-blk: ids[] stack overflow in vhost_blk_set_workers() for num-queues above 32
- [Medium] vhost-blk: ENOTTY from VHOST_SET_FORK_FROM_OWNER wrongly forces a single thread
- [Low] vhost-blk: num-threads 0 and values above nvqs silently clamped in vhost_blk_start()
--

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

vhost-blk: create kernel threads with the mainstream worker ioctls

This commit replaces the VHOST_SET_NWORKERS ioctl with VHOST_NEW_WORKER and
VHOST_ATTACH_VRING_WORKER. It spreads the virtqueues round-robin over
num-threads workers. It also switches the device to vhost_task mode with
VHOST_SET_FORK_FROM_OWNER before VHOST_SET_OWNER.

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

> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> index abddff02c6d92..99fec9533a9cc 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c

[ ... ]

> +static int vhost_blk_set_workers(VHostBlk *s, int nworkers)
> +{
> +    uint32_t ids[VHOST_BLK_MAX_QUEUES];
> +    struct vhost_worker_state worker;
> +    struct vhost_vring_worker vq_worker;
> +    int ret, i;
> +
> +    for (i = 1; i < nworkers; i++) {
> +        memset(&worker, 0, sizeof(worker));
> +        if (ioctl(s->vhostfd, VHOST_NEW_WORKER, &worker)) {
> +            ret = -errno;
> +            if (ret == -ENOTTY && i == 1) {
> +                warn_report_once("vhost-blk: kernel does not support worker "
> +                                 "ioctls, using a single kernel thread");
> +                return 0;
> +            }
> +            error_report("vhost-blk: unable to create kernel thread: %s",
> +                         strerror(-ret));
> +            return ret;
> +        }
> +        ids[i] = worker.worker_id;

[Severity: High]
Can this overflow ids[]?

ids[] has VHOST_BLK_MAX_QUEUES (32) entries, but nworkers is bounded only
by nvqs. In vhost_blk_start():

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

vhost_blk_device_realize() and vhost_blk_pci_realize() clamp num-queues to
VHOST_BLK_MAX_QUEUES only when it equals VHOST_BLK_AUTO_NUM_QUEUES:

    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 value such as num-queues=64 is accepted and becomes
s->dev.nvqs. With num-threads=64, nworkers is 64.

The loop above then writes ids[i] past the end of the stack array for
i >= 32.

> +    }
> +
> +    for (i = 0; i < s->dev.nvqs; i++) {
> +        if (i % nworkers == 0) {
> +            continue;
> +        }
> +        memset(&vq_worker, 0, sizeof(vq_worker));
> +        vq_worker.index = i;
> +        vq_worker.worker_id = ids[i % nworkers];

The same configuration also reads ids[i % nworkers] out of bounds when
i % nworkers >= 32.

Should realize reject num-queues above VHOST_BLK_MAX_QUEUES, or should
ids[] be sized from nworkers or nvqs?

[ ... ]

>  static int vhost_blk_start(VirtIODevice *vdev)
>  {

[ ... ]

> +    nworkers = MIN(s->conf.num_threads, s->dev.nvqs);
> +    if (nworkers > 1) {

[Severity: Low]
Is it intended that num-threads=0 and values above nvqs are silently
clamped here?

vhost_blk_device_realize() validates num-queues but has no equivalent
check for num-threads, and the property is an unconstrained uint16:

    DEFINE_PROP_UINT16("num-threads", VHostBlk, conf.num_threads, 1),

With num-threads=0, nworkers becomes 0, the nworkers > 1 test is false,
and no worker ioctls are issued. Before this patch the old condition was
nworkers != 1, so a value of 0 reached VHOST_SET_NWORKERS.

Would a realize-time check that rejects 0, or warns when num-threads
exceeds num-queues, make the behaviour explicit? The commit message only
says that num-threads=1 is unchanged.

> +        /*
> +         * VHOST_NEW_WORKER works only in vhost_task mode, and
> +         * VHOST_RESET_OWNER brings the kthread default back.
> +         */
> +        uint8_t fork_owner = VHOST_FORK_OWNER_TASK;
> +
> +        if (ioctl(s->vhostfd, VHOST_SET_FORK_FROM_OWNER, &fork_owner)) {
> +            ret = -errno;
> +            if (ret != -ENOTTY) {
> +                error_report("vhost-blk: unable to set vhost task mode: %s",
> +                             strerror(-ret));
> +                return ret;
> +            }
> +            warn_report_once("vhost-blk: kernel does not support vhost "
> +                             "tasks, using a single kernel thread");
> +            nworkers = 1;

[Severity: Medium]
Does this -ENOTTY fallback drop a working multi-thread configuration?

linux-headers/linux/vhost.h documents VHOST_SET_FORK_FROM_OWNER as "Only
available when CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL=y". It also
documents VHOST_FORK_OWNER_TASK as the "(default value)".

A kernel built without that option returns -ENOTTY here. According to the
header, such a kernel runs in task mode, which VHOST_NEW_WORKER and
VHOST_ATTACH_VRING_WORKER need. In that case nworkers = 1 discards a
usable setup.

The commit message says "the kernel defaults to kthreads", which
contradicts the header. I could not check the kernel source for the target
kernel, so which one is correct is unverified.

vhost_blk_set_workers() already handles -ENOTTY from VHOST_NEW_WORKER when
i == 1, with its own warning and single-thread fallback. Is the earlier
-ENOTTY fallback needed, given that?

> +        }
> +    }
> +

[ ... ]

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