A userspace triggerable infinite loop could happen in
mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
cvq requests.
Fixing this by introducing a quota and re-queue the work if we're out
of the budget (currently the implicit budget is one) . While at it,
using a per device work struct to avoid on demand memory allocation
for cvq.
Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <[email protected]>
---
Changes since V1:
- Using 1 as the budget
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index d0f91078600e..b2afd2b6fbca 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
u32 cur_num_vqs;
struct notifier_block nb;
struct vdpa_callback config_cb;
+ struct mlx5_vdpa_wq_ent cvq_ent;
};
static void free_resources(struct mlx5_vdpa_net *ndev);
@@ -1616,10 +1617,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
ndev = to_mlx5_vdpa_ndev(mvdev);
cvq = &mvdev->cvq;
if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
- goto out;
+ return;
if (!cvq->ready)
- goto out;
+ return;
while (true) {
err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1653,9 +1654,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
if (vringh_need_notify_iotlb(&cvq->vring))
vringh_notify(&cvq->vring);
+
+ queue_work(mvdev->wq, &wqent->work);
+ break;
}
-out:
- kfree(wqent);
}
static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -1663,7 +1665,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
struct mlx5_vdpa_virtqueue *mvq;
- struct mlx5_vdpa_wq_ent *wqent;
if (!is_index_valid(mvdev, idx))
return;
@@ -1672,13 +1673,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
if (!mvdev->cvq.ready)
return;
- wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
- if (!wqent)
- return;
-
- wqent->mvdev = mvdev;
- INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
- queue_work(mvdev->wq, &wqent->work);
+ queue_work(mvdev->wq, &ndev->cvq_ent.work);
return;
}
@@ -2668,6 +2663,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
if (err)
goto err_mr;
+ ndev->cvq_ent.mvdev = mvdev;
+ INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
if (!mvdev->wq) {
err = -ENOMEM;
--
2.18.1
Currently, CVQ doesn't have any synchronization with the driver
status. Then CVQ emulation code run in the middle of:
1) device reset
2) device status changed
3) map updating
The will lead several unexpected issue like trying to execute CVQ
command after the driver has been teared down.
Fixing this by using reslock to synchronize CVQ emulation code with
the driver status changing:
- protect the whole device reset, status changing and set_map()
updating with reslock
- protect the CVQ handler with the reslock and check
VIRTIO_CONFIG_S_DRIVER_OK in the CVQ handler
This will guarantee that:
1) CVQ handler won't work if VIRTIO_CONFIG_S_DRIVER_OK is not set
2) CVQ handler will see a consistent state of the driver instead of
the partial one when it is running in the middle of the
teardown_driver() or setup_driver().
Cc: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
Signed-off-by: Jason Wang <[email protected]>
---
Changes since V1:
- document the lock requirement
- protect the whole .set_map()
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
1 file changed, 37 insertions(+), 14 deletions(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b2afd2b6fbca..53b8c1a68f90 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1616,11 +1616,17 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
mvdev = wqent->mvdev;
ndev = to_mlx5_vdpa_ndev(mvdev);
cvq = &mvdev->cvq;
+
+ mutex_lock(&ndev->reslock);
+
+ if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
- return;
+ goto out;
if (!cvq->ready)
- return;
+ goto out;
while (true) {
err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
@@ -1658,6 +1664,9 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
queue_work(mvdev->wq, &wqent->work);
break;
}
+
+out:
+ mutex_unlock(&ndev->reslock);
}
static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
@@ -2132,7 +2141,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
goto err_mr;
if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
- return 0;
+ goto err_mr;
restore_channels_info(ndev);
err = setup_driver(mvdev);
@@ -2147,12 +2156,14 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev, struct vhost_iotlb
return err;
}
+/* reslock must be held for this function */
static int setup_driver(struct mlx5_vdpa_dev *mvdev)
{
struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
int err;
- mutex_lock(&ndev->reslock);
+ WARN_ON(!mutex_is_locked(&ndev->reslock));
+
if (ndev->setup) {
mlx5_vdpa_warn(mvdev, "setup driver called for already setup driver\n");
err = 0;
@@ -2182,7 +2193,6 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
goto err_fwd;
}
ndev->setup = true;
- mutex_unlock(&ndev->reslock);
return 0;
@@ -2193,23 +2203,23 @@ static int setup_driver(struct mlx5_vdpa_dev *mvdev)
err_rqt:
teardown_virtqueues(ndev);
out:
- mutex_unlock(&ndev->reslock);
return err;
}
+/* reslock must be held for this function */
static void teardown_driver(struct mlx5_vdpa_net *ndev)
{
- mutex_lock(&ndev->reslock);
+
+ WARN_ON(!mutex_is_locked(&ndev->reslock));
+
if (!ndev->setup)
- goto out;
+ return;
remove_fwd_to_tir(ndev);
destroy_tir(ndev);
destroy_rqt(ndev);
teardown_virtqueues(ndev);
ndev->setup = false;
-out:
- mutex_unlock(&ndev->reslock);
}
static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
@@ -2230,6 +2240,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
print_status(mvdev, status, true);
+ mutex_lock(&ndev->reslock);
+
if ((status ^ ndev->mvdev.status) & VIRTIO_CONFIG_S_DRIVER_OK) {
if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
err = setup_driver(mvdev);
@@ -2239,16 +2251,19 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
}
} else {
mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
- return;
+ goto err_clear;
}
}
ndev->mvdev.status = status;
+ mutex_unlock(&ndev->reslock);
return;
err_setup:
mlx5_vdpa_destroy_mr(&ndev->mvdev);
ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
+err_clear:
+ mutex_unlock(&ndev->reslock);
}
static int mlx5_vdpa_reset(struct vdpa_device *vdev)
@@ -2258,6 +2273,8 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
print_status(mvdev, 0, true);
mlx5_vdpa_info(mvdev, "performing device reset\n");
+
+ mutex_lock(&ndev->reslock);
teardown_driver(ndev);
clear_vqs_ready(ndev);
mlx5_vdpa_destroy_mr(&ndev->mvdev);
@@ -2270,6 +2287,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
if (mlx5_vdpa_create_mr(mvdev, NULL))
mlx5_vdpa_warn(mvdev, "create MR failed\n");
}
+ mutex_unlock(&ndev->reslock);
return 0;
}
@@ -2305,19 +2323,24 @@ static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
static int mlx5_vdpa_set_map(struct vdpa_device *vdev, struct vhost_iotlb *iotlb)
{
struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
+ struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
bool change_map;
int err;
+ mutex_lock(&ndev->reslock);
+
err = mlx5_vdpa_handle_set_map(mvdev, iotlb, &change_map);
if (err) {
mlx5_vdpa_warn(mvdev, "set map failed(%d)\n", err);
- return err;
+ goto err;
}
if (change_map)
- return mlx5_vdpa_change_map(mvdev, iotlb);
+ err = mlx5_vdpa_change_map(mvdev, iotlb);
- return 0;
+err:
+ mutex_unlock(&ndev->reslock);
+ return err;
}
static void mlx5_vdpa_free(struct vdpa_device *vdev)
--
2.18.1
> From: Jason Wang <[email protected]>
> Sent: Tuesday, March 29, 2022 7:21 AM
> To: [email protected]; [email protected]
> Cc: Eli Cohen <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: [PATCH RESEND V2 1/3] vdpa: mlx5: prevent cvq work from hogging CPU
>
> A userspace triggerable infinite loop could happen in
> mlx5_cvq_kick_handler() if userspace keeps sending a huge amount of
> cvq requests.
>
> Fixing this by introducing a quota and re-queue the work if we're out
> of the budget (currently the implicit budget is one) . While at it,
> using a per device work struct to avoid on demand memory allocation
> for cvq.
>
> Fixes: 5262912ef3cfc ("vdpa/mlx5: Add support for control VQ and MAC setting")
> Signed-off-by: Jason Wang <[email protected]>
Acked-by: Eli Cohen <[email protected]>
> ---
> Changes since V1:
> - Using 1 as the budget
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 21 +++++++++------------
> 1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index d0f91078600e..b2afd2b6fbca 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -163,6 +163,7 @@ struct mlx5_vdpa_net {
> u32 cur_num_vqs;
> struct notifier_block nb;
> struct vdpa_callback config_cb;
> + struct mlx5_vdpa_wq_ent cvq_ent;
> };
>
> static void free_resources(struct mlx5_vdpa_net *ndev);
> @@ -1616,10 +1617,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
> ndev = to_mlx5_vdpa_ndev(mvdev);
> cvq = &mvdev->cvq;
> if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ)))
> - goto out;
> + return;
>
> if (!cvq->ready)
> - goto out;
> + return;
>
> while (true) {
> err = vringh_getdesc_iotlb(&cvq->vring, &cvq->riov, &cvq->wiov, &cvq->head,
> @@ -1653,9 +1654,10 @@ static void mlx5_cvq_kick_handler(struct work_struct *work)
>
> if (vringh_need_notify_iotlb(&cvq->vring))
> vringh_notify(&cvq->vring);
> +
> + queue_work(mvdev->wq, &wqent->work);
> + break;
> }
> -out:
> - kfree(wqent);
> }
>
> static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> @@ -1663,7 +1665,6 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> struct mlx5_vdpa_virtqueue *mvq;
> - struct mlx5_vdpa_wq_ent *wqent;
>
> if (!is_index_valid(mvdev, idx))
> return;
> @@ -1672,13 +1673,7 @@ static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
> if (!mvdev->cvq.ready)
> return;
>
> - wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> - if (!wqent)
> - return;
> -
> - wqent->mvdev = mvdev;
> - INIT_WORK(&wqent->work, mlx5_cvq_kick_handler);
> - queue_work(mvdev->wq, &wqent->work);
> + queue_work(mvdev->wq, &ndev->cvq_ent.work);
> return;
> }
>
> @@ -2668,6 +2663,8 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> if (err)
> goto err_mr;
>
> + ndev->cvq_ent.mvdev = mvdev;
> + INIT_WORK(&ndev->cvq_ent.work, mlx5_cvq_kick_handler);
> mvdev->wq = create_singlethread_workqueue("mlx5_vdpa_wq");
> if (!mvdev->wq) {
> err = -ENOMEM;
> --
> 2.18.1