path: root/kdbus-reduce-scope-of-handle-locking.patch

From: David Herrmann <dh.herrmann@gmail.com>
Date: Sat, 18 Apr 2015 12:39:51 +0200
Subject: [PATCH] kdbus: reduce scope of handle locking

A kdbus handle is used to create objects in the kdbus hierarchy. During
open(), we do not have enough information to know how to setup the object.
Therefore, we provide setup ioctls, which allow user-space to pass in
parameters and options how the to-be-created object should behave. Once
setup is done, we allow user-space to use ioctls to operate on that newly
created object.

It is important to notice:
  1) Only one setup ioctl can ever be called on a handle. You cannot call
     multiple, different setup ioctls on the same handle.
  2) A setup ioctl can only be called once, if it succeeded. If it failed,
     it must not modify the handle in any way. If it succeeded, no further
     setup ioctl can be issued.
  3) After a setup ioctl is done, the handle is constant and must not be
     modified in any way.

So far, we used a write-lock around all setup ioctls, and a read-lock
around everything else. The handle setup-indicator (the type field) can
only be set under the write-lock. Whenever you access the handle under a
read-lock, you must verify it was set before, otherwise, you must bail out
as the handle was not initialized, yet.

This has the downside that we need a read-lock on all operations on the
handle. For performance reasons, we should avoid that. This patch turns
the rwlock into a mutex and removes the read-side lock from all paths. It
relies on the 3 behaviors described above.

With this patch, the mutex is only taken around setup ioctls. Furthermore,
the setup-indicator (the type field) is only ever set if the mutex is
held. The mutex guarantees that multiple setup ioctls cannot race, and
also, that only one setup ioctl will ever succeed. If a setup ioctl is
called after setup was already finished, we do not touch the handle at all
and immediately fail.

Furthermore, all other operations (non-setup operations) can only be
called once setup is done. Therefore, we must synchronize them with any
racing setup, otherwise, they might access the handle which is currently
modified by setup.
We protect from this race by setting the setup-indicator (the type field)
_last_, and issue a write-barrier before setting it. Once it is set, we
never modify the handle ever again; it is constant from now on until
file-release.
Hence, on the read-side we simply read the type field and issue a
read-barrier afterwards. _Iff_ the type field was not set, yet, we must
not access the handle in any way, but bail out immediately. Setup was not
done, yet. But if the type field was set, the read-barrier pairs with the
write-barrier during setup. All member fields of the handle object are
guaranteed to be accessible by us, as the type-field is always the last
field that is written.

With this in place, we reduce the locking-overhead of all non-setup ioctls
to a read-barrier, instead of a read-side lock. And in combination with
the follow-up that removes the active-refs from kdbus_handle_poll(), we're
now lock-free in ->poll and ->mmap callbacks.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Daniel Mack <daniel@zonque.org>
---
 ipc/kdbus/handle.c | 110 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 83 insertions(+), 27 deletions(-)

diff --git a/ipc/kdbus/handle.c b/ipc/kdbus/handle.c
index 3f5d8085a297..a3e01383a6f6 100644
--- a/ipc/kdbus/handle.c
+++ b/ipc/kdbus/handle.c
@@ -18,6 +18,7 @@
 #include <linux/init.h>
 #include <linux/kdev_t.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/rwsem.h>
 #include <linux/sched.h>
@@ -229,7 +230,7 @@ enum kdbus_handle_type {
 
 /**
  * struct kdbus_handle - handle to the kdbus system
- * @rwlock:		handle lock
+ * @lock:		handle lock
  * @type:		type of this handle (KDBUS_HANDLE_*)
  * @bus_owner:		bus this handle owns
  * @ep_owner:		endpoint this handle owns
@@ -237,7 +238,7 @@ enum kdbus_handle_type {
  * @privileged:		Flag to mark a handle as privileged
  */
 struct kdbus_handle {
-	struct rw_semaphore rwlock;
+	struct mutex lock;
 
 	enum kdbus_handle_type type;
 	union {
@@ -265,7 +266,7 @@ static int kdbus_handle_open(struct inode *inode, struct file *file)
 		goto exit;
 	}
 
-	init_rwsem(&handle->rwlock);
+	mutex_init(&handle->lock);
 	handle->type = KDBUS_HANDLE_NONE;
 
 	if (node->type == KDBUS_NODE_ENDPOINT) {
@@ -355,8 +356,8 @@ static long kdbus_handle_ioctl_control(struct file *file, unsigned int cmd,
 			break;
 		}
 
-		handle->type = KDBUS_HANDLE_BUS_OWNER;
 		handle->bus_owner = bus;
+		ret = KDBUS_HANDLE_BUS_OWNER;
 		break;
 	}
 
@@ -396,8 +397,8 @@ static long kdbus_handle_ioctl_ep(struct file *file, unsigned int cmd,
 			break;
 		}
 
-		handle->type = KDBUS_HANDLE_EP_OWNER;
 		handle->ep_owner = ep;
+		ret = KDBUS_HANDLE_EP_OWNER;
 		break;
 
 	case KDBUS_CMD_HELLO:
@@ -407,8 +408,8 @@ static long kdbus_handle_ioctl_ep(struct file *file, unsigned int cmd,
 			break;
 		}
 
-		handle->type = KDBUS_HANDLE_CONNECTED;
 		handle->conn = conn;
+		ret = KDBUS_HANDLE_CONNECTED;
 		break;
 
 	default:
@@ -522,19 +523,41 @@ static long kdbus_handle_ioctl(struct file *file, unsigned int cmd,
 	case KDBUS_CMD_BUS_MAKE:
 	case KDBUS_CMD_ENDPOINT_MAKE:
 	case KDBUS_CMD_HELLO:
-		/* bail out early if already typed */
-		if (handle->type != KDBUS_HANDLE_NONE)
-			break;
-
-		down_write(&handle->rwlock);
+		mutex_lock(&handle->lock);
 		if (handle->type == KDBUS_HANDLE_NONE) {
 			if (node->type == KDBUS_NODE_CONTROL)
 				ret = kdbus_handle_ioctl_control(file, cmd,
 								 argp);
 			else if (node->type == KDBUS_NODE_ENDPOINT)
 				ret = kdbus_handle_ioctl_ep(file, cmd, argp);
+
+			if (ret > 0) {
+				/*
+				 * The data given via open() is not sufficient
+				 * to setup a kdbus handle. Hence, we require
+				 * the user to perform a setup ioctl. This setup
+				 * can only be performed once and defines the
+				 * type of the handle. The different setup
+				 * ioctls are locked against each other so they
+				 * cannot race. Once the handle type is set,
+				 * the type-dependent ioctls are enabled. To
+				 * improve performance, we don't lock those via
+				 * handle->lock. Instead, we issue a
+				 * write-barrier before performing the
+				 * type-change, which pairs with smp_rmb() in
+				 * all handlers that access the type field. This
+				 * guarantees the handle is fully setup, if
+				 * handle->type is set. If handle->type is
+				 * unset, you must not make any assumptions
+				 * without taking handle->lock.
+				 * Note that handle->type is only set once. It
+				 * will never change afterwards.
+				 */
+				smp_wmb();
+				handle->type = ret;
+			}
 		}
-		up_write(&handle->rwlock);
+		mutex_unlock(&handle->lock);
 		break;
 
 	case KDBUS_CMD_ENDPOINT_UPDATE:
@@ -549,14 +572,30 @@ static long kdbus_handle_ioctl(struct file *file, unsigned int cmd,
 	case KDBUS_CMD_MATCH_REMOVE:
 	case KDBUS_CMD_SEND:
 	case KDBUS_CMD_RECV:
-	case KDBUS_CMD_FREE:
-		down_read(&handle->rwlock);
-		if (handle->type == KDBUS_HANDLE_EP_OWNER)
+	case KDBUS_CMD_FREE: {
+		enum kdbus_handle_type type;
+
+		/*
+		 * This read-barrier pairs with smp_wmb() of the handle setup.
+		 * it guarantees the handle is fully written, in case the
+		 * type has been set. It allows us to access the handle without
+		 * taking handle->lock, given the guarantee that the type is
+		 * only ever set once, and stays constant afterwards.
+		 * Furthermore, the handle object itself is not modified in any
+		 * way after the type is set. That is, the type-field is the
+		 * last field that is written on any handle. If it has not been
+		 * set, we must not access the handle here.
+		 */
+		type = handle->type;
+		smp_rmb();
+
+		if (type == KDBUS_HANDLE_EP_OWNER)
 			ret = kdbus_handle_ioctl_ep_owner(file, cmd, argp);
-		else if (handle->type == KDBUS_HANDLE_CONNECTED)
+		else if (type == KDBUS_HANDLE_CONNECTED)
 			ret = kdbus_handle_ioctl_connected(file, cmd, argp);
-		up_read(&handle->rwlock);
+
 		break;
+	}
 	default:
 		ret = -ENOTTY;
 		break;
@@ -569,16 +608,23 @@ static unsigned int kdbus_handle_poll(struct file *file,
 				      struct poll_table_struct *wait)
 {
 	struct kdbus_handle *handle = file->private_data;
+	enum kdbus_handle_type type;
 	unsigned int mask = POLLOUT | POLLWRNORM;
 	int ret;
 
+	/*
+	 * This pairs with smp_wmb() during handle setup. It guarantees that
+	 * _iff_ the handle type is set, handle->conn is valid. Furthermore,
+	 * _iff_ the type is set, the handle object is constant and never
+	 * changed again. If it's not set, we must not access the handle but
+	 * bail out. We also must assume no setup has taken place, yet.
+	 */
+	type = handle->type;
+	smp_rmb();
+
 	/* Only a connected endpoint can read/write data */
-	down_read(&handle->rwlock);
-	if (handle->type != KDBUS_HANDLE_CONNECTED) {
-		up_read(&handle->rwlock);
+	if (type != KDBUS_HANDLE_CONNECTED)
 		return POLLERR | POLLHUP;
-	}
-	up_read(&handle->rwlock);
 
 	ret = kdbus_conn_acquire(handle->conn);
 	if (ret < 0)
@@ -598,13 +644,23 @@ static unsigned int kdbus_handle_poll(struct file *file,
 static int kdbus_handle_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct kdbus_handle *handle = file->private_data;
+	enum kdbus_handle_type type;
 	int ret = -EBADFD;
 
-	if (down_read_trylock(&handle->rwlock)) {
-		if (handle->type == KDBUS_HANDLE_CONNECTED)
-			ret = kdbus_pool_mmap(handle->conn->pool, vma);
-		up_read(&handle->rwlock);
-	}
+	/*
+	 * This pairs with smp_wmb() during handle setup. It guarantees that
+	 * _iff_ the handle type is set, handle->conn is valid. Furthermore,
+	 * _iff_ the type is set, the handle object is constant and never
+	 * changed again. If it's not set, we must not access the handle but
+	 * bail out. We also must assume no setup has taken place, yet.
+	 */
+	type = handle->type;
+	smp_rmb();
+
+	/* Only connected handles have a pool we can map */
+	if (type == KDBUS_HANDLE_CONNECTED)
+		ret = kdbus_pool_mmap(handle->conn->pool, vma);
+
 	return ret;
 }