Re: [PATCH] vhost-scsi: initialize command pools for late I/O virtqueues

[Mike Christie <michael.christie@oracle.com>]

On 9/16/26 1:46 AM, Linfeng Sun wrote:
> On Tue, Sep 15, 2026 at 04: 38: 38PM -0500, Mike Christie wrote: > On 9/8/26 9: 30 AM, Linfeng Sun wrote: > > vhost_scsi_set_endpoint() allocates command pools only for I/O virtqueues that > > are already set up, but publishes the
> 
> 
> On Tue, Sep 15, 2026 at 04:38:38PM -0500, Mike Christie wrote:
>> On 9/8/26 9:30 AM, Linfeng Sun wrote:
>> > vhost_scsi_set_endpoint() allocates command pools only for I/O virtqueues that
>> > are already set up, but publishes the backend for every virtqueue. If userspace
>> > finishes configuring a skipped queue later, a kick reaches vhost_scsi_get_cmd() with
>> >
>> >
>> > vhost_scsi_set_endpoint() allocates command pools only for I/O virtqueues
>> > that are already set up, but publishes the backend for every virtqueue. If
>> > userspace finishes configuring a skipped queue later, a kick reaches
>> > vhost_scsi_get_cmd() with scsi_tags uninitialized and triggers the
>> > !sb->alloc_hint warning in sbitmap_get().
>> >
>> > Leave skipped I/O virtqueues inactive at SET_ENDPOINT. When SET_VRING_ADDR
>> > later completes their setup, allocate the command pools and activate the
>> > queues. Roll back both if access initialization fails.
>> >
>> > Fixes: 25b98b64e284 ("vhost scsi: alloc cmds per vq instead of session")
>> > Signed-off-by: Linfeng Sun <linfeng.sun.dev@gmail.com>
>> > ---
>> > I checked QEMU and found no evidence it's affected. I haven't checked
>> > VirtualBox because I am not familiar with it. However, the virtio spec
>> > doesn't require a specific operation order, so a new VMM could still
>> > trigger this issue.
>> >
>> > Crash log:
>> > [   34.698961] ------------[ cut here ]------------
>> > [   34.699385] WARNING: lib/sbitmap.c:286 at sbitmap_get+0x227/0x270, CPU#0: vhost-82/88
>> > [   34.700542] Modules linked in:
>> > [   34.701309] CPU: 0 UID: 0 PID: 88 Comm: vhost-82 Not tainted 7.3.0-rc2+ #14 PREEMPT(full)
>> > [   34.701958] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
>> > [   34.702717] RIP: 0010:sbitmap_get+0x227/0x270
>> > [   34.703247] Code: 41 8d 5d 01 89 df 44 89 fe e8 f5 3a 46 ff 31 c0 44 39 fb 0f 43 d8 e8 38 46 46 ff 65 41 89 1c 24 e9 25 ff ff ff e8 29 46 46 ff <0f> 0b eb ba e8 20 46 46 ff 44 89 fe 31 ff 45 31 e4 e8 c3 3b 46 ff
>> > [   34.704234] RSP: 0018:ffffc90000ec3bb0 EFLAGS: 00000246
>> > [   34.704677] RAX: 0000000000000000 RBX: ffff88800500dee8 RCX: 0000000000000000
>> > [   34.705166] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff88800500dee8
>> > [   34.705584] RBP: ffffc90000ec3be0 R08: 0000000000000000 R09: 0000000000000000
>> > [   34.705988] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000006
>> > [   34.706477] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8880050094d0
>> > [   34.706955] FS:  0000000014d05380(0000) GS:ffff888099c36000(0000) knlGS:0000000000000000
>> > [   34.707463] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> > [   34.707854] CR2: 00007c0c3c20e000 CR3: 0000000008e47000 CR4: 00000000000006f0
>> > [   34.708592] Call Trace:
>> > [   34.709259]  <TASK>
>> > [   34.709822]  vhost_scsi_handle_vq+0x4a9/0x1b20
>> > [   34.710437]  ? update_cfs_rq_load_avg+0x2e/0x230
>> > [   34.710874]  vhost_scsi_handle_kick+0x2c/0x40
>> > [   34.711292]  vhost_run_work_list+0xa6/0xf0
>> > [   34.711673]  vhost_task_fn+0x76/0x1b0
>> > [   34.712003]  ? __sanitizer_cov_trace_const_cmp1+0x1a/0x30
>> > [   34.712506]  ? recalc_sigpending+0xe1/0x130
>> > [   34.712890]  ? __pfx_vhost_task_fn+0x10/0x10
>> > [   34.713325]  ret_from_fork+0x37a/0x630
>> > [   34.713673]  ? __pfx_vhost_task_fn+0x10/0x10
>> > [   34.714031]  ret_from_fork_asm+0x1a/0x30
>> > [   34.714535] RIP: 0033:0x0
>> > [   34.715201] Code: Unable to access opcode bytes at 0xffffffffffffffd6.
>> > [   34.715587] RSP: 002b:0000000000000000 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>> > [   34.716158] RAX: 0000000000000000 RBX: 000000000048bb20 RCX: 000000000041ca3d
>> > [   34.716571] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003
>> > [   34.716957] RBP: 00007ffcac47c4e0 R08: 00000000ffffffff R09: 0000000000000000
>> > [   34.717418] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
>> > [   34.717826] R13: 00007ffcac47c778 R14: 00000000004af868 R15: 0000000000000001
>> > [   34.718363]  </TASK>
>> > ---
>> >   drivers/vhost/scsi.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>> >   1 file changed, 42 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
>> > index 4f8c0260bc9e..069a6ff4dd40 100644
>> > --- a/drivers/vhost/scsi.c
>> > +++ b/drivers/vhost/scsi.c
>> > @@ -2008,6 +2008,40 @@ static int vhost_scsi_setup_vq_cmds(struct vhost_virtqueue *vq, int max_cmds)
>> >     return -ENOMEM;
>> >   }
>> >
>> > +/* Callers must hold dev mutex. */
>> > +static int vhost_scsi_activate_vqs(struct vhost_scsi *vs)
>> > +{
>> > +   struct vhost_virtqueue *vq;
>> > +   int i, ret = 0;
>> > +
>> > +   if (!vs->vs_tpg)
>> > +           return 0;
>> > +
>> > +   for (i = VHOST_SCSI_VQ_IO; i < vs->dev.nvqs; i++) {
>> > +           vq = &vs->vqs[i].vq;
>> > +           mutex_lock(&vq->mutex);
>> > +           if (vhost_vq_get_backend(vq) || !vhost_vq_is_setup(vq))
>> > +                   goto unlock;
>> > +
>> > +           ret = vhost_scsi_setup_vq_cmds(vq, vq->num);
>> > +           if (ret)
>> > +                   goto unlock;
>> > +
>> > +           vhost_vq_set_backend(vq, vs->vs_tpg);
>> > +           ret = vhost_vq_init_access(vq);
>> > +           if (ret) {
>> > +                   vhost_vq_set_backend(vq, NULL);
>> > +                   vhost_scsi_destroy_vq_cmds(vq);
>> > +           }
>> > +unlock:
>> > +           mutex_unlock(&vq->mutex);
>> > +           if (ret)
>> > +                   break;
>> > +   }
>> > +
>> > +   return ret;
>> > +}
>> > +
>> >   /*
>> >    * Called from vhost_scsi_ioctl() context to walk the list of available
>> >    * vhost_scsi_tpg with an active struct vhost_scsi_nexus
>> > @@ -2105,6 +2139,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
>> >
>> >             for (i = 0; i < vs->dev.nvqs; i++) {
>> >                     vq = &vs->vqs[i].vq;
>> > +                   if (i >= VHOST_SCSI_VQ_IO && !vs->vqs[i].scsi_cmds)
>> > +                           continue;
>> > +
>> >                     mutex_lock(&vq->mutex);>                        vhost_vq_set_backend(vq, vs_tpg);
>> >                     vhost_vq_init_access(vq);
>> > @@ -2466,10 +2503,13 @@ vhost_scsi_ioctl(struct file *f,
>> >     default:
>> >             mutex_lock(&vs->dev.mutex);
>> >             r = vhost_dev_ioctl(&vs->dev, ioctl, argp);
>> > -           if (r == -ENOIOCTLCMD)
>> > +           if (r == -ENOIOCTLCMD) {
>> >                     r = vhost_vring_ioctl(&vs->dev, ioctl, argp);
>> > -           else
>> > +                   if (!r && ioctl == VHOST_SET_VRING_ADDR)
>> > +                           r = vhost_scsi_activate_vqs(vs);
>> Is it ok to just not support the delayed setup? So just do the
>> fix in vhost_scsi_set_endpoint?
>>
> A later SET_VRING_ADDR would still succeed while leaving the queue
> inactive. If we donot support it, I think it should fail explicitly
> instead of causing a silent I/O stall.
> 
>> Is this a useful feature?
>>
> No current VMM appears to rely on it, but the UAPI does not specify this
> ordering. A new VMM could reasonably use it.

For the current code, I'm not sure what the use case would be. If are
are just saying we want to support the ioctls in any old order then
I don't think it's worth the trouble. I would just return a failure
like you mentioned above.

However, if you are saying we could do something like have a running
vhost-scsi device then dynamically add queues to the running device
to increase perf then that could be useful. I think you would want to
also fix the guest to be able to support remapping queues on the fly.
In vhost-scsi you then also probably want to make the vhost_virtqueue
allocation more dynamic as well. You would want to start with nvqs = 3,
add queues are more are created then limit it to vhost_scsi_max_io_vqs.



> 
>> I don't think net supports it. When it does the vhost_net_set_backend
>> call it does vhost_vq_access_ok. If it's not ready to go then it
>> returns failure.
>>
> Right, net returns an error for that VQ. Simply skipping a scsi VQ would
> make the later ioctl succeed without activating it.
>