Re: [PATCH] virtio_pci: Fix admin vq cleanup by using correct info pointer

[Feng Liu <feliu@xxxxxxxxxx>]

Hi,

Any comments?

Thanks
Feng

On 2024-10-24 a.m.9:54, Feng Liu wrote:
> 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@xxxxxxxxxx>
> Signed-off-by: Jiri Pirko <jiri@xxxxxxxxxx>
> Reviewed-by: Parav Pandit <parav@xxxxxxxxxx>
> ---
>   drivers/virtio/virtio_pci_common.c | 24 ++++++++++++++++++------
>   drivers/virtio/virtio_pci_common.h |  1 +
>   drivers/virtio/virtio_pci_modern.c | 12 +-----------
>   3 files changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index c44d8ba00c02..88074451dd61 100644
> --- 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 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int in
>   	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 1d9c49947f52..8beecf23ec85 100644
> --- 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 9193c30d640a..4fbcbc7a9ae1 100644
> --- 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;
>