a larger set of locking reconsiderations

In particular, kdbus_bus_find_conn_by_id() must always be called with
bus->lock held, and it refs the found connection. Consequently, all
users of that function have to unref the returned connection as well.

7 files changed, 124 insertions, 46 deletions

diff --git a/bus.c b/bus.c
index 293e968e1de..58035a90366 100644
--- a/bus.c
+++ b/bus.c
@@ -77,15 +77,28 @@ void kdbus_bus_unref(struct kdbus_bus *bus)
 	kref_put(&bus->kref, __kdbus_bus_free);
 }
 
+/**
+ * kdbus_bus_find_conn_by_id - find a connection with a given id
+ * @bus:	The bus to look for the connection
+ * @id:		The 64-bit connection id
+ *
+ * Looks up a connection with a given id. The returned connection
+ * is ref'ed, and needs to be unref'ed by the user. Returns NULL if
+ * the connection can't be found.
+ *
+ * This function must be called with bus->lock held.
+ */
 struct kdbus_conn *kdbus_bus_find_conn_by_id(struct kdbus_bus *bus, u64 id)
 {
-	struct kdbus_conn *conn;
+	struct kdbus_conn *conn, *found = NULL;
 
 	hash_for_each_possible(bus->conn_hash, conn, hentry, id)
-		if (conn->id == id)
-			return conn;
+		if (conn->id == id) {
+			found = kdbus_conn_ref(conn);
+			break;
+		}
 
-	return NULL;
+	return found;
 }
 
 /**
diff --git a/connection.c b/connection.c
index d55de245003..ebb3700d6da 100644
--- a/connection.c
+++ b/connection.c
@@ -522,14 +522,14 @@ static int kdbus_conn_get_conn_dst(struct kdbus_bus *bus,
 		}
 
 		if (name_entry->starter)
-			c = name_entry->starter;
+			c = kdbus_conn_ref(name_entry->starter);
 		else
-			c = name_entry->conn;
+			c = kdbus_conn_ref(name_entry->conn);
 
 		if ((msg->flags & KDBUS_MSG_FLAGS_NO_AUTO_START) &&
 		    (c->flags & KDBUS_HELLO_STARTER)) {
 			ret = -EADDRNOTAVAIL;
-			goto exit_unlock;
+			goto exit_unref;
 		}
 	} else {
 		c = kdbus_bus_find_conn_by_id(bus, msg->dst_id);
@@ -544,7 +544,7 @@ static int kdbus_conn_get_conn_dst(struct kdbus_bus *bus,
 		 */
 		if (c->flags & KDBUS_HELLO_STARTER) {
 			ret = -ENXIO;
-			goto exit_unlock;
+			goto exit_unref;
 		}
 	}
 
@@ -554,12 +554,19 @@ static int kdbus_conn_get_conn_dst(struct kdbus_bus *bus,
 
 	if (disconnected) {
 		ret = -ESRCH;
-		goto exit_unlock;
+		goto exit_unref;
 	}
 
-	kdbus_conn_ref(c);
+	/* the connection is already ref'ed at this point */
 	*conn = c;
 
+	/* nullify it so it won't be freed below */
+	c = NULL;
+
+exit_unref:
+	if (c)
+		kdbus_conn_unref(c);
+
 exit_unlock:
 	mutex_unlock(&bus->lock);
 	return ret;
@@ -655,7 +662,9 @@ int kdbus_conn_kmsg_send(struct kdbus_ep *ep,
 		kdbus_conn_timeout_schedule_scan(conn_dst);
 
 exit:
+	/* conn_dst got an extra ref from kdbus_conn_get_conn_dst */
 	kdbus_conn_unref(conn_dst);
+
 	return ret;
 }
 
@@ -1001,8 +1010,11 @@ int kdbus_cmd_conn_info(struct kdbus_name_registry *reg,
 
 	/* The API offers to look up a connection by ID or by name */
 	if (cmd_info->id != 0) {
-		owner_conn = kdbus_bus_find_conn_by_id(conn->ep->bus,
-						       cmd_info->id);
+		struct kdbus_bus *bus = conn->ep->bus;
+
+		mutex_lock(&bus->lock);
+		owner_conn = kdbus_bus_find_conn_by_id(bus, cmd_info->id);
+		mutex_unlock(&bus->lock);
 	} else {
 		if (size == sizeof(struct kdbus_cmd_conn_info)) {
 			ret = -EINVAL;
@@ -1030,7 +1042,8 @@ int kdbus_cmd_conn_info(struct kdbus_name_registry *reg,
 			goto exit_free;
 		}
 
-		owner_conn = e->conn;
+		if (e->conn)
+			owner_conn = kdbus_conn_ref(e->conn);
 	}
 
 	if (!owner_conn) {
@@ -1056,7 +1069,7 @@ int kdbus_cmd_conn_info(struct kdbus_name_registry *reg,
 
 	ret = kdbus_pool_alloc(conn->pool, info.size, &off);
 	if (ret < 0)
-		goto exit_unlock;
+		goto exit_unref_owner_conn;
 
 	ret = kdbus_pool_write(conn->pool, off, &info, sizeof(info));
 	if (ret < 0)
@@ -1101,7 +1114,8 @@ exit_free:
 	if (ret < 0)
 		kdbus_pool_free(conn->pool, off);
 
-exit_unlock:
+exit_unref_owner_conn:
+	kdbus_conn_unref(owner_conn);
 	mutex_unlock(&conn->names_lock);
 	kfree(cmd_info);
 
diff --git a/handle.c b/handle.c
index 5597e5a58ef..e0de0c87214 100644
--- a/handle.c
+++ b/handle.c
@@ -469,7 +469,7 @@ static long kdbus_handle_ioctl_ep_connected(struct file *file, unsigned int cmd,
 	case KDBUS_CMD_MONITOR: {
 		/* turn on/turn off monitor mode */
 		struct kdbus_cmd_monitor cmd_monitor;
-		struct kdbus_conn *mconn = conn;
+		struct kdbus_conn *mconn = NULL;
 
 		if (!KDBUS_IS_ALIGNED8((uintptr_t)buf)) {
 			ret = -EFAULT;
@@ -482,27 +482,33 @@ static long kdbus_handle_ioctl_ep_connected(struct file *file, unsigned int cmd,
 		}
 
 		/* privileged users can act on behalf of someone else */
-		if (cmd_monitor.id == 0) {
-			mconn = conn;
-		} else if (cmd_monitor.id != conn->id) {
+		if (cmd_monitor.id == 0 || cmd_monitor.id == conn->id) {
+			mconn = kdbus_conn_ref(conn);
+		} else {
 			if (!kdbus_bus_uid_is_privileged(bus)) {
 				ret = -EPERM;
 				break;
 			}
 
+			mutex_lock(&bus->lock);
 			mconn = kdbus_bus_find_conn_by_id(bus, cmd_monitor.id);
-			if (!mconn) {
-				ret = -ENXIO;
-				break;
-			}
+			mutex_unlock(&bus->lock);
+		}
+
+		if (!mconn) {
+			ret = -ENXIO;
+			break;
 		}
 
 		mutex_lock(&bus->lock);
-		if (cmd_monitor.flags && KDBUS_MONITOR_ENABLE)
+		if (cmd_monitor.flags & KDBUS_MONITOR_ENABLE)
 			list_add_tail(&mconn->monitor_entry, &bus->monitors_list);
 		else
 			list_del(&mconn->monitor_entry);
 		mutex_unlock(&bus->lock);
+
+		kdbus_conn_unref(mconn);
+
 		break;
 	}
 
diff --git a/match.c b/match.c
index eb954af4583..d653b39219b 100644
--- a/match.c
+++ b/match.c
@@ -101,6 +101,12 @@ void kdbus_match_db_unref(struct kdbus_match_db *db)
 	kref_put(&db->kref, __kdbus_match_db_free);
 }
 
+struct kdbus_match_db *kdbus_match_db_ref(struct kdbus_match_db *db)
+{
+	kref_get(&db->kref);
+	return db;
+}
+
 int kdbus_match_db_new(struct kdbus_match_db **db)
 {
 	struct kdbus_match_db *d;
@@ -318,17 +324,21 @@ int kdbus_match_db_add(struct kdbus_conn *conn, void __user *buf)
 
 	if (cmd_match->id != 0 && cmd_match->id != conn->id) {
 		struct kdbus_conn *targ_conn;
+		struct kdbus_bus *bus = conn->ep->bus;
+
+		mutex_lock(&bus->lock);
+		targ_conn = kdbus_bus_find_conn_by_id(bus, cmd_match->id);
+		mutex_unlock(&bus->lock);
 
-		targ_conn = kdbus_bus_find_conn_by_id(conn->ep->bus,
-						      cmd_match->id);
-		if (targ_conn)
-			db = targ_conn->match_db;
-		else {
+		if (!targ_conn) {
 			ret = -ENXIO;
 			goto exit_free;
 		}
+
+		db = kdbus_match_db_ref(targ_conn->match_db);
+		kdbus_conn_unref(targ_conn);
 	} else
-		db = conn->match_db;
+		db = kdbus_match_db_ref(conn->match_db);
 
 	e = kzalloc(sizeof(*e), GFP_KERNEL);
 	if (!e) {
@@ -406,6 +416,7 @@ int kdbus_match_db_add(struct kdbus_conn *conn, void __user *buf)
 		kdbus_match_db_entry_free(e);
 
 	mutex_unlock(&db->entries_lock);
+	kdbus_match_db_unref(db);
 
 exit_free:
 	kfree(cmd_match);
@@ -425,17 +436,22 @@ int kdbus_match_db_remove(struct kdbus_conn *conn, void __user *buf)
 
 	if (cmd_match->id != 0 && cmd_match->id != conn->id) {
 		struct kdbus_conn *targ_conn;
+		struct kdbus_bus *bus = conn->ep->bus;
+
+		mutex_lock(&bus->lock);
+		targ_conn = kdbus_bus_find_conn_by_id(bus, cmd_match->id);
+		mutex_unlock(&bus->lock);
 
-		targ_conn = kdbus_bus_find_conn_by_id(conn->ep->bus,
-						      cmd_match->id);
-		if (targ_conn)
-			db = targ_conn->match_db;
-		else {
+		if (targ_conn) {
+			db = kdbus_match_db_ref(targ_conn->match_db);
+			kdbus_conn_unref(targ_conn);
+		} else {
 			kfree(cmd_match);
 			return -ENXIO;
 		}
-	} else
-		db = conn->match_db;
+	} else {
+		db = kdbus_match_db_ref(conn->match_db);
+	}
 
 	mutex_lock(&db->entries_lock);
 	list_for_each_entry_safe(e, tmp, &db->entries, list_entry)
@@ -444,6 +460,7 @@ int kdbus_match_db_remove(struct kdbus_conn *conn, void __user *buf)
 			kdbus_match_db_entry_free(e);
 	mutex_unlock(&db->entries_lock);
 
+	kdbus_match_db_unref(db);
 	kfree(cmd_match);
 
 	return 0;
diff --git a/match.h b/match.h
index c9922f7d4e8..1ff23111965 100644
--- a/match.h
+++ b/match.h
@@ -24,6 +24,7 @@ struct kdbus_conn;
 struct kdbus_kmsg;
 
 int kdbus_match_db_new(struct kdbus_match_db **db);
+struct kdbus_match_db *kdbus_match_db_ref(struct kdbus_match_db *db);
 void kdbus_match_db_unref(struct kdbus_match_db *db);
 int kdbus_match_db_add(struct kdbus_conn *conn, void __user *buf);
 int kdbus_match_db_remove(struct kdbus_conn *conn, void __user *buf);
diff --git a/names.c b/names.c
index a35d991011f..3bd5451f68e 100644
--- a/names.c
+++ b/names.c
@@ -467,22 +467,27 @@ int kdbus_cmd_name_acquire(struct kdbus_name_registry *reg,
 	}
 
 	/* privileged users can act on behalf of someone else */
-	if (cmd_name->id > 0) {
+	if (cmd_name->id != 0) {
 		struct kdbus_conn *new_conn;
+		struct kdbus_bus *bus = conn->ep->bus;
 
-		if (!kdbus_bus_uid_is_privileged(conn->ep->bus)) {
+		if (!kdbus_bus_uid_is_privileged(bus)) {
 			ret = -EPERM;
 			goto exit_free;
 		}
 
-		new_conn = kdbus_bus_find_conn_by_id(conn->ep->bus,
-						     cmd_name->id);
+		mutex_lock(&bus->lock);
+		new_conn = kdbus_bus_find_conn_by_id(bus, cmd_name->id);
+		mutex_unlock(&bus->lock);
+
 		if (!new_conn) {
 			ret = -ENXIO;
 			goto exit_free;
 		}
 
 		conn = new_conn;
+	} else {
+		kdbus_conn_ref(conn);
 	}
 
 	cmd_name->flags &= ~KDBUS_NAME_IN_QUEUE;
@@ -492,19 +497,22 @@ int kdbus_cmd_name_acquire(struct kdbus_name_registry *reg,
 		ret = kdbus_policy_db_check_own_access(conn->ep->policy_db,
 						       conn, cmd_name->name);
 		if (ret < 0)
-			goto exit_free;
+			goto exit_unref_conn;
 	}
 
 	ret = kdbus_name_acquire(reg, conn, cmd_name->name,
 				 cmd_name->flags, &e);
 	if (ret < 0)
-		goto exit_free;
+		goto exit_unref_conn;
 
 	if (copy_to_user(buf, cmd_name, size)) {
 		ret = -EFAULT;
 		kdbus_name_entry_release(e);
 	}
 
+exit_unref_conn:
+	kdbus_conn_unref(conn);
+
 exit_free:
 	kfree(cmd_name);
 
@@ -556,14 +564,27 @@ int kdbus_cmd_name_release(struct kdbus_name_registry *reg,
 
 	/* privileged users can act on behalf of someone else */
 	if (e->conn != conn) {
-		if (!kdbus_bus_uid_is_privileged(conn->ep->bus)) {
+		struct kdbus_bus *bus = conn->ep->bus;
+
+		if (!kdbus_bus_uid_is_privileged(bus)) {
 			ret = -EPERM;
 			goto exit_unlock;
 		}
-		conn = kdbus_bus_find_conn_by_id(conn->ep->bus, cmd_name->id);
+
+		mutex_lock(&bus->lock);
+		conn = kdbus_bus_find_conn_by_id(bus, cmd_name->id);
+		mutex_unlock(&bus->lock);
+
+		if (!conn) {
+			ret = -EPERM;
+			goto exit_unlock;
+		}
+	} else {
+		kdbus_conn_ref(conn);
 	}
 
 	ret = kdbus_name_release(e, conn);
+	kdbus_conn_unref(conn);
 
 exit_unlock:
 	mutex_unlock(&reg->entries_lock);
diff --git a/notify.c b/notify.c
index f1b8cc2afc7..12c1dd0fffb 100644
--- a/notify.c
+++ b/notify.c
@@ -36,13 +36,16 @@ static int kdbus_notify_reply(struct kdbus_ep *ep, u64 src_id,
 	struct kdbus_item *item;
 	int ret;
 
+	mutex_lock(&ep->bus->lock);
 	dst_conn = kdbus_bus_find_conn_by_id(ep->bus, src_id);
+	mutex_unlock(&ep->bus->lock);
+
 	if (!dst_conn)
 		return -ENXIO;
 
 	ret = kdbus_kmsg_new(KDBUS_ITEM_SIZE(0), &kmsg);
 	if (ret < 0)
-		return ret;
+		goto exit_unref_conn;
 
 	/*
 	 * a kernel-generated notification can only contain one
@@ -62,6 +65,9 @@ static int kdbus_notify_reply(struct kdbus_ep *ep, u64 src_id,
 	ret = kdbus_conn_kmsg_send(ep, NULL, kmsg);
 	kdbus_kmsg_free(kmsg);
 
+exit_unref_conn:
+	kdbus_conn_unref(dst_conn);
+
 	return ret;
 }