bus: make conn_rwlock a low-level lock

conn_rwlock protects the connection lists on a bus. Those lists are
usually only accessed deep down in our call-paths, so we can safely order
conn_rwlock _after_ bus->lock and ep->lock. We can even order it after
registry->lock and thus fix a dead-lock in list_names where we used to
have:
    down_read(&bus->conn_rwlock);
    down_read(&reg->rwlock);

.. which dead-locks against kmsg_send():
    kdbus_name_lock(reg); (=> down_read(&reg->rwlock))
    down_read(&bus->conn_rwlock);

The new lock-order isn't particularly beautiful, but there's currently no
way around it. We have to lock destination names on kmsg_send() to make
sure an activator does not get activated concurrently. We could lock
conn_rwlock in kmsg_send() early, but this is kinda ugly regarding
kdbus_conn_wait_reply(). Therefore, for now, lock conn_rwlock late. We can
always change the lock order again later.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

diff --git a/connection.c b/connection.c
index 2dc9b162f2e317467332701d07a742bbbc279e68..8210d1d9ca95a85d553cb7bb9dd2b2a96e16aa8c 100644 (file)
--- a/connection.c
+++ b/connection.c
@@ -932,16 +932,16 @@ int kdbus_conn_disconnect(struct kdbus_conn *conn, bool ensure_queue_empty)
        cancel_delayed_work_sync(&conn->work);
 
        /* lock order: domain -> bus -> ep -> names -> conn */
-       down_write(&conn->bus->conn_rwlock);
        mutex_lock(&conn->ep->lock);
+       down_write(&conn->bus->conn_rwlock);
 
        /* remove from bus and endpoint */
        hash_del(&conn->hentry);
        list_del(&conn->monitor_entry);
        list_del(&conn->ep_entry);
 
-       mutex_unlock(&conn->ep->lock);
        up_write(&conn->bus->conn_rwlock);
+       mutex_unlock(&conn->ep->lock);
 
        /*
         * Remove all names associated with this connection; this possibly
@@ -1606,9 +1606,9 @@ int kdbus_conn_new(struct kdbus_ep *ep,
        }
 
        /* lock order: domain -> bus -> ep -> names -> conn */
-       down_write(&bus->conn_rwlock);
        mutex_lock(&bus->lock);
        mutex_lock(&ep->lock);
+       down_write(&bus->conn_rwlock);
 
        if (bus->disconnected || ep->disconnected) {
                ret = -ESHUTDOWN;
@@ -1626,9 +1626,9 @@ int kdbus_conn_new(struct kdbus_ep *ep,
        list_add_tail(&conn->ep_entry, &ep->conn_list);
        hash_add(bus->conn_hash, &conn->hentry, conn->id);
 
+       up_write(&bus->conn_rwlock);
        mutex_unlock(&ep->lock);
        mutex_unlock(&bus->lock);
-       up_write(&bus->conn_rwlock);
 
        /* notify subscribers about the new active connection */
        ret = kdbus_notify_id_change(conn->bus, KDBUS_ITEM_ID_ADD,
@@ -1644,9 +1644,9 @@ int kdbus_conn_new(struct kdbus_ep *ep,
        return 0;
 
 exit_unref_user_unlock:
+       up_write(&bus->conn_rwlock);
        mutex_unlock(&ep->lock);
        mutex_unlock(&bus->lock);
-       up_write(&bus->conn_rwlock);
 exit_domain_user_unref:
        kdbus_domain_user_unref(conn->user);
 exit_free_meta:

diff --git a/names.c b/names.c
index 917d66cfb541b43a22687275b3bc6ccf5aeca6b4..e0a7760c992c0241da70cdcb994148e61325ffa4 100644 (file)
--- a/names.c
+++ b/names.c
@@ -872,8 +872,8 @@ int kdbus_cmd_name_list(struct kdbus_name_registry *reg,
        policy_db = &conn->ep->policy_db;
 
        /* lock order: domain -> bus -> ep -> names -> conn */
-       down_read(&conn->bus->conn_rwlock);
        down_read(&reg->rwlock);
+       down_read(&conn->bus->conn_rwlock);
        down_read(&policy_db->entries_rwlock);
 
        /* size of header + records */
@@ -907,7 +907,7 @@ exit_pool_free:
                kdbus_pool_slice_free(slice);
 exit_unlock:
        up_read(&policy_db->entries_rwlock);
-       up_read(&reg->rwlock);
        up_read(&conn->bus->conn_rwlock);
+       up_read(&reg->rwlock);
        return ret;
 }