From 4ece3b3c77d4e169e5e0dae0acbf135fdd1ec4b3 Mon Sep 17 00:00:00 2001
From: Alexander Boettcher <alexander.boettcher@genode-labs.com>
Date: Tue, 3 Jul 2012 14:45:52 +0200
Subject: [PATCH] Deadlock fix in rm_session on NOVA platform

Rm_client is derived from Pager_object. If the Pager_object is also
derived from Thread_base (which is the case for NOVA) then the
Rm_client object must be destructed without holding the rm_session_object
lock. The native platform specific Thread_base implementation has to take
care that all in-flight page handling requests are finished before
destruction. On NOVA it is done by doing an IPC to the pager thread.
(performed in Pager_object::dissolve() in base-nova). The
called thread than executes its operation until end which also requires
in some cases to take the rm_session_object lock.

Since _client_slab insertion/deletion also must be performed
synchronized but can't be protected by the rm_session_object lock
because of the described dead_lock situation, we have
to use a synchronized allocator object to perform insertion and
deletion of Rm_clients.
---
 base/src/core/include/rm_session_component.h |  6 ++--
 base/src/core/rm_session_component.cc        | 31 +++++++++++++++-----
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/base/src/core/include/rm_session_component.h b/base/src/core/include/rm_session_component.h
index e24f7aa7a0..5e6df370b4 100644
--- a/base/src/core/include/rm_session_component.h
+++ b/base/src/core/include/rm_session_component.h
@@ -22,6 +22,7 @@
 #include <base/pager.h>
 #include <base/allocator_avl.h>
 #include <base/allocator_guard.h>
+#include <base/sync_allocator.h>
 #include <base/signal.h>
 #include <base/rpc_server.h>
 #include <util/list.h>
@@ -258,8 +259,9 @@ namespace Genode {
 			};
 
 
-			Tslab<Rm_client, 1024>        _client_slab;  /* backing store for
-			                                                client structures */
+			typedef Synchronized_allocator<Tslab<Rm_client, 1024> > Client_slab_alloc;
+			Client_slab_alloc              _client_slab; /* backing store for
+			                                                client structures, synchronized */
 			Tslab<Rm_region_ref, 1024>    _ref_slab;     /* backing store for
 			                                                region list */
 			Allocator_avl_tpl<Rm_region>  _map;          /* region map for attach,
diff --git a/base/src/core/rm_session_component.cc b/base/src/core/rm_session_component.cc
index 340dd87eaa..d86c43ac66 100644
--- a/base/src/core/rm_session_component.cc
+++ b/base/src/core/rm_session_component.cc
@@ -683,9 +683,28 @@ Rm_session::State Rm_session_component::state()
 
 void Rm_session_component::dissolve(Rm_client *cl)
 {
-	Lock::Guard lock_guard(_lock);
-	_pager_ep->dissolve(cl);
-	_clients.remove(cl);
+	{
+		Lock::Guard lock_guard(_lock);
+		_pager_ep->dissolve(cl);
+		_clients.remove(cl);
+	}
+
+	/*
+	 * Rm_client is derived from Pager_object. If the Pager_object is also
+	 * derived from Thread_base then the Rm_client object must be
+	 * destructed without holding the rm_session_object lock. The native
+	 * platform specific Thread_base implementation has to take care that
+	 * all in-flight page handling requests are finished before
+	 * destruction. (Either by waiting until the end of or by
+	 * <deadlock free> cancellation of the last in-flight request.
+	 * This operation can also require taking the rm_session_object lock.
+	 *
+	 * Since _client_slab insertion/deletion also must be performed
+	 * synchronized but can't be protected by the rm_session_object lock
+	 * because of the described potential dead_lock situation, we have
+	 * to use a synchronized allocator object to perform insertion and 
+	 * deletion of Rm_clients.
+	 */
 	destroy(&_client_slab, cl);
 }
 
@@ -728,13 +747,11 @@ Rm_session_component::~Rm_session_component()
 	}
 
 	/* remove all clients */
-	while (Rm_client *cl = _client_slab.first_object()) {
-		_pager_ep->dissolve(cl);
+	while (Rm_client *cl = _client_slab.raw()->first_object()) {
 		_lock.unlock();
 		cl->dissolve_from_faulting_rm_session();
+		this->dissolve(cl);
 		_lock.lock();
-		_clients.remove(cl);
-		destroy(&_client_slab, cl);
 	}
 
 	/* detach all regions */