IB/uverbs: Allow RDMA_REMOVE_DESTROY to work concurrently with disassociate

After all the recent structural changes this is now straightfoward, hoist
the hw_destroy_rwsem up out of rdma_destroy_explicit and wrap it around
the uobject write lock as well as the destroy.

This is necessary as obtaining a write lock concurrently with
uverbs_destroy_ufile_hw() will cause malfunction.

After this change none of the destroy callbacks require the
disassociate_srcu lock to be correct.

This requires introducing a new lookup mode, UVERBS_LOOKUP_DESTROY as the
IOCTL interface needs to hold an unlocked kref until all command
verification is completed.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>

diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
index 435dbe8ef2a28d7ffaff135044ac511eb9d5cb1d..81d668abe18e45cd4dee1ea44725b4d572e96cbc 100644 (file)
--- a/drivers/infiniband/core/rdma_core.c
+++ b/drivers/infiniband/core/rdma_core.c
@@ -127,8 +127,10 @@ static int uverbs_try_lock_object(struct ib_uobject *uobj,
                return __atomic_add_unless(&uobj->usecnt, 1, -1) == -1 ?
                        -EBUSY : 0;
        case UVERBS_LOOKUP_WRITE:
-               /* lock is either WRITE or DESTROY - should be exclusive */
+               /* lock is exclusive */
                return atomic_cmpxchg(&uobj->usecnt, 0, -1) == 0 ? 0 : -EBUSY;
+       case UVERBS_LOOKUP_DESTROY:
+               return 0;
        }
        return 0;
 }
@@ -144,6 +146,8 @@ static void assert_uverbs_usecnt(struct ib_uobject *uobj,
        case UVERBS_LOOKUP_WRITE:
                WARN_ON(atomic_read(&uobj->usecnt) != -1);
                break;
+       case UVERBS_LOOKUP_DESTROY:
+               break;
        }
 #endif
 }
@@ -227,6 +231,35 @@ static int uverbs_destroy_uobject(struct ib_uobject *uobj,
        return 0;
 }
 
+/*
+ * This calls uverbs_destroy_uobject() using the RDMA_REMOVE_DESTROY
+ * sequence. It should only be used from command callbacks. On success the
+ * caller must pair this with rdma_lookup_put_uobject(LOOKUP_WRITE). This
+ * version requires the caller to have already obtained an
+ * LOOKUP_DESTROY uobject kref.
+ */
+int uobj_destroy(struct ib_uobject *uobj)
+{
+       struct ib_uverbs_file *ufile = uobj->ufile;
+       int ret;
+
+       down_read(&ufile->hw_destroy_rwsem);
+
+       ret = uverbs_try_lock_object(uobj, UVERBS_LOOKUP_WRITE);
+       if (ret)
+               goto out_unlock;
+
+       ret = uverbs_destroy_uobject(uobj, RDMA_REMOVE_DESTROY);
+       if (ret) {
+               atomic_set(&uobj->usecnt, 0);
+               goto out_unlock;
+       }
+
+out_unlock:
+       up_read(&ufile->hw_destroy_rwsem);
+       return ret;
+}
+
 /*
  * uobj_get_destroy destroys the HW object and returns a handle to the uobj
  * with a NULL object pointer. The caller must pair this with
@@ -238,13 +271,13 @@ struct ib_uobject *__uobj_get_destroy(const struct uverbs_obj_type *type,
        struct ib_uobject *uobj;
        int ret;
 
-       uobj = rdma_lookup_get_uobject(type, ufile, id, UVERBS_LOOKUP_WRITE);
+       uobj = rdma_lookup_get_uobject(type, ufile, id, UVERBS_LOOKUP_DESTROY);
        if (IS_ERR(uobj))
                return uobj;
 
-       ret = rdma_explicit_destroy(uobj);
+       ret = uobj_destroy(uobj);
        if (ret) {
-               rdma_lookup_put_uobject(uobj, UVERBS_LOOKUP_WRITE);
+               rdma_lookup_put_uobject(uobj, UVERBS_LOOKUP_DESTROY);
                return ERR_PTR(ret);
        }
 
@@ -265,6 +298,11 @@ int __uobj_perform_destroy(const struct uverbs_obj_type *type, u32 id,
        if (IS_ERR(uobj))
                return PTR_ERR(uobj);
 
+       /*
+        * FIXME: After destroy this is not safe. We no longer hold the rwsem
+        * so disassociation could have completed and unloaded the module that
+        * backs the uobj->type pointer.
+        */
        rdma_lookup_put_uobject(uobj, UVERBS_LOOKUP_WRITE);
        return success_res;
 }
@@ -534,23 +572,6 @@ static int __must_check remove_commit_fd_uobject(struct ib_uobject *uobj,
        return 0;
 }
 
-int rdma_explicit_destroy(struct ib_uobject *uobject)
-{
-       int ret;
-       struct ib_uverbs_file *ufile = uobject->ufile;
-
-       /* Cleanup is running. Calling this should have been impossible */
-       if (!down_read_trylock(&ufile->hw_destroy_rwsem)) {
-               WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
-               return 0;
-       }
-
-       ret = uverbs_destroy_uobject(uobject, RDMA_REMOVE_DESTROY);
-
-       up_read(&ufile->hw_destroy_rwsem);
-       return ret;
-}
-
 static int alloc_commit_idr_uobject(struct ib_uobject *uobj)
 {
        struct ib_uverbs_file *ufile = uobj->ufile;
@@ -686,6 +707,8 @@ void rdma_lookup_put_uobject(struct ib_uobject *uobj,
        case UVERBS_LOOKUP_WRITE:
                atomic_set(&uobj->usecnt, 0);
                break;
+       case UVERBS_LOOKUP_DESTROY:
+               break;
        }
 
        /* Pairs with the kref obtained by type->lookup_get */
@@ -911,6 +934,9 @@ uverbs_get_uobject_from_file(const struct uverbs_obj_type *type_attrs,
                return rdma_lookup_get_uobject(type_attrs, ufile, id,
                                               UVERBS_LOOKUP_READ);
        case UVERBS_ACCESS_DESTROY:
+               /* Actual destruction is done inside uverbs_handle_method */
+               return rdma_lookup_get_uobject(type_attrs, ufile, id,
+                                              UVERBS_LOOKUP_DESTROY);
        case UVERBS_ACCESS_WRITE:
                return rdma_lookup_get_uobject(type_attrs, ufile, id,
                                               UVERBS_LOOKUP_WRITE);
@@ -942,7 +968,8 @@ int uverbs_finalize_object(struct ib_uobject *uobj,
                rdma_lookup_put_uobject(uobj, UVERBS_LOOKUP_WRITE);
                break;
        case UVERBS_ACCESS_DESTROY:
-               rdma_lookup_put_uobject(uobj, UVERBS_LOOKUP_WRITE);
+               if (uobj)
+                       rdma_lookup_put_uobject(uobj, UVERBS_LOOKUP_DESTROY);
                break;
        case UVERBS_ACCESS_NEW:
                if (commit)

diff --git a/drivers/infiniband/core/rdma_core.h b/drivers/infiniband/core/rdma_core.h
index a736b46d18e34cc299dfb7aea472b932dea6da1a..e4d8b985c311355c1907f4e0f30ceaef5903dae2 100644 (file)
--- a/drivers/infiniband/core/rdma_core.h
+++ b/drivers/infiniband/core/rdma_core.h
@@ -52,6 +52,8 @@ const struct uverbs_method_spec *uverbs_get_method(const struct uverbs_object_sp
 void uverbs_destroy_ufile_hw(struct ib_uverbs_file *ufile,
                             enum rdma_remove_reason reason);
 
+int uobj_destroy(struct ib_uobject *uobj);
+
 /*
  * uverbs_uobject_get is called in order to increase the reference count on
  * an uobject. This is useful when a handler wants to keep the uobject's memory

diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 404acfcdbeb230231b423c0704eef3c82cee1c68..f3776f909ca58b439380cf0090df352abf449f26 100644 (file)
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -349,13 +349,18 @@ static int uverbs_handle_method(struct ib_uverbs_attr __user *uattr_ptr,
         * not get to manipulate the HW objects.
         */
        if (destroy_attr) {
-               ret = rdma_explicit_destroy(destroy_attr->uobject);
+               ret = uobj_destroy(destroy_attr->uobject);
                if (ret)
                        goto cleanup;
        }
 
        ret = method_spec->handler(ibdev, ufile, attr_bundle);
 
+       if (destroy_attr) {
+               uobj_put_destroy(destroy_attr->uobject);
+               destroy_attr->uobject = NULL;
+       }
+
 cleanup:
        finalize_ret = uverbs_finalize_attrs(attr_bundle,
                                             method_spec->attr_buckets,

diff --git a/include/rdma/uverbs_types.h b/include/rdma/uverbs_types.h
index 0676672dbbb99589da38300d14b17d2b3432b9ea..f64f413cecac22279d85e896a3ea868f0f059e93 100644 (file)
--- a/include/rdma/uverbs_types.h
+++ b/include/rdma/uverbs_types.h
@@ -41,6 +41,12 @@ struct uverbs_obj_type;
 enum rdma_lookup_mode {
        UVERBS_LOOKUP_READ,
        UVERBS_LOOKUP_WRITE,
+       /*
+        * Destroy is like LOOKUP_WRITE, except that the uobject is not
+        * locked.  uobj_destroy is used to convert a LOOKUP_DESTROY lock into
+        * a LOOKUP_WRITE lock.
+        */
+       UVERBS_LOOKUP_DESTROY,
 };
 
 /*
@@ -129,7 +135,6 @@ struct ib_uobject *rdma_alloc_begin_uobject(const struct uverbs_obj_type *type,
                                            struct ib_uverbs_file *ufile);
 void rdma_alloc_abort_uobject(struct ib_uobject *uobj);
 int __must_check rdma_alloc_commit_uobject(struct ib_uobject *uobj);
-int rdma_explicit_destroy(struct ib_uobject *uobject);
 
 struct uverbs_obj_fd_type {
        /*