From: David Herrmann <dh.herrmann@gmail.com>
Date: Mon, 20 Oct 2014 13:24:15 +0000 (+0200)
Subject: bus: make conn_rwlock a low-level lock
X-Git-Tag: upstream/0.20141102.012929utc~79
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=9525de196e533b76e5f7d935a60e1b6481269d45;p=platform%2Fcore%2Fsystem%2Fkdbus-bus.git

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 2dc9b16..8210d1d 100644
--- 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 917d66c..e0a7760 100644
--- 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;
 }