From: Keir Fraser <keir.fraser@citrix.com>
Date: Wed, 4 Nov 2009 18:13:32 +0000 (+0000)
Subject: xenbus: do not hold transaction_mutex when returning to userspace
X-Git-Tag: xen-3.4.2
X-Git-Url: http://xenbits.xensource.com/gitweb?a=commitdiff_plain;h=e30493b03bc28030af2d0d605abcc04e77b4abe7;p=legacy%2Flinux-2.6.18-xen.git

xenbus: do not hold transaction_mutex when returning to userspace

  ================================================
  [ BUG: lock held when returning to user space! ]
  ------------------------------------------------
  xenstore-list/3522 is leaving the kernel with locks still held!
  1 lock held by xenstore-list/3522:
   #0:  (&xs_state.transaction_mutex){......}, at: [<c026dc6f>]
   xenbus_dev_request_and_reply+0x8f/0xa0

The canonical fix for this type of issue appears to be to maintain a
count manually rather than using an rwsem so do that here.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index c51bbd03..99a8614c 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -84,6 +84,14 @@ struct xs_handle {
 	/*
 	 * Mutex ordering: transaction_mutex -> watch_mutex -> request_mutex.
 	 * response_mutex is never taken simultaneously with the other three.
+	 *
+	 * transaction_mutex must be held before incrementing
+	 * transaction_count. The mutex is held when a suspend is in
+	 * progress to prevent new transactions starting.
+	 *
+	 * When decrementing transaction_count to zero the wait queue
+	 * should be woken up, the suspend code waits for count to
+	 * reach zero.
 	 */
 
 	/* One request at a time. */
@@ -93,7 +101,9 @@ struct xs_handle {
 	struct mutex response_mutex;
 
 	/* Protect transactions against save/restore. */
-	struct rw_semaphore transaction_mutex;
+	struct mutex transaction_mutex;
+	atomic_t transaction_count;
+	wait_queue_head_t transaction_wq;
 
 	/* Protect watch (de)register against save/restore. */
 	struct rw_semaphore watch_mutex;
@@ -165,6 +175,31 @@ static void *read_reply(enum xsd_sockmsg_type *type, unsigned int *len)
 	return body;
 }
 
+static void transaction_start(void)
+{
+	mutex_lock(&xs_state.transaction_mutex);
+	atomic_inc(&xs_state.transaction_count);
+	mutex_unlock(&xs_state.transaction_mutex);
+}
+
+static void transaction_end(void)
+{
+	if (atomic_dec_and_test(&xs_state.transaction_count))
+		wake_up(&xs_state.transaction_wq);
+}
+
+static void transaction_suspend(void)
+{
+	mutex_lock(&xs_state.transaction_mutex);
+	wait_event(xs_state.transaction_wq,
+		   atomic_read(&xs_state.transaction_count) == 0);
+}
+
+static void transaction_resume(void)
+{
+	mutex_unlock(&xs_state.transaction_mutex);
+}
+
 void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg)
 {
 	void *ret;
@@ -172,7 +207,7 @@ void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg)
 	int err;
 
 	if (req_msg.type == XS_TRANSACTION_START)
-		down_read(&xs_state.transaction_mutex);
+		transaction_start();
 
 	mutex_lock(&xs_state.request_mutex);
 
@@ -188,7 +223,7 @@ void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg)
 	if ((req_msg.type == XS_TRANSACTION_END) ||
 	    ((req_msg.type == XS_TRANSACTION_START) &&
 	     (msg->type == XS_ERROR)))
-		up_read(&xs_state.transaction_mutex);
+		transaction_end();
 
 	return ret;
 }
@@ -440,11 +475,11 @@ int xenbus_transaction_start(struct xenbus_transaction *t)
 {
 	char *id_str;
 
-	down_read(&xs_state.transaction_mutex);
+	transaction_start();
 
 	id_str = xs_single(XBT_NIL, XS_TRANSACTION_START, "", NULL);
 	if (IS_ERR(id_str)) {
-		up_read(&xs_state.transaction_mutex);
+		transaction_end();
 		return PTR_ERR(id_str);
 	}
 
@@ -469,7 +504,7 @@ int xenbus_transaction_end(struct xenbus_transaction t, int abort)
 
 	err = xs_error(xs_single(t, XS_TRANSACTION_END, abortstr, NULL));
 
-	up_read(&xs_state.transaction_mutex);
+	transaction_end();
 
 	return err;
 }
@@ -670,7 +705,7 @@ EXPORT_SYMBOL_GPL(unregister_xenbus_watch);
 
 void xs_suspend(void)
 {
-	down_write(&xs_state.transaction_mutex);
+	transaction_suspend();
 	down_write(&xs_state.watch_mutex);
 	mutex_lock(&xs_state.request_mutex);
 	mutex_lock(&xs_state.response_mutex);
@@ -683,7 +718,7 @@ void xs_resume(void)
 
 	mutex_unlock(&xs_state.response_mutex);
 	mutex_unlock(&xs_state.request_mutex);
-	up_write(&xs_state.transaction_mutex);
+	transaction_resume();
 
 	/* No need for watches_lock: the watch_mutex is sufficient. */
 	list_for_each_entry(watch, &watches, list) {
@@ -699,7 +734,7 @@ void xs_suspend_cancel(void)
 	mutex_unlock(&xs_state.response_mutex);
 	mutex_unlock(&xs_state.request_mutex);
 	up_write(&xs_state.watch_mutex);
-	up_write(&xs_state.transaction_mutex);
+	mutex_unlock(&xs_state.transaction_mutex);
 }
 
 static int xenwatch_handle_callback(void *data)
@@ -880,8 +915,10 @@ int xs_init(void)
 
 	mutex_init(&xs_state.request_mutex);
 	mutex_init(&xs_state.response_mutex);
-	init_rwsem(&xs_state.transaction_mutex);
+	mutex_init(&xs_state.transaction_mutex);
 	init_rwsem(&xs_state.watch_mutex);
+	atomic_set(&xs_state.transaction_count, 0);
+	init_waitqueue_head(&xs_state.transaction_wq);
 
 	task = kthread_run(xenwatch_thread, NULL, "xenwatch");
 	if (IS_ERR(task))