Re: Should there be a mode in which the virtqueue -> MSI mapping is fixed?

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4835 bytes --]

On 4/5/26 17:09, Michael S. Tsirkin wrote:
> On Sun, Apr 05, 2026 at 04:58:39PM -0400, Demi Marie Obenour wrote:
>> On 4/5/26 16:15, Michael S. Tsirkin wrote:
>>> On Sun, Apr 05, 2026 at 01:50:25PM -0400, Demi Marie Obenour wrote:
>>>> On 4/4/26 20:56, Michael S. Tsirkin wrote:
>>>>> On Sat, Apr 04, 2026 at 05:19:41PM -0400, Demi Marie Obenour wrote:
>>>>>> Cloud Hypervisor's vhost-user frontend does not implement MSI-X
>>>>>> properly [1].  Specifically:
>>>>>>
>>>>>> 1. Reads from the Pending Bit Array (PBA) always return 0.
>>>>>> 2. Changes to the MSI associated with a virtqueue after the device
>>>>>>    is activated are ignored.
>>>>>>    
>>>>>> Amazingly, there have not been any reports of this causing breakage.
>>>>>> I have a fix for the first [2], which actually decreases the amount
>>>>>> of code.  However, the second is trickier and I'm tempted to not
>>>>>> bother unless it causes real-world problems.
>>>>>>
>>>>>> Are there real-world drivers that will run into either of the above
>>>>>> bugs?  Linux seems to only choose anything else as a fallback, which
>>>>>> presumably is not triggered.
>>>>>>
>>>>>> [1]: https://github.com/cloud-hypervisor/cloud-hypervisor/issues/7813
>>>>>> [2]: https://github.com/cloud-hypervisor/cloud-hypervisor/pull/7963
>>>>>
>>>>> It will sometimes trigger.
>>>>
>>>> Would it be possible to provide an example?  A reproducible test
>>>> case would be ideal, but conditions under which this will trigger
>>>> are also sufficient.
>>>
>>>
>>> I am not sure what does "is activated" mean.
>>> For example, on latest Linux:
>>>
>>>         vq = vp_find_one_vq_msix(vdev, avq->vq_index, vp_modern_avq_done,
>>>                                  avq->name, false, true, &allocated_vectors,
>>>                                  vector_policy, &vp_dev->admin_vq.info);
>>>         if (IS_ERR(vq)) {
>>>                 err = PTR_ERR(vq);
>>>                 goto error_find;
>>>         }
>>>
>>>         return 0;
>>>
>>> error_find:
>>>         vp_del_vqs(vdev);
>>>         return err;
>>> }
>>>
>>>
>>> And 
>>>
>>> static void del_vq(struct virtio_pci_vq_info *info)
>>> {
>>>         struct virtqueue *vq = info->vq;
>>>         struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
>>>         struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
>>>
>>>         if (vp_dev->msix_enabled)
>>>                 vp_modern_queue_vector(mdev, vq->index,
>>>                                        VIRTIO_MSI_NO_VECTOR);
>>>
>>>         if (!mdev->notify_base)
>>>                 pci_iounmap(mdev->pci_dev, (void __force __iomem *)vq->priv);
>>>
>>>         vring_del_virtqueue(vq);
>>> }
>>>
>>>
>>> and this happens after feature negotiation.
>>>
>>> It's before device_ready, however.
>>
>> Are there drivers that will change virtqueue => MSI-X vector mappings
>> after DRIVER_OK without an intervening reset?  Cloud Hypervisor
>> supports this for devices it implements internally, but it ignores
>> such changes for vhost-user devices.  Is this going to cause problems
>> in practice?
> 
> 
> Also yes.
> 
> For example, dpdk uses this during cleanup to block interrupts:
> drivers/net/virtio/virtio_ethdev.c
> 
> 
> int
> virtio_dev_close(struct rte_eth_dev *dev)
> {       
>         struct virtio_hw *hw = dev->data->dev_private;
>         struct rte_eth_intr_conf *intr_conf = &dev->data->dev_conf.intr_conf;
>                                              
>         PMD_INIT_LOG(DEBUG, "virtio_dev_close");
>         if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>                 return 0;
> 
>         if (!hw->opened)
>                 return 0;
>         hw->opened = 0;
> 
>         /* reset the NIC */
>         if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>                 VIRTIO_OPS(hw)->set_config_irq(hw, VIRTIO_MSI_NO_VECTOR);
>         if (intr_conf->rxq)
>                 virtio_queues_unbind_intr(dev);
> 
>         if (intr_conf->lsc || intr_conf->rxq) {
>                 virtio_intr_disable(dev);
>                 rte_intr_efd_disable(dev->intr_handle);
>                 rte_intr_vec_list_free(dev->intr_handle);
>         }
> 
>         virtio_reset(hw);
>         virtio_dev_free_mbufs(dev);
>         virtio_free_queues(hw);
>         virtio_free_rss(hw);
> 
>         return VIRTIO_OPS(hw)->dev_close(hw);
> }

What would happen if DPDK received a spurious MSI-X interrupt during
this time?

I know that this is non-compliant with the virtio specification.
However, a problem that will trigger in practice, or which can be
used for an exploit, is far more severe (and thus far more important
to fix) than one that does not have practical consequences.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]