Re: [PATCH rdma-rc 1/3] RDMA/core: Postpone uobject cleanup on failure till FD close

[Jason Gunthorpe <jgg@nvidia.com>]

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: