linux-rdma.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH rdma-rc 0/3] Fixes to coming PR
@ 2020-10-12  4:55 Leon Romanovsky
  2020-10-12  4:55 ` [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close Leon Romanovsky
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-12  4:55 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe
  Cc: Leon Romanovsky, linux-kernel, linux-rdma, Maor Gottlieb

From: Leon Romanovsky <leonro@nvidia.com>

Leon Romanovsky (2):
  RDMA/core: Postpone uobject cleanup on failure till FD close
  RDMA/core: Make FD destroy callback void

Maor Gottlieb (1):
  RDMA/ucma: Fix use after free in destroy id flow

 drivers/infiniband/core/rdma_core.c           | 45 ++++++++-----------
 drivers/infiniband/core/ucma.c                | 11 ++---
 drivers/infiniband/core/uverbs_cmd.c          |  5 +--
 drivers/infiniband/core/uverbs_std_types.c    | 18 +++-----
 .../core/uverbs_std_types_async_fd.c          |  5 +--
 .../core/uverbs_std_types_counters.c          |  5 +--
 drivers/infiniband/core/uverbs_std_types_cq.c |  4 +-
 drivers/infiniband/core/uverbs_std_types_dm.c |  6 +--
 .../core/uverbs_std_types_flow_action.c       |  6 +--
 drivers/infiniband/core/uverbs_std_types_qp.c |  4 +-
 .../infiniband/core/uverbs_std_types_srq.c    |  4 +-
 drivers/infiniband/core/uverbs_std_types_wq.c |  4 +-
 drivers/infiniband/hw/mlx5/devx.c             | 14 +++---
 drivers/infiniband/hw/mlx5/fs.c               |  6 +--
 include/rdma/ib_verbs.h                       | 42 -----------------
 include/rdma/uverbs_types.h                   |  4 +-
 16 files changed, 59 insertions(+), 124 deletions(-)

--
2.26.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close
  2020-10-12  4:55 [PATCH rdma-rc 0/3] Fixes to coming PR Leon Romanovsky
@ 2020-10-12  4:55 ` Leon Romanovsky
  2020-10-27 16:55   ` Jason Gunthorpe
  2020-10-12  4:55 ` [PATCH rdma-rc 2/3] RDMA/core: Make FD destroy callback void Leon Romanovsky
  2020-10-12  4:56 ` [PATCH rdma-rc 3/3] RDMA/ucma: Fix use after free in destroy id flow Leon Romanovsky
  2 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-12  4:55 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: Leon Romanovsky, linux-rdma

From: Leon Romanovsky <leonro@nvidia.com>

The drivers were updated to allow return their destroy status,
while the failure during object cleanup is allowed as long as the
driver can guarantee to clean them during FD close().

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/rdma_core.c           | 46 ++++++++-----------
 drivers/infiniband/core/uverbs_cmd.c          |  5 +-
 drivers/infiniband/core/uverbs_std_types.c    | 15 +++---
 .../core/uverbs_std_types_counters.c          |  5 +-
 drivers/infiniband/core/uverbs_std_types_cq.c |  4 +-
 drivers/infiniband/core/uverbs_std_types_dm.c |  6 +--
 .../core/uverbs_std_types_flow_action.c       |  6 +--
 drivers/infiniband/core/uverbs_std_types_qp.c |  4 +-
 .../infiniband/core/uverbs_std_types_srq.c    |  4 +-
 drivers/infiniband/core/uverbs_std_types_wq.c |  4 +-
 drivers/infiniband/hw/mlx5/devx.c             |  4 +-
 drivers/infiniband/hw/mlx5/fs.c               |  6 +--
 include/rdma/ib_verbs.h                       | 42 -----------------
 13 files changed, 44 insertions(+), 107 deletions(-)

diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 5d41785f507b..9d5f2faae181 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -137,15 +137,9 @@ static int uverbs_destroy_uobject(struct ib_uobject *uobj,
 	} else if (uobj->object) {
 		ret = uobj->uapi_object->type_class->destroy_hw(uobj, reason,
 								attrs);
-		if (ret) {
-			if (ib_is_destroy_retryable(ret, reason, uobj))
-				return ret;
-
-			/* Nothing to be done, dangle the memory and move on */
-			WARN(true,
-			     "ib_uverbs: failed to remove uobject id %d, driver err=%d",
-			     uobj->id, ret);
-		}
+		if (ret)
+			/* Nothing to be done, wait till ucontext will clean it */
+			return ret;

 		uobj->object = NULL;
 	}
@@ -543,17 +537,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
 			     struct uverbs_obj_idr_type, type);
 	int ret = idr_type->destroy_object(uobj, why, attrs);

-	/*
-	 * We can only fail gracefully if the user requested to destroy the
-	 * object or when a retry may be called upon an error.
-	 * In the rest of the cases, just remove whatever you can.
-	 */
-	if (ib_is_destroy_retryable(ret, why, uobj))
+	if (ret)
 		return ret;

-	if (why == RDMA_REMOVE_ABORT)
-		return 0;
-
 	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
 			   RDMACG_RESOURCE_HCA_OBJECT);

@@ -581,12 +567,8 @@ static int __must_check destroy_hw_fd_uobject(struct ib_uobject *uobj,
 {
 	const struct uverbs_obj_fd_type *fd_type = container_of(
 		uobj->uapi_object->type_attrs, struct uverbs_obj_fd_type, type);
-	int ret = fd_type->destroy_object(uobj, why);
-
-	if (ib_is_destroy_retryable(ret, why, uobj))
-		return ret;

-	return 0;
+	return fd_type->destroy_object(uobj, why);
 }

 static void remove_handle_fd_uobject(struct ib_uobject *uobj)
@@ -879,6 +861,9 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
 void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 			     enum rdma_remove_reason reason)
 {
+	struct ib_uobject *obj, *next_obj;
+	unsigned long flags;
+
 	down_write(&ufile->hw_destroy_rwsem);

 	/*
@@ -888,7 +873,6 @@ void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 	if (!ufile->ucontext)
 		goto done;

-	ufile->ucontext->cleanup_retryable = true;
 	while (!list_empty(&ufile->uobjects))
 		if (__uverbs_cleanup_ufile(ufile, reason)) {
 			/*
@@ -899,10 +883,16 @@ void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 			break;
 		}

-	ufile->ucontext->cleanup_retryable = false;
-	if (!list_empty(&ufile->uobjects))
-		__uverbs_cleanup_ufile(ufile, reason);
-
+	list_for_each_entry_safe (obj, next_obj, &ufile->uobjects, list) {
+		spin_lock_irqsave(&ufile->uobjects_lock, flags);
+		list_del_init(&obj->list);
+		spin_unlock_irqrestore(&ufile->uobjects_lock, flags);
+		/*
+		 * Pairs with the get in rdma_alloc_commit_uobject(), could
+		 * destroy uobj.
+		 */
+		uverbs_uobject_put(obj);
+	}
 	ufile_destroy_ucontext(ufile, reason);

 done:
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index c89880aec63e..9e6699f54413 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -680,8 +680,7 @@ int ib_uverbs_dealloc_xrcd(struct ib_uobject *uobject, struct ib_xrcd *xrcd,
 		return 0;

 	ret = ib_dealloc_xrcd_user(xrcd, &attrs->driver_udata);
-
-	if (ib_is_destroy_retryable(ret, why, uobject)) {
+	if (ret) {
 		atomic_inc(&xrcd->usecnt);
 		return ret;
 	}
@@ -689,7 +688,7 @@ int ib_uverbs_dealloc_xrcd(struct ib_uobject *uobject, struct ib_xrcd *xrcd,
 	if (inode)
 		xrcd_table_delete(dev, inode);

-	return ret;
+	return 0;
 }

 static int ib_uverbs_reg_mr(struct uverbs_attr_bundle *attrs)
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index 0658101fca00..585042ead939 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -88,7 +88,7 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,
 		return -EBUSY;

 	ret = rwq_ind_tbl->device->ops.destroy_rwq_ind_table(rwq_ind_tbl);
-	if (ib_is_destroy_retryable(ret, why, uobject))
+	if (ret)
 		return ret;

 	for (i = 0; i < table_size; i++)
@@ -96,7 +96,7 @@ static int uverbs_free_rwq_ind_tbl(struct ib_uobject *uobject,

 	kfree(rwq_ind_tbl);
 	kfree(ind_tbl);
-	return ret;
+	return 0;
 }

 static int uverbs_free_xrcd(struct ib_uobject *uobject,
@@ -108,9 +108,8 @@ static int uverbs_free_xrcd(struct ib_uobject *uobject,
 		container_of(uobject, struct ib_uxrcd_object, uobject);
 	int ret;

-	ret = ib_destroy_usecnt(&uxrcd->refcnt, why, uobject);
-	if (ret)
-		return ret;
+	if (atomic_read(&uxrcd->refcnt))
+		return -EBUSY;

 	mutex_lock(&attrs->ufile->device->xrcd_tree_mutex);
 	ret = ib_uverbs_dealloc_xrcd(uobject, xrcd, why, attrs);
@@ -124,11 +123,9 @@ static int uverbs_free_pd(struct ib_uobject *uobject,
 			  struct uverbs_attr_bundle *attrs)
 {
 	struct ib_pd *pd = uobject->object;
-	int ret;

-	ret = ib_destroy_usecnt(&pd->usecnt, why, uobject);
-	if (ret)
-		return ret;
+	if (atomic_read(&pd->usecnt))
+		return -EBUSY;

 	return ib_dealloc_pd_user(pd, &attrs->driver_udata);
 }
diff --git a/drivers/infiniband/core/uverbs_std_types_counters.c b/drivers/infiniband/core/uverbs_std_types_counters.c
index b3c6c066b601..999da9c79866 100644
--- a/drivers/infiniband/core/uverbs_std_types_counters.c
+++ b/drivers/infiniband/core/uverbs_std_types_counters.c
@@ -42,9 +42,8 @@ static int uverbs_free_counters(struct ib_uobject *uobject,
 	struct ib_counters *counters = uobject->object;
 	int ret;

-	ret = ib_destroy_usecnt(&counters->usecnt, why, uobject);
-	if (ret)
-		return ret;
+	if (atomic_read(&counters->usecnt))
+		return -EBUSY;

 	ret = counters->device->ops.destroy_counters(counters);
 	if (ret)
diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c
index 253dabb71f0e..b5e8a6aebecf 100644
--- a/drivers/infiniband/core/uverbs_std_types_cq.c
+++ b/drivers/infiniband/core/uverbs_std_types_cq.c
@@ -46,7 +46,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject,
 	int ret;

 	ret = ib_destroy_cq_user(cq, &attrs->driver_udata);
-	if (ib_is_destroy_retryable(ret, why, uobject))
+	if (ret)
 		return ret;

 	ib_uverbs_release_ucq(
@@ -55,7 +55,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject,
 					ev_queue) :
 			   NULL,
 		ucq);
-	return ret;
+	return 0;
 }

 static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(
diff --git a/drivers/infiniband/core/uverbs_std_types_dm.c b/drivers/infiniband/core/uverbs_std_types_dm.c
index d5a1de33c2c9..98c522cf86d6 100644
--- a/drivers/infiniband/core/uverbs_std_types_dm.c
+++ b/drivers/infiniband/core/uverbs_std_types_dm.c
@@ -39,11 +39,9 @@ static int uverbs_free_dm(struct ib_uobject *uobject,
 			  struct uverbs_attr_bundle *attrs)
 {
 	struct ib_dm *dm = uobject->object;
-	int ret;

-	ret = ib_destroy_usecnt(&dm->usecnt, why, uobject);
-	if (ret)
-		return ret;
+	if (atomic_read(&dm->usecnt))
+		return -EBUSY;

 	return dm->device->ops.dealloc_dm(dm, attrs);
 }
diff --git a/drivers/infiniband/core/uverbs_std_types_flow_action.c b/drivers/infiniband/core/uverbs_std_types_flow_action.c
index 459cf165b231..d42ed7ff223e 100644
--- a/drivers/infiniband/core/uverbs_std_types_flow_action.c
+++ b/drivers/infiniband/core/uverbs_std_types_flow_action.c
@@ -39,11 +39,9 @@ static int uverbs_free_flow_action(struct ib_uobject *uobject,
 				   struct uverbs_attr_bundle *attrs)
 {
 	struct ib_flow_action *action = uobject->object;
-	int ret;

-	ret = ib_destroy_usecnt(&action->usecnt, why, uobject);
-	if (ret)
-		return ret;
+	if (atomic_read(&action->usecnt))
+		return -EBUSY;

 	return action->device->ops.destroy_flow_action(action);
 }
diff --git a/drivers/infiniband/core/uverbs_std_types_qp.c b/drivers/infiniband/core/uverbs_std_types_qp.c
index 198c3cd6f8b9..c00cfb5ed387 100644
--- a/drivers/infiniband/core/uverbs_std_types_qp.c
+++ b/drivers/infiniband/core/uverbs_std_types_qp.c
@@ -32,14 +32,14 @@ static int uverbs_free_qp(struct ib_uobject *uobject,
 	}

 	ret = ib_destroy_qp_user(qp, &attrs->driver_udata);
-	if (ib_is_destroy_retryable(ret, why, uobject))
+	if (ret)
 		return ret;

 	if (uqp->uxrcd)
 		atomic_dec(&uqp->uxrcd->refcnt);

 	ib_uverbs_release_uevent(&uqp->uevent);
-	return ret;
+	return 0;
 }

 static int check_creation_flags(enum ib_qp_type qp_type,
diff --git a/drivers/infiniband/core/uverbs_std_types_srq.c b/drivers/infiniband/core/uverbs_std_types_srq.c
index c0ecbba26bf4..e5513f828bdc 100644
--- a/drivers/infiniband/core/uverbs_std_types_srq.c
+++ b/drivers/infiniband/core/uverbs_std_types_srq.c
@@ -18,7 +18,7 @@ static int uverbs_free_srq(struct ib_uobject *uobject,
 	int ret;

 	ret = ib_destroy_srq_user(srq, &attrs->driver_udata);
-	if (ib_is_destroy_retryable(ret, why, uobject))
+	if (ret)
 		return ret;

 	if (srq_type == IB_SRQT_XRC) {
@@ -30,7 +30,7 @@ static int uverbs_free_srq(struct ib_uobject *uobject,
 	}

 	ib_uverbs_release_uevent(uevent);
-	return ret;
+	return 0;
 }

 static int UVERBS_HANDLER(UVERBS_METHOD_SRQ_CREATE)(
diff --git a/drivers/infiniband/core/uverbs_std_types_wq.c b/drivers/infiniband/core/uverbs_std_types_wq.c
index f2e6a625724a..7ded8339346f 100644
--- a/drivers/infiniband/core/uverbs_std_types_wq.c
+++ b/drivers/infiniband/core/uverbs_std_types_wq.c
@@ -17,11 +17,11 @@ static int uverbs_free_wq(struct ib_uobject *uobject,
 	int ret;

 	ret = ib_destroy_wq_user(wq, &attrs->driver_udata);
-	if (ib_is_destroy_retryable(ret, why, uobject))
+	if (ret)
 		return ret;

 	ib_uverbs_release_uevent(&uwq->uevent);
-	return ret;
+	return 0;
 }

 static int UVERBS_HANDLER(UVERBS_METHOD_WQ_CREATE)(
diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
index 26c6bbb5e77a..7f4b186c146a 100644
--- a/drivers/infiniband/hw/mlx5/devx.c
+++ b/drivers/infiniband/hw/mlx5/devx.c
@@ -1308,7 +1308,7 @@ static int devx_obj_cleanup(struct ib_uobject *uobject,
 	else
 		ret = mlx5_cmd_exec(obj->ib_dev->mdev, obj->dinbox,
 				    obj->dinlen, out, sizeof(out));
-	if (ib_is_destroy_retryable(ret, why, uobject))
+	if (ret)
 		return ret;

 	devx_event_table = &dev->devx_event_table;
@@ -2175,7 +2175,7 @@ static int devx_umem_cleanup(struct ib_uobject *uobject,
 	int err;

 	err = mlx5_cmd_exec(obj->mdev, obj->dinbox, obj->dinlen, out, sizeof(out));
-	if (ib_is_destroy_retryable(err, why, uobject))
+	if (err)
 		return err;

 	ib_umem_release(obj->umem);
diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c
index 492cfe063bca..25da0b05b4e2 100644
--- a/drivers/infiniband/hw/mlx5/fs.c
+++ b/drivers/infiniband/hw/mlx5/fs.c
@@ -2035,11 +2035,9 @@ static int flow_matcher_cleanup(struct ib_uobject *uobject,
 				struct uverbs_attr_bundle *attrs)
 {
 	struct mlx5_ib_flow_matcher *obj = uobject->object;
-	int ret;

-	ret = ib_destroy_usecnt(&obj->usecnt, why, uobject);
-	if (ret)
-		return ret;
+	if (atomic_read(&obj->usecnt))
+		return -EBUSY;

 	kfree(obj);
 	return 0;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index d1b8f374aafa..d68868173f92 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1481,8 +1481,6 @@ struct ib_ucontext {
 	struct ib_device       *device;
 	struct ib_uverbs_file  *ufile;

-	bool cleanup_retryable;
-
 	struct ib_rdmacg_object	cg_obj;
 	/*
 	 * Implementation details of the RDMA core, don't use in drivers:
@@ -2898,46 +2896,6 @@ static inline bool ib_is_udata_cleared(struct ib_udata *udata,
 	return ib_is_buffer_cleared(udata->inbuf + offset, len);
 }

-/**
- * ib_is_destroy_retryable - Check whether the uobject destruction
- * is retryable.
- * @ret: The initial destruction return code
- * @why: remove reason
- * @uobj: The uobject that is destroyed
- *
- * This function is a helper function that IB layer and low-level drivers
- * can use to consider whether the destruction of the given uobject is
- * retry-able.
- * It checks the original return code, if it wasn't success the destruction
- * is retryable according to the ucontext state (i.e. cleanup_retryable) and
- * the remove reason. (i.e. why).
- * Must be called with the object locked for destroy.
- */
-static inline bool ib_is_destroy_retryable(int ret, enum rdma_remove_reason why,
-					   struct ib_uobject *uobj)
-{
-	return ret && (why == RDMA_REMOVE_DESTROY ||
-		       uobj->context->cleanup_retryable);
-}
-
-/**
- * ib_destroy_usecnt - Called during destruction to check the usecnt
- * @usecnt: The usecnt atomic
- * @why: remove reason
- * @uobj: The uobject that is destroyed
- *
- * Non-zero usecnts will block destruction unless destruction was triggered by
- * a ucontext cleanup.
- */
-static inline int ib_destroy_usecnt(atomic_t *usecnt,
-				    enum rdma_remove_reason why,
-				    struct ib_uobject *uobj)
-{
-	if (atomic_read(usecnt) && ib_is_destroy_retryable(-EBUSY, why, uobj))
-		return -EBUSY;
-	return 0;
-}
-
 /**
  * ib_modify_qp_is_ok - Check that the supplied attribute mask
  * contains all required attributes and no attributes not allowed for
--
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH rdma-rc 2/3] RDMA/core: Make FD destroy callback void
  2020-10-12  4:55 [PATCH rdma-rc 0/3] Fixes to coming PR Leon Romanovsky
  2020-10-12  4:55 ` [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close Leon Romanovsky
@ 2020-10-12  4:55 ` Leon Romanovsky
  2020-10-12  4:56 ` [PATCH rdma-rc 3/3] RDMA/ucma: Fix use after free in destroy id flow Leon Romanovsky
  2 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-12  4:55 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: Leon Romanovsky, linux-rdma

From: Leon Romanovsky <leonro@nvidia.com>

All FD object destroy implementations return 0, so declare
this callback void.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/rdma_core.c                 |  3 ++-
 drivers/infiniband/core/uverbs_std_types.c          |  3 +--
 drivers/infiniband/core/uverbs_std_types_async_fd.c |  5 ++---
 drivers/infiniband/hw/mlx5/devx.c                   | 10 ++++------
 include/rdma/uverbs_types.h                         |  4 ++--
 5 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 9d5f2faae181..badbee2c39aa 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -568,7 +568,8 @@ static int __must_check destroy_hw_fd_uobject(struct ib_uobject *uobj,
 	const struct uverbs_obj_fd_type *fd_type = container_of(
 		uobj->uapi_object->type_attrs, struct uverbs_obj_fd_type, type);

-	return fd_type->destroy_object(uobj, why);
+	fd_type->destroy_object(uobj, why);
+	return 0;
 }

 static void remove_handle_fd_uobject(struct ib_uobject *uobj)
diff --git a/drivers/infiniband/core/uverbs_std_types.c b/drivers/infiniband/core/uverbs_std_types.c
index 585042ead939..13776a66e2e4 100644
--- a/drivers/infiniband/core/uverbs_std_types.c
+++ b/drivers/infiniband/core/uverbs_std_types.c
@@ -154,7 +154,7 @@ void ib_uverbs_free_event_queue(struct ib_uverbs_event_queue *event_queue)
 	spin_unlock_irq(&event_queue->lock);
 }

-static int
+static void
 uverbs_completion_event_file_destroy_uobj(struct ib_uobject *uobj,
 					  enum rdma_remove_reason why)
 {
@@ -163,7 +163,6 @@ uverbs_completion_event_file_destroy_uobj(struct ib_uobject *uobj,
 			     uobj);

 	ib_uverbs_free_event_queue(&file->ev_queue);
-	return 0;
 }

 int uverbs_destroy_def_handler(struct uverbs_attr_bundle *attrs)
diff --git a/drivers/infiniband/core/uverbs_std_types_async_fd.c b/drivers/infiniband/core/uverbs_std_types_async_fd.c
index 61899eaf1f91..cc24cfdf7aee 100644
--- a/drivers/infiniband/core/uverbs_std_types_async_fd.c
+++ b/drivers/infiniband/core/uverbs_std_types_async_fd.c
@@ -19,8 +19,8 @@ static int UVERBS_HANDLER(UVERBS_METHOD_ASYNC_EVENT_ALLOC)(
 	return 0;
 }

-static int uverbs_async_event_destroy_uobj(struct ib_uobject *uobj,
-					   enum rdma_remove_reason why)
+static void uverbs_async_event_destroy_uobj(struct ib_uobject *uobj,
+					    enum rdma_remove_reason why)
 {
 	struct ib_uverbs_async_event_file *event_file =
 		container_of(uobj, struct ib_uverbs_async_event_file, uobj);
@@ -30,7 +30,6 @@ static int uverbs_async_event_destroy_uobj(struct ib_uobject *uobj,
 	if (why == RDMA_REMOVE_DRIVER_REMOVE)
 		ib_uverbs_async_handler(event_file, 0, IB_EVENT_DEVICE_FATAL,
 					NULL, NULL);
-	return 0;
 }

 int uverbs_async_event_release(struct inode *inode, struct file *filp)
diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
index 7f4b186c146a..b20ff4a64ab2 100644
--- a/drivers/infiniband/hw/mlx5/devx.c
+++ b/drivers/infiniband/hw/mlx5/devx.c
@@ -2588,8 +2588,8 @@ static const struct file_operations devx_async_event_fops = {
 	.llseek	 = no_llseek,
 };

-static int devx_async_cmd_event_destroy_uobj(struct ib_uobject *uobj,
-					     enum rdma_remove_reason why)
+static void devx_async_cmd_event_destroy_uobj(struct ib_uobject *uobj,
+					      enum rdma_remove_reason why)
 {
 	struct devx_async_cmd_event_file *comp_ev_file =
 		container_of(uobj, struct devx_async_cmd_event_file,
@@ -2611,11 +2611,10 @@ static int devx_async_cmd_event_destroy_uobj(struct ib_uobject *uobj,
 		kvfree(entry);
 	}
 	spin_unlock_irq(&comp_ev_file->ev_queue.lock);
-	return 0;
 };

-static int devx_async_event_destroy_uobj(struct ib_uobject *uobj,
-					 enum rdma_remove_reason why)
+static void devx_async_event_destroy_uobj(struct ib_uobject *uobj,
+					  enum rdma_remove_reason why)
 {
 	struct devx_async_event_file *ev_file =
 		container_of(uobj, struct devx_async_event_file,
@@ -2659,7 +2658,6 @@ static int devx_async_event_destroy_uobj(struct ib_uobject *uobj,
 	mutex_unlock(&dev->devx_event_table.event_xa_lock);

 	put_device(&dev->ib_dev.dev);
-	return 0;
 };

 DECLARE_UVERBS_NAMED_METHOD(
diff --git a/include/rdma/uverbs_types.h b/include/rdma/uverbs_types.h
index 06db27e35f40..a27e9fb4903f 100644
--- a/include/rdma/uverbs_types.h
+++ b/include/rdma/uverbs_types.h
@@ -138,8 +138,8 @@ struct uverbs_obj_fd_type {
 	 * because the driver is removed or the FD is closed.
 	 */
 	struct uverbs_obj_type  type;
-	int (*destroy_object)(struct ib_uobject *uobj,
-			      enum rdma_remove_reason why);
+	void (*destroy_object)(struct ib_uobject *uobj,
+			       enum rdma_remove_reason why);
 	const struct file_operations	*fops;
 	const char			*name;
 	int				flags;
--
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH rdma-rc 3/3] RDMA/ucma: Fix use after free in destroy id flow
  2020-10-12  4:55 [PATCH rdma-rc 0/3] Fixes to coming PR Leon Romanovsky
  2020-10-12  4:55 ` [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close Leon Romanovsky
  2020-10-12  4:55 ` [PATCH rdma-rc 2/3] RDMA/core: Make FD destroy callback void Leon Romanovsky
@ 2020-10-12  4:56 ` Leon Romanovsky
  2020-10-16 17:09   ` Jason Gunthorpe
  2 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-12  4:56 UTC (permalink / raw)
  To: Doug Ledford, Jason Gunthorpe; +Cc: Maor Gottlieb, linux-rdma

From: Maor Gottlieb <maorg@nvidia.com>

ucma_free_ctx should call to __destroy_id on all the connection
requests that have not been delivered to user space. Currently
it calls on the context itself and cause to use after free.

Fixes the below trace:
BUG: Unable to handle kernel data access on write at
0x5deadbeef0000108
Faulting instruction address: 0xc0080000002428f4
Oops: Kernel access of bad area, sig: 11 [#1]
Call Trace:
[c000000207f2b680] [c00800000024280c] .__destroy_id+0x28c/0x610 [rdma_ucm] (unreliable)
[c000000207f2b750] [c0080000002429c4] .__destroy_id+0x444/0x610 [rdma_ucm]
[c000000207f2b820] [c008000000242c24] .ucma_close+0x94/0xf0 [rdma_ucm]
[c000000207f2b8c0] [c00000000046fbdc] .__fput+0xac/0x330
[c000000207f2b960] [c00000000015d48c] .task_work_run+0xbc/0x110
[c000000207f2b9f0] [c00000000012fb00] .do_exit+0x430/0xc50
[c000000207f2bae0] [c0000000001303ec] .do_group_exit+0x5c/0xd0
[c000000207f2bb70] [c000000000144a34] .get_signal+0x194/0xe30
[c000000207f2bc60] [c00000000001f6b4] .do_notify_resume+0x124/0x470
[c000000207f2bd60] [c000000000032484]
.interrupt_exit_user_prepare+0x1b4/0x240
[c000000207f2be20] [c000000000010034] interrupt_return+0x14/0x1c0
Instruction dump:
7d094378 3906ffe8 4082ffa8 3f205dea 3f405dea e95d0120 e91d0118
6339dbee
635adbee e93f0888 7b3907c6 7b5a07c6 <f9480008> 6739f000 f90a0000
675af000
---[ end trace 9796e2b012b61b83 ]---

Fixes: a1d33b70dbbc ("RDMA/ucma: Rework how new connections are passed through event delivery")
Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/ucma.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c
index 08a628246bd6..ffe2563ad345 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -112,7 +112,7 @@ struct ucma_multicast {

 struct ucma_event {
 	struct ucma_context	*ctx;
-	struct ucma_context	*listen_ctx;
+	struct ucma_context	*conn_req_ctx;
 	struct ucma_multicast	*mc;
 	struct list_head	list;
 	struct rdma_ucm_event_resp resp;
@@ -308,7 +308,7 @@ static int ucma_connect_event_handler(struct rdma_cm_id *cm_id,
 	uevent = ucma_create_uevent(listen_ctx, event);
 	if (!uevent)
 		goto err_alloc;
-	uevent->listen_ctx = listen_ctx;
+	uevent->conn_req_ctx = ctx;
 	uevent->resp.id = ctx->id;

 	ctx->cm_id->context = ctx;
@@ -534,7 +534,7 @@ static int ucma_free_ctx(struct ucma_context *ctx)
 	/* Cleanup events not yet reported to the user. */
 	mutex_lock(&ctx->file->mut);
 	list_for_each_entry_safe(uevent, tmp, &ctx->file->event_list, list) {
-		if (uevent->ctx == ctx || uevent->listen_ctx == ctx)
+		if (uevent->ctx == ctx || uevent->conn_req_ctx == ctx)
 			list_move_tail(&uevent->list, &list);
 	}
 	list_del(&ctx->list);
@@ -548,8 +548,9 @@ static int ucma_free_ctx(struct ucma_context *ctx)
 	 */
 	list_for_each_entry_safe(uevent, tmp, &list, list) {
 		list_del(&uevent->list);
-		if (uevent->resp.event == RDMA_CM_EVENT_CONNECT_REQUEST)
-			__destroy_id(uevent->ctx);
+		if (uevent->resp.event == RDMA_CM_EVENT_CONNECT_REQUEST &&
+		    uevent->conn_req_ctx != ctx)
+			__destroy_id(uevent->conn_req_ctx);
 		kfree(uevent);
 	}

--
2.26.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-rc 3/3] RDMA/ucma: Fix use after free in destroy id flow
  2020-10-12  4:56 ` [PATCH rdma-rc 3/3] RDMA/ucma: Fix use after free in destroy id flow Leon Romanovsky
@ 2020-10-16 17:09   ` Jason Gunthorpe
  0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2020-10-16 17:09 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: Doug Ledford, Maor Gottlieb, linux-rdma

On Mon, Oct 12, 2020 at 07:56:00AM +0300, Leon Romanovsky wrote:
> From: Maor Gottlieb <maorg@nvidia.com>
> 
> ucma_free_ctx should call to __destroy_id on all the connection
> requests that have not been delivered to user space. Currently
> it calls on the context itself and cause to use after free.
> 
> Fixes the below trace:
> BUG: Unable to handle kernel data access on write at
> 0x5deadbeef0000108
> Faulting instruction address: 0xc0080000002428f4
> Oops: Kernel access of bad area, sig: 11 [#1]
> Call Trace:
> [c000000207f2b680] [c00800000024280c] .__destroy_id+0x28c/0x610 [rdma_ucm] (unreliable)
> [c000000207f2b750] [c0080000002429c4] .__destroy_id+0x444/0x610 [rdma_ucm]
> [c000000207f2b820] [c008000000242c24] .ucma_close+0x94/0xf0 [rdma_ucm]
> [c000000207f2b8c0] [c00000000046fbdc] .__fput+0xac/0x330
> [c000000207f2b960] [c00000000015d48c] .task_work_run+0xbc/0x110
> [c000000207f2b9f0] [c00000000012fb00] .do_exit+0x430/0xc50
> [c000000207f2bae0] [c0000000001303ec] .do_group_exit+0x5c/0xd0
> [c000000207f2bb70] [c000000000144a34] .get_signal+0x194/0xe30
> [c000000207f2bc60] [c00000000001f6b4] .do_notify_resume+0x124/0x470
> [c000000207f2bd60] [c000000000032484]
> .interrupt_exit_user_prepare+0x1b4/0x240
> [c000000207f2be20] [c000000000010034] interrupt_return+0x14/0x1c0
> Instruction dump:
> 7d094378 3906ffe8 4082ffa8 3f205dea 3f405dea e95d0120 e91d0118
> 6339dbee
> 635adbee e93f0888 7b3907c6 7b5a07c6 <f9480008> 6739f000 f90a0000
> 675af000
> ---[ end trace 9796e2b012b61b83 ]---
> 
> Fixes: a1d33b70dbbc ("RDMA/ucma: Rework how new connections are passed through event delivery")
> Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/core/ucma.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Don't word wrap oops messages

Applied to for-next 

Thanks,
Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close
  2020-10-12  4:55 ` [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close Leon Romanovsky
@ 2020-10-27 16:55   ` Jason Gunthorpe
  2020-10-27 17:11     ` Jason Gunthorpe
  2020-10-29 11:49     ` Leon Romanovsky
  0 siblings, 2 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2020-10-27 16:55 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: Doug Ledford, Leon Romanovsky, linux-rdma

On Mon, Oct 12, 2020 at 07:55:58AM +0300, Leon Romanovsky wrote:
> @@ -543,17 +537,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
>  			     struct uverbs_obj_idr_type, type);
>  	int ret = idr_type->destroy_object(uobj, why, attrs);
> 
> -	/*
> -	 * We can only fail gracefully if the user requested to destroy the
> -	 * object or when a retry may be called upon an error.
> -	 * In the rest of the cases, just remove whatever you can.
> -	 */
> -	if (ib_is_destroy_retryable(ret, why, uobj))
> +	if (ret)
>  		return ret;
> 
> -	if (why == RDMA_REMOVE_ABORT)
> -		return 0;

This shouldn't be deleted..

There are also a few too many WARN_ONs if this path triggers, I came up
with this:

diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 3d366cb79cef42..3ae878f3d173d3 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -540,6 +540,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
 	if (ret)
 		return ret;
 
+	if (why == RDMA_REMOVE_ABORT)
+		return 0;
+
 	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
 			   RDMACG_RESOURCE_HCA_OBJECT);
 
@@ -727,10 +730,8 @@ void release_ufile_idr_uobject(struct ib_uverbs_file *ufile)
 	 *
 	 * This is an optimized equivalent to remove_handle_idr_uobject
 	 */
-	xa_for_each(&ufile->idr, id, entry) {
-		WARN_ON(entry->object);
+	xa_for_each(&ufile->idr, id, entry)
 		uverbs_uobject_put(entry);
-	}
 
 	xa_destroy(&ufile->idr);
 }
@@ -875,25 +876,31 @@ void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 		goto done;
 
 	while (!list_empty(&ufile->uobjects))
-		if (__uverbs_cleanup_ufile(ufile, reason)) {
+		if (__uverbs_cleanup_ufile(ufile, reason))
+			break;
+
+	/*
+	 * In case destruction failed try to free as much memory as we can,
+	 * and leak the HW objects.
+	 */
+	if (!list_empty(&ufile->uobjects)) {
+		WARN(true, "RDMA driver did not destroy all HW objects, leaking memory");
+		list_for_each_entry_safe (obj, next_obj, &ufile->uobjects,
+					  list) {
+			spin_lock_irqsave(&ufile->uobjects_lock, flags);
+			list_del_init(&obj->list);
+			spin_unlock_irqrestore(&ufile->uobjects_lock, flags);
 			/*
-			 * No entry was cleaned-up successfully during this
-			 * iteration. It is a driver bug to fail destruction.
+			 * Pairs with the get in rdma_alloc_commit_uobject(),
+			 * could destroy uobj.
 			 */
-			WARN_ON(!list_empty(&ufile->uobjects));
-			break;
+			uverbs_uobject_put(obj);
 		}
-
-	list_for_each_entry_safe (obj, next_obj, &ufile->uobjects, list) {
-		spin_lock_irqsave(&ufile->uobjects_lock, flags);
-		list_del_init(&obj->list);
-		spin_unlock_irqrestore(&ufile->uobjects_lock, flags);
-		/*
-		 * Pairs with the get in rdma_alloc_commit_uobject(), could
-		 * destroy uobj.
-		 */
-		uverbs_uobject_put(obj);
+		/* release_ufile_idr_uobject() will clean up the IDR */
+	} else {
+		WARN_ON(!xa_empty(&ufile->idr));
 	}
+
 	ufile_destroy_ucontext(ufile, reason);
 
 done:

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close
  2020-10-27 16:55   ` Jason Gunthorpe
@ 2020-10-27 17:11     ` Jason Gunthorpe
  2020-11-01 19:50       ` Leon Romanovsky
  2020-10-29 11:49     ` Leon Romanovsky
  1 sibling, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2020-10-27 17:11 UTC (permalink / raw)
  To: Leon Romanovsky; +Cc: Doug Ledford, Leon Romanovsky, linux-rdma

On Tue, Oct 27, 2020 at 01:55:08PM -0300, Jason Gunthorpe wrote:

> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index 3d366cb79cef42..3ae878f3d173d3 100644
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -540,6 +540,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
>  	if (ret)
>  		return ret;
>  
> +	if (why == RDMA_REMOVE_ABORT)
> +		return 0;
> +
>  	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
>  			   RDMACG_RESOURCE_HCA_OBJECT);
>  
> @@ -727,10 +730,8 @@ void release_ufile_idr_uobject(struct ib_uverbs_file *ufile)
>  	 *
>  	 * This is an optimized equivalent to remove_handle_idr_uobject
>  	 */
> -	xa_for_each(&ufile->idr, id, entry) {
> -		WARN_ON(entry->object);
> +	xa_for_each(&ufile->idr, id, entry)
>  		uverbs_uobject_put(entry);
> -	}

Actually this is not a good idea

This one is better:

diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 3d366cb79cef42..fd012be700ccc2 100644
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -540,6 +540,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
 	if (ret)
 		return ret;
 
+	if (why == RDMA_REMOVE_ABORT)
+		return 0;
+
 	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
 			   RDMACG_RESOURCE_HCA_OBJECT);
 
@@ -845,11 +848,17 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
 		 * racing with a lookup_get.
 		 */
 		WARN_ON(uverbs_try_lock_object(obj, UVERBS_LOOKUP_WRITE));
+		if (reason == RDMA_REMOVE_DRIVER_FAILURE)
+			obj->object = NULL;
 		if (!uverbs_destroy_uobject(obj, reason, &attrs))
 			ret = 0;
 		else
 			atomic_set(&obj->usecnt, 0);
 	}
+	if (reason == RDMA_REMOVE_DRIVER_FAILURE) {
+		WARN_ON(!list_empty(&ufile->uobjects));
+		return 0;
+	}
 	return ret;
 }
 
@@ -862,9 +871,6 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
 void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 			     enum rdma_remove_reason reason)
 {
-	struct ib_uobject *obj, *next_obj;
-	unsigned long flags;
-
 	down_write(&ufile->hw_destroy_rwsem);
 
 	/*
@@ -875,25 +881,10 @@ void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
 		goto done;
 
 	while (!list_empty(&ufile->uobjects))
-		if (__uverbs_cleanup_ufile(ufile, reason)) {
-			/*
-			 * No entry was cleaned-up successfully during this
-			 * iteration. It is a driver bug to fail destruction.
-			 */
-			WARN_ON(!list_empty(&ufile->uobjects));
+		if (__uverbs_cleanup_ufile(ufile, reason))
 			break;
-		}
-
-	list_for_each_entry_safe (obj, next_obj, &ufile->uobjects, list) {
-		spin_lock_irqsave(&ufile->uobjects_lock, flags);
-		list_del_init(&obj->list);
-		spin_unlock_irqrestore(&ufile->uobjects_lock, flags);
-		/*
-		 * Pairs with the get in rdma_alloc_commit_uobject(), could
-		 * destroy uobj.
-		 */
-		uverbs_uobject_put(obj);
-	}
+	if (WARN_ON(!list_empty(&ufile->uobjects)))
+		__uverbs_cleanup_ufile(ufile, RDMA_REMOVE_DRIVER_FAILURE);
 	ufile_destroy_ucontext(ufile, reason);
 
 done:
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index edfc1d7d3766ca..7e330f4a6d33ff 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1471,6 +1471,8 @@ enum rdma_remove_reason {
 	RDMA_REMOVE_DRIVER_REMOVE,
 	/* uobj is being cleaned-up before being committed */
 	RDMA_REMOVE_ABORT,
+	/* The driver failed to destroy the uobject and is being disconnected */
+	RDMA_REMOVE_DRIVER_FAILURE,
 };
 
 struct ib_rdmacg_object {

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close
  2020-10-27 16:55   ` Jason Gunthorpe
  2020-10-27 17:11     ` Jason Gunthorpe
@ 2020-10-29 11:49     ` Leon Romanovsky
  1 sibling, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-29 11:49 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma

On Tue, Oct 27, 2020 at 01:55:08PM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 12, 2020 at 07:55:58AM +0300, Leon Romanovsky wrote:
> > @@ -543,17 +537,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
> >  			     struct uverbs_obj_idr_type, type);
> >  	int ret = idr_type->destroy_object(uobj, why, attrs);
> >
> > -	/*
> > -	 * We can only fail gracefully if the user requested to destroy the
> > -	 * object or when a retry may be called upon an error.
> > -	 * In the rest of the cases, just remove whatever you can.
> > -	 */
> > -	if (ib_is_destroy_retryable(ret, why, uobj))
> > +	if (ret)
> >  		return ret;
> >
> > -	if (why == RDMA_REMOVE_ABORT)
> > -		return 0;
>
> This shouldn't be deleted..

Yes, this is what we noticed too.

I'll add your version for a couple of days to our regression.

Thanks

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close
  2020-10-27 17:11     ` Jason Gunthorpe
@ 2020-11-01 19:50       ` Leon Romanovsky
  0 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2020-11-01 19:50 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma

On Tue, Oct 27, 2020 at 02:11:22PM -0300, Jason Gunthorpe wrote:
> On Tue, Oct 27, 2020 at 01:55:08PM -0300, Jason Gunthorpe wrote:
>
> > diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> > index 3d366cb79cef42..3ae878f3d173d3 100644
> > +++ b/drivers/infiniband/core/rdma_core.c
> > @@ -540,6 +540,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
> >  	if (ret)
> >  		return ret;
> >
> > +	if (why == RDMA_REMOVE_ABORT)
> > +		return 0;
> > +
> >  	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
> >  			   RDMACG_RESOURCE_HCA_OBJECT);
> >
> > @@ -727,10 +730,8 @@ void release_ufile_idr_uobject(struct ib_uverbs_file *ufile)
> >  	 *
> >  	 * This is an optimized equivalent to remove_handle_idr_uobject
> >  	 */
> > -	xa_for_each(&ufile->idr, id, entry) {
> > -		WARN_ON(entry->object);
> > +	xa_for_each(&ufile->idr, id, entry)
> >  		uverbs_uobject_put(entry);
> > -	}
>
> Actually this is not a good idea
>
> This one is better:

This causes to many syzkaller bugs, I didn't debug yet.

Thanks

>
> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index 3d366cb79cef42..fd012be700ccc2 100644
> --- a/drivers/infiniband/core/rdma_core.c
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -540,6 +540,9 @@ static int __must_check destroy_hw_idr_uobject(struct ib_uobject *uobj,
>  	if (ret)
>  		return ret;
>
> +	if (why == RDMA_REMOVE_ABORT)
> +		return 0;
> +
>  	ib_rdmacg_uncharge(&uobj->cg_obj, uobj->context->device,
>  			   RDMACG_RESOURCE_HCA_OBJECT);
>
> @@ -845,11 +848,17 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
>  		 * racing with a lookup_get.
>  		 */
>  		WARN_ON(uverbs_try_lock_object(obj, UVERBS_LOOKUP_WRITE));
> +		if (reason == RDMA_REMOVE_DRIVER_FAILURE)
> +			obj->object = NULL;
>  		if (!uverbs_destroy_uobject(obj, reason, &attrs))
>  			ret = 0;
>  		else
>  			atomic_set(&obj->usecnt, 0);
>  	}
> +	if (reason == RDMA_REMOVE_DRIVER_FAILURE) {
> +		WARN_ON(!list_empty(&ufile->uobjects));
> +		return 0;
> +	}
>  	return ret;
>  }
>
> @@ -862,9 +871,6 @@ static int __uverbs_cleanup_ufile(struct ib_uverbs_file *ufile,
>  void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
>  			     enum rdma_remove_reason reason)
>  {
> -	struct ib_uobject *obj, *next_obj;
> -	unsigned long flags;
> -
>  	down_write(&ufile->hw_destroy_rwsem);
>
>  	/*
> @@ -875,25 +881,10 @@ void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
>  		goto done;
>
>  	while (!list_empty(&ufile->uobjects))
> -		if (__uverbs_cleanup_ufile(ufile, reason)) {
> -			/*
> -			 * No entry was cleaned-up successfully during this
> -			 * iteration. It is a driver bug to fail destruction.
> -			 */
> -			WARN_ON(!list_empty(&ufile->uobjects));
> +		if (__uverbs_cleanup_ufile(ufile, reason))
>  			break;
> -		}
> -
> -	list_for_each_entry_safe (obj, next_obj, &ufile->uobjects, list) {
> -		spin_lock_irqsave(&ufile->uobjects_lock, flags);
> -		list_del_init(&obj->list);
> -		spin_unlock_irqrestore(&ufile->uobjects_lock, flags);
> -		/*
> -		 * Pairs with the get in rdma_alloc_commit_uobject(), could
> -		 * destroy uobj.
> -		 */
> -		uverbs_uobject_put(obj);
> -	}
> +	if (WARN_ON(!list_empty(&ufile->uobjects)))
> +		__uverbs_cleanup_ufile(ufile, RDMA_REMOVE_DRIVER_FAILURE);
>  	ufile_destroy_ucontext(ufile, reason);
>
>  done:
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index edfc1d7d3766ca..7e330f4a6d33ff 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1471,6 +1471,8 @@ enum rdma_remove_reason {
>  	RDMA_REMOVE_DRIVER_REMOVE,
>  	/* uobj is being cleaned-up before being committed */
>  	RDMA_REMOVE_ABORT,
> +	/* The driver failed to destroy the uobject and is being disconnected */
> +	RDMA_REMOVE_DRIVER_FAILURE,
>  };
>
>  struct ib_rdmacg_object {

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-11-01 19:50 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-12  4:55 [PATCH rdma-rc 0/3] Fixes to coming PR Leon Romanovsky
2020-10-12  4:55 ` [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close Leon Romanovsky
2020-10-27 16:55   ` Jason Gunthorpe
2020-10-27 17:11     ` Jason Gunthorpe
2020-11-01 19:50       ` Leon Romanovsky
2020-10-29 11:49     ` Leon Romanovsky
2020-10-12  4:55 ` [PATCH rdma-rc 2/3] RDMA/core: Make FD destroy callback void Leon Romanovsky
2020-10-12  4:56 ` [PATCH rdma-rc 3/3] RDMA/ucma: Fix use after free in destroy id flow Leon Romanovsky
2020-10-16 17:09   ` Jason Gunthorpe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).