virtio_pci: Fix admin vq cleanup by using correct info pointer

vp_modern_avq_cleanup() and vp_del_vqs() clean up admin vq
resources by virtio_pci_vq_info pointer. The info pointer of admin
vq is stored in vp_dev->admin_vq.info instead of vp_dev->vqs[].
Using the info pointer from vp_dev->vqs[] for admin vq causes a
kernel NULL pointer dereference bug.
In vp_modern_avq_cleanup() and vp_del_vqs(), get the info pointer
from vp_dev->admin_vq.info for admin vq to clean up the resources.
Also make info ptr as argument of vp_del_vq() to be symmetric with
vp_setup_vq().

vp_reset calls vp_modern_avq_cleanup, and causes the Call Trace:
==================================================================
BUG: kernel NULL pointer dereference, address:0000000000000000
...
CPU: 49 UID: 0 PID: 4439 Comm: modprobe Not tainted 6.11.0-rc5 #1
RIP: 0010:vp_reset+0x57/0x90 [virtio_pci]
Call Trace:
 <TASK>
...
 ? vp_reset+0x57/0x90 [virtio_pci]
 ? vp_reset+0x38/0x90 [virtio_pci]
 virtio_reset_device+0x1d/0x30
 remove_vq_common+0x1c/0x1a0 [virtio_net]
 virtnet_remove+0xa1/0xc0 [virtio_net]
 virtio_dev_remove+0x46/0xa0
...
 virtio_pci_driver_exit+0x14/0x810 [virtio_pci]
==================================================================

Fixes: 4c3b54af907e ("virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result")
Signed-off-by: Feng Liu <feliu@nvidia.com>
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Message-Id: <20241024135406.81388-1-feliu@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index c44d8ba00c02c38f9d641b8ccf423efbf86ea83b..88074451dd6151f03cfa384ff0893852f302eda4 100644 (file)
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -24,6 +24,16 @@ MODULE_PARM_DESC(force_legacy,
                 "Force legacy mode for transitional virtio 1 devices");
 #endif
 
+bool vp_is_avq(struct virtio_device *vdev, unsigned int index)
+{
+       struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+
+       if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
+               return false;
+
+       return index == vp_dev->admin_vq.vq_index;
+}
+
 /* wait for pending irq handlers */
 void vp_synchronize_vectors(struct virtio_device *vdev)
 {
@@ -234,10 +244,9 @@ out_info:
        return vq;
 }
 
-static void vp_del_vq(struct virtqueue *vq)
+static void vp_del_vq(struct virtqueue *vq, struct virtio_pci_vq_info *info)
 {
        struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-       struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
        unsigned long flags;
 
        /*
@@ -258,13 +267,16 @@ static void vp_del_vq(struct virtqueue *vq)
 void vp_del_vqs(struct virtio_device *vdev)
 {
        struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+       struct virtio_pci_vq_info *info;
        struct virtqueue *vq, *n;
        int i;
 
        list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-               if (vp_dev->per_vq_vectors) {
-                       int v = vp_dev->vqs[vq->index]->msix_vector;
+               info = vp_is_avq(vdev, vq->index) ? vp_dev->admin_vq.info :
+                                                   vp_dev->vqs[vq->index];
 
+               if (vp_dev->per_vq_vectors) {
+                       int v = info->msix_vector;
                        if (v != VIRTIO_MSI_NO_VECTOR &&
                            !vp_is_slow_path_vector(v)) {
                                int irq = pci_irq_vector(vp_dev->pci_dev, v);
@@ -273,7 +285,7 @@ void vp_del_vqs(struct virtio_device *vdev)
                                free_irq(irq, vq);
                        }
                }
-               vp_del_vq(vq);
+               vp_del_vq(vq, info);
        }
        vp_dev->per_vq_vectors = false;
 
@@ -354,7 +366,7 @@ vp_find_one_vq_msix(struct virtio_device *vdev, int queue_idx,
                          vring_interrupt, 0,
                          vp_dev->msix_names[msix_vec], vq);
        if (err) {
-               vp_del_vq(vq);
+               vp_del_vq(vq, *p_info);
                return ERR_PTR(err);
        }
 

diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 1d9c49947f52d1b6e46057bbf9f0f71216f307b5..8beecf23ec85e05e66f99afae0be6073df473f24 100644 (file)
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -178,6 +178,7 @@ struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
 #define VIRTIO_ADMIN_CMD_BITMAP 0
 #endif
 
+bool vp_is_avq(struct virtio_device *vdev, unsigned int index);
 void vp_modern_avq_done(struct virtqueue *vq);
 int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
                             struct virtio_admin_cmd *cmd);

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 9193c30d640aeb91b6bcd6bccc97bedda2418239..4fbcbc7a9ae1ccd6efa6093310d698389372ac23 100644 (file)
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -43,16 +43,6 @@ static int vp_avq_index(struct virtio_device *vdev, u16 *index, u16 *num)
        return 0;
 }
 
-static bool vp_is_avq(struct virtio_device *vdev, unsigned int index)
-{
-       struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-
-       if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
-               return false;
-
-       return index == vp_dev->admin_vq.vq_index;
-}
-
 void vp_modern_avq_done(struct virtqueue *vq)
 {
        struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
@@ -245,7 +235,7 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
        if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
                return;
 
-       vq = vp_dev->vqs[vp_dev->admin_vq.vq_index]->vq;
+       vq = vp_dev->admin_vq.info->vq;
        if (!vq)
                return;