[PATCH hci-8.1 0/2] vhost-blk: switch from VHOST_SET_NWORKERS to the mainstream worker ioctls

[PATCH hci-8.1 0/2] vhost-blk: switch from VHOST_SET_NWORKERS to the mainstream worker ioctls

[Alexander Ivanov]

The kernel drops our VHOST_SET_NWORKERS ioctl in favour of the
mainstream VHOST_NEW_WORKER/VHOST_ATTACH_VRING_WORKER, which vhost-blk
now supports.

Patch 1 makes vhost-blk create its num-threads workers with the
mainstream ioctls, keeping the same worker layout, and fall back to a
single thread on a kernel without them. Patch 2 drops
VHOST_SET_NWORKERS from linux-headers.

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

Alexander Ivanov (2):
  vhost-blk: create kernel threads with the mainstream worker ioctls
  linux-headers: drop VHOST_SET_NWORKERS

 hw/block/vhost-blk.c        | 75 +++++++++++++++++++++++++++++++++++--
 linux-headers/linux/vhost.h |  9 -----
 2 files changed, 71 insertions(+), 13 deletions(-)

-- 
2.53.0

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

[Alexander Ivanov]

num-threads was passed to our own VHOST_SET_NWORKERS kernel ioctl.
The kernel is dropping it in favour of the mainstream worker ioctls,
which vhost-blk now supports, so use those: create num-threads - 1
workers with VHOST_NEW_WORKER and attach them to the virtqueues
round-robin with VHOST_ATTACH_VRING_WORKER, the default worker being
the first one. This is the layout VHOST_SET_NWORKERS produced.

VHOST_NEW_WORKER works only in vhost_task mode, while the kernel
defaults to kthreads, so switch the device to vhost_task mode with
VHOST_SET_FORK_FROM_OWNER before VHOST_SET_OWNER. VHOST_RESET_OWNER
restores the default, hence this is done on every start. With
num-threads=1 nothing changes: no worker ioctls, kthread mode.

On a kernel without these ioctls warn and run a single thread rather
than fail to start vhost-blk.

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

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 hw/block/vhost-blk.c | 75 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index abddff02c6d..99fec9533a9 100644
--- a/hw/block/vhost-blk.c
+++ b/hw/block/vhost-blk.c
@@ -33,6 +33,51 @@
 #include "migration/cpr.h"
 #include "system/runstate.h"
 
+/*
+ * Spread the virtqueues round-robin over num-threads workers, the first one
+ * being the default worker that VHOST_SET_OWNER created.
+ */
+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;
+    }
+
+    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];
+        if (ioctl(s->vhostfd, VHOST_ATTACH_VRING_WORKER, &vq_worker)) {
+            ret = -errno;
+            error_report("vhost-blk: unable to attach kernel thread to "
+                         "queue %d: %s", i, strerror(-ret));
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
 static int vhost_blk_start(VirtIODevice *vdev)
 {
     VHostBlk *s = VHOST_BLK(vdev);
@@ -51,15 +96,37 @@ static int vhost_blk_start(VirtIODevice *vdev)
         return 0;
     }
 
+    nworkers = MIN(s->conf.num_threads, s->dev.nvqs);
+    if (nworkers > 1) {
+        /*
+         * 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;
+        }
+    }
+
     if (ioctl(s->vhostfd, VHOST_SET_OWNER, NULL)) {
         error_report("vhost-blk: unable to set owner");
         return -ENOSYS;
     }
 
-    nworkers = s->conf.num_threads;
-    if (nworkers != 1 && ioctl(s->vhostfd, VHOST_SET_NWORKERS, &nworkers)) {
-        error_report("vhost-blk: unable to set number of kernel threads");
-        return -EINVAL;
+    if (nworkers > 1) {
+        ret = vhost_blk_set_workers(s, nworkers);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     if (s->conf.serial) {
-- 
2.53.0

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

[no-reply]

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

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

[Alexander Ivanov]

>> +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.
Fixed.
>
>> +    }
>> +
>> +    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?
Fixed.
>
> [ ... ]
>
>>   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.
No behaviour change: the old VHOST_SET_NWORKERS handler already
clamped n to nvqs and created no extra workers for n=0, and the patch
does the same on the QEMU side.
>> +        /*
>> +         * 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?
Our kernel, unlike the mainline header, defaults to kthreads, so without
CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL VHOST_NEW_WORKER
fails with -EFAULT, which the vhost_blk_set_workers() fallback does not 
catch,
so the early -ENOTTY fallback is needed (commit message reworded to say
"unlike mainline").

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

[Alexander Ivanov]

vhost-blk no longer uses it, and the kernel drops the ioctl.

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

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 linux-headers/linux/vhost.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
index baad06412d4..b6eb628d973 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. */
 
-- 
2.53.0

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

[no-reply]

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

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

[Alexander Ivanov]

>> 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.
Correct, but this is not a regression: the old VHOST_SET_NWORKERS
failure path behaved the same way, VHOST_RESET_OWNER on device
reset or closing vhostfd frees everything, and resetting the owner on
every start error path is a separate pre-existing issue I'd fix in its own
patch.
> [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?
The clamp is already there: vhost_blk_set_workers() does
nworkers = MIN(nworkers, VHOST_BLK_MAX_QUEUES) before the loop,
so ids[] is only written and read below index 32. With more queues than
that, the extra queues share the 32 workers round-robin.