[PATCH] vdpa/mlx5: fix MR state corruption on setup_vq_resources failure

[PATCH] vdpa/mlx5: fix MR state corruption on setup_vq_resources failure

[Weimin Xiong]

From: xiongweimin <xiongweimin@kylinos.cn>

In mlx5_vdpa_change_map(), mlx5_vdpa_update_mr() is called before
setup_vq_resources(), which releases the old MR and updates
mres.mr[asid] to point to new_mr. If setup_vq_resources() then
fails, the function returns an error but the MR pointer is already
updated, leaving mres.mr[asid] pointing to a potentially invalid
state.

Fix by calling setup_vq_resources() before updating the MR pointer.
On failure, the old MR is still intact and can be restored by the
caller.

Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 24 ++++++++++++++++----------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index ad0d5fbbb..8a6ddc00b 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -3067,24 +3067,28 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
 		teardown_vq_resources(ndev);
 	}
 
-	mlx5_vdpa_update_mr(mvdev, new_mr, asid);
-
 	for (int i = 0; i < mvdev->max_vqs; i++)
 		ndev->vqs[i].modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_MKEY |
 						MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY;
 
 	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || mvdev->suspended)
-		return 0;
+		goto done;
 
 	if (teardown) {
 		restore_channels_info(ndev);
 		err = setup_vq_resources(ndev, true);
 		if (err)
-			return err;
+			goto out_err;
 	}
 
 	resume_vqs(ndev, 0, ndev->cur_num_vqs);
+	goto done;
 
+out_err:
+	return err;
+
+done:
+	mlx5_vdpa_update_mr(mvdev, new_mr, asid);
 	return 0;
 }
 
--
2.39.3

Re: [PATCH] vdpa/mlx5: fix MR state corruption on setup_vq_resources failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Moving the MR update to the end of mlx5_vdpa_change_map() causes hardware virtqueues to be programmed with the old Memory Region (MR) keys instead of the new ones, leading to device malfunction and hardware faults.
--

--- Patch [1]: [PATCH] vdpa/mlx5: fix MR state corruption on setup_vq_resources failure ---
commit 0e93d2962a61c5461c3028e043cb48591fa19ebe
Author: xiongweimin <xiongweimin@kylinos.cn>

vdpa/mlx5: fix MR state corruption on setup_vq_resources failure

This commit defers mlx5_vdpa_update_mr() until after setup_vq_resources()
and resume_vqs() to prevent MR state corruption if resource setup fails.

> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index ad0d5fbbbca84..8a6ddc00b8e07 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3067,24 +3067,28 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
>  		teardown_vq_resources(ndev);
>  	}
>  
> -	mlx5_vdpa_update_mr(mvdev, new_mr, asid);
> -
>  	for (int i = 0; i < mvdev->max_vqs; i++)
>  		ndev->vqs[i].modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_MKEY |
>  						MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY;
>  
>  	if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || mvdev->suspended)
> -		return 0;
> +		goto done;
>  
>  	if (teardown) {
>  		restore_channels_info(ndev);
>  		err = setup_vq_resources(ndev, true);
>  		if (err)
> -			return err;
> +			goto out_err;

[Severity: High]
Does deferring the MR update cause the hardware virtqueues to be programmed
with the old MR keys?

setup_vq_resources() eventually calls create_virtqueue(), which reads the MR
keys from the mvdev->mres.mr array before it has been updated:

drivers/vdpa/mlx5/net/mlx5_vnet.c:create_virtqueue() {
    ...
    vq_mr = mvdev->mres.mr[mvdev->mres.group2asid[MLX5_VDPA_DATAVQ_GROUP]];
    if (vq_mr)
        MLX5_SET(virtio_q, vq_ctx, virtio_q_mkey, vq_mr->mkey);
    ...
}

>  	}
>  
>  	resume_vqs(ndev, 0, ndev->cur_num_vqs);

Similarly, resume_vqs() modifies virtqueues using the stale MR keys:

drivers/vdpa/mlx5/net/mlx5_vnet.c:fill_modify_virtqueue_cmd() {
    ...
    vq_mr = mvdev->mres.mr[mvdev->mres.group2asid[MLX5_VDPA_DATAVQ_GROUP]];
    if (vq_mr)
        MLX5_SET(virtio_q, vq_ctx, virtio_q_mkey, vq_mr->mkey);
    ...
}

> +	goto done;
>  
> +out_err:
> +	return err;
> +
> +done:
> +	mlx5_vdpa_update_mr(mvdev, new_mr, asid);

Since mlx5_vdpa_update_mr() happens here at the end, does it destroy the old MR
while the hardware is actively configured to use it?

Could this lead to IOMMU faults and memory corruption when the memory
map changes?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716030241.124412-1-xiongwm2026@163.com?part=1