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

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

>> 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.