Re: [PATCH rdma-next] RDMA/mlx5: Fix implicit ODP use after free

[Jason Gunthorpe <jgg@xxxxxxxxxx>]

On Tue, Jan 07, 2025 at 02:12:53PM +0200, Leon Romanovsky wrote:
> From: Patrisious Haddad <phaddad@xxxxxxxxxx>
> 
> Prevent double queueing of implicit ODP mr destroy work by adding a bit
> to the MR indicating if this MR is already queued for destruction.

I do not like adding random state bits to structs, you don't have any
locking protecting these bits, it is probably not safe.

We already have the xarray with its own locking and tracking, just use it:

diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
index 4eb03fc0d30228..858a35a335dff6 100644
--- a/drivers/infiniband/hw/mlx5/odp.c
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -228,13 +228,20 @@ static void destroy_unused_implicit_child_mr(struct mlx5_ib_mr *mr)
 	unsigned long idx = ib_umem_start(odp) >> MLX5_IMR_MTT_SHIFT;
 	struct mlx5_ib_mr *imr = mr->parent;
 
-	if (!refcount_inc_not_zero(&imr->mmkey.usecount))
+	xa_lock(&imr->implicit_children);
+	if (__xa_cmpxchg(&imr->implicit_children, idx, mr, NULL, GFP_KERNEL) !=
+	    mr) {
+		xa_unlock(&imr->implicit_children);
 		return;
+	}
 
-	xa_erase(&imr->implicit_children, idx);
 	if (MLX5_CAP_ODP(mr_to_mdev(mr)->mdev, mem_page_fault))
 		xa_erase(&mr_to_mdev(mr)->odp_mkeys,
 			 mlx5_base_mkey(mr->mmkey.key));
+	xa_unlock(&imr->implicit_children);
+
+	if (!refcount_inc_not_zero(&imr->mmkey.usecount))
+		return;
 
 	/* Freeing a MR is a sleeping operation, so bounce to a work queue */
 	INIT_WORK(&mr->odp_destroy.work, free_implicit_child_mr_work);
@@ -500,18 +507,18 @@ static struct mlx5_ib_mr *implicit_get_child_mr(struct mlx5_ib_mr *imr,
 		refcount_inc(&ret->mmkey.usecount);
 		goto out_lock;
 	}
-	xa_unlock(&imr->implicit_children);
 
 	if (MLX5_CAP_ODP(dev->mdev, mem_page_fault)) {
 		ret = xa_store(&dev->odp_mkeys, mlx5_base_mkey(mr->mmkey.key),
 			       &mr->mmkey, GFP_KERNEL);
 		if (xa_is_err(ret)) {
 			ret = ERR_PTR(xa_err(ret));
-			xa_erase(&imr->implicit_children, idx);
-			goto out_mr;
+			__xa_erase(&imr->implicit_children, idx);
+			goto out_lock;
 		}
 		mr->mmkey.type = MLX5_MKEY_IMPLICIT_CHILD;
 	}
+	xa_unlock(&imr->implicit_children);
 	mlx5_ib_dbg(mr_to_mdev(imr), "key %x mr %p\n", mr->mmkey.key, mr);
 	return mr;