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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

>> +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").