linux-kernel - Re: [PATCH] vdpa/mlx5: should not activate virtq object when suspended

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <536e8152-b575-7b8f-d7c9-37975bbedd3d@nvidia.com>
Date:   Sun, 12 Feb 2023 08:55:38 +0200
From:   Eli Cohen <elic@...dia.com>
To:     Si-Wei Liu <si-wei.liu@...cle.com>, mst@...hat.com,
        jasowang@...hat.com
Cc:     parav@...dia.com, virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vdpa/mlx5: should not activate virtq object when
 suspended


On 11/02/2023 8:08, Si-Wei Liu wrote:
> Otherwise the virtqueue object to instate could point to invalid address
> that was unmapped from the MTT:
>
>    mlx5_core 0000:41:04.2: mlx5_cmd_out_err:782:(pid 8321):
>    CREATE_GENERAL_OBJECT(0xa00) op_mod(0xd) failed, status
>    bad parameter(0x3), syndrome (0x5fa1c), err(-22)
I am familiar with this issue but I think it should be handled at the 
qemu level. This logic just hides the real problem. qemu provides the 
iova and the VQs' addresses so it should not shrink the map while such 
that VQ addresses are out of the iova range.
>
> While at it, add warning message to tell apart which object is
> responsible for the CREATE_GENERAL_OBJECT command failure.
>
> Fixes: cae15c2ed8e6 ("vdpa/mlx5: Implement susupend virtqueue callback")
> Cc: Eli Cohen <elic@...dia.com>
> Signed-off-by: Si-Wei Liu <si-wei.liu@...cle.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 3a6dbbc6..c05c7f6 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -165,6 +165,7 @@ struct mlx5_vdpa_net {
>   	u32 cur_num_vqs;
>   	u32 rqt_size;
>   	bool nb_registered;
> +	bool suspended;
>   	struct notifier_block nb;
>   	struct vdpa_callback config_cb;
>   	struct mlx5_vdpa_wq_ent cvq_ent;
> @@ -1245,12 +1246,18 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
>   		goto err_connect;
>   
>   	err = counter_set_alloc(ndev, mvq);
> -	if (err)
> +	if (err) {
> +		mlx5_vdpa_warn(&ndev->mvdev, "failed to alloc counter on vq idx %d(%d)\n",
> +			       idx, err);
Although you mention in the commit log, maybe it's better to have a 
separate patch for improving warning messages.
>   		goto err_counter;
> +	}
>   
>   	err = create_virtqueue(ndev, mvq);
> -	if (err)
> +	if (err) {
> +		mlx5_vdpa_warn(&ndev->mvdev, "failed to create virtqueue idx %d(%d)\n",
> +			       idx, err);
>   		goto err_connect;
> +	}
>   
>   	if (mvq->ready) {
>   		err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> @@ -2411,7 +2418,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
>   	if (err)
>   		goto err_mr;
>   
> -	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> +	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || ndev->suspended)
>   		goto err_mr;
>   
>   	restore_channels_info(ndev);
> @@ -2580,6 +2587,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
>   	mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   	ndev->mvdev.status = 0;
>   	ndev->cur_num_vqs = 0;
> +	ndev->suspended = false;
>   	ndev->mvdev.cvq.received_desc = 0;
>   	ndev->mvdev.cvq.completed_desc = 0;
>   	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
> @@ -2815,6 +2823,8 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
>   	struct mlx5_vdpa_virtqueue *mvq;
>   	int i;
>   
> +	mlx5_vdpa_info(mvdev, "suspending device\n");
> +
>   	down_write(&ndev->reslock);
>   	ndev->nb_registered = false;
>   	mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> @@ -2824,6 +2834,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
>   		suspend_vq(ndev, mvq);
>   	}
>   	mlx5_vdpa_cvq_suspend(mvdev);
> +	ndev->suspended = true;
>   	up_write(&ndev->reslock);
>   	return 0;
>   }