RDMA/core: Change how failing destroy is handled during uobj abort

PATCH Applied Leon Romanovsky 2 participants 2 messages Sep 2, 2020 – Sep 9, 2020 InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2020-09-02 08:17 UTC #0 lore raw

PATCH 1/1 RDMA/core: Change how failing destroy is handled during uobj abort

From: Jason Gunthorpe <[email protected]>

Currently it triggers a WARN_ON and then goes ahead and destroys the uobject anyhow, leaking any driver memory.

The only place that leaks driver memory should be during FD close() in uverbs_destroy_ufile_hw().

Drivers are only allowed to fail destroy uobjects if they guarentee destroy will eventually succeed. uverbs_destroy_ufile_hw() provides the loop to give the driver that chance.

Signed-off-by Jason Gunthorpe <[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/rdma_core.c | 30 ++++++++++++++---------------
 include/rdma/ib_verbs.h             |  5 -----
 2 files changed, 15 insertions(+), 20 deletions(-)

drivers/infiniband/core/rdma_core.c +15 ~~-15~~

index d5bf25c6eadc..492ee941ecdf 100644
@@ -130,17 +130,6 @@ static int uverbs_destroy_uobject(struct ib_uobject *uobj,
 	lockdep_assert_held(&ufile->hw_destroy_rwsem);
 	assert_uverbs_usecnt(uobj, UVERBS_LOOKUP_WRITE);
 
-	if (reason == RDMA_REMOVE_ABORT_HWOBJ) {
-		reason = RDMA_REMOVE_ABORT;
-		ret = uobj->uapi_object->type_class->destroy_hw(uobj, reason,
-								attrs);
-		/*
-		 * Drivers are not permitted to ignore RDMA_REMOVE_ABORT, see
-		 * ib_is_destroy_retryable, cleanup_retryable == false here.
-		 */
-		WARN_ON(ret);
-	}
-
 	if (reason == RDMA_REMOVE_ABORT) {
 		WARN_ON(!list_empty(&uobj->list));
 		WARN_ON(!uobj->context);

@@ -674,11 +663,22 @@ void rdma_alloc_abort_uobject(struct ib_uobject *uobj,
 			      bool hw_obj_valid)
 {
 	struct ib_uverbs_file *ufile = uobj->ufile;
+	int ret;
+
+	if (hw_obj_valid) {
+		ret = uobj->uapi_object->type_class->destroy_hw(
+			uobj, RDMA_REMOVE_ABORT, attrs);
+		/*
+		 * If the driver couldn't destroy the object then go ahead and
+		 * commit it. Leaking objects that can't be destroyed is only
+		 * done during FD close after the driver has a few more tries to
+		 * destroy it.
+		 */
+		if (WARN_ON(ret))
+			return rdma_alloc_commit_uobject(uobj, attrs);
+	}
 
-	uverbs_destroy_uobject(uobj,
-			       hw_obj_valid ? RDMA_REMOVE_ABORT_HWOBJ :
-					      RDMA_REMOVE_ABORT,
-			       attrs);
+	uverbs_destroy_uobject(uobj, RDMA_REMOVE_ABORT, attrs);
 
 	/* Matches the down_read in rdma_alloc_begin_uobject */
 	up_read(&ufile->hw_destroy_rwsem);

include/rdma/ib_verbs.h +0 -5

index 73738fe91e5b..72805a7813c9 100644
@@ -1464,11 +1464,6 @@ enum rdma_remove_reason {
 	RDMA_REMOVE_DRIVER_REMOVE,
 	/* uobj is being cleaned-up before being committed */
 	RDMA_REMOVE_ABORT,
-	/*
-	 * uobj has been fully created, with the uobj->object set, but is being
-	 * cleaned up before being comitted
-	 */
-	RDMA_REMOVE_ABORT_HWOBJ,
 };
 
 struct ib_rdmacg_object {

Signature · 1 line

2.26.2

Jason Gunthorpe

maintainer Applied

2020-09-09 16:21 UTC #1 lore raw

Quoted 12 lines · Leon Romanovsky

From: Jason Gunthorpe <[email protected]>
Currently it triggers a WARN_ON and then goes ahead and destroys the uobject anyhow, leaking any driver memory.
The only place that leaks driver memory should be during FD close() in uverbs_destroy_ufile_hw().
Drivers are only allowed to fail destroy uobjects if they guarentee destroy will eventually succeed. uverbs_destroy_ufile_hw() provides the loop to give the driver that chance.

Quoted 6 lines

Signed-off-by Jason Gunthorpe <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/core/rdma_core.c | 30 ++++++++++++++---------------
 include/rdma/ib_verbs.h             |  5 -----
 2 files changed, 15 insertions(+), 20 deletions(-)

Applied to for-next

Jason