Fix deadlock in trap_manager_t during acquire.

Also fixes a TOCTOU issue regarding the use of entry_t.pending.

The deadlock was caused because the rwlock was being locked while
waiting for an IKE_SA. Triggering the deadlock was a bit tricky, here
is the description by Thomas Egerer (the reporter of this issue):

"
The deadlock occurs when the following happens (in the given order):

a) an IKE_SA is built and a thread is processing the IKE_AUTH request,
   which can take a bit longer when a smartcard is involved. This
   causes the ike_sa_manager to lock a particular IKE_SA exclusively.
b) an acquire is triggered which causes the rwlock in the trap_manager
   to be read-locked, the subsequent call to
   ike_sa_manager->checkout_by_config has to wait until a) unlocks
   it's ike_sa.
c) a child_cfg contained in the peer_cfg belonging to the ike_sa
   a) has locked is routed causes the child_configs contained
   in the peer config to be locked by c) while the actual routing
   code within trap_manager tries to writelock it's rwlock.

That's about it. As soon as a) finishes authentication of the peer
and tries to find a matching child sa it will try to lock the child
configs of the peer config which is not possible since it has been
locked by c).

Thread | Resource locked                | Resource desired
-------+--------------------------------+--------------------------------
  (a)  | ike_sa in ike_sa_manager       | child_cfgs of peer_cfg
       |                                |
  (b)  | rwlock in trap-manager (read)  | ike_sa in ike_sa_manager
       |                                |
  (c)  | child_cfgs of peer_cfg         | rwlock in trap-manager (write)
"

With this patch thread (b) now does not hold the lock while waiting for
the IKE_SA. Thus (c) can get the write lock, and (a) can subsequently
lock the mutex in the peer_cfg which then finally allows (b) to checkout
the IKE_SA.

1 files changed, 43 insertions, 28 deletions

diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c
index c7a8a6e0c..86d9f4c22 100644
--- a/src/libcharon/sa/trap_manager.c
+++ b/src/libcharon/sa/trap_manager.c
@@ -1,4 +1,5 @@
 /*
+ * Copyright (C) 2011 Tobias Brunner
  * Copyright (C) 2009 Martin Willi
  * Hochschule fuer Technik Rapperswil
  *
@@ -74,8 +75,10 @@ typedef struct {
 	peer_cfg_t *peer_cfg;
 	/** ref to instanciated CHILD_SA */
 	child_sa_t *child_sa;
+	/** TRUE if an acquire is pending */
+	bool pending;
 	/** pending IKE_SA connecting upon acquire */
-	ike_sa_t *pending;
+	ike_sa_t *ike_sa;
 } entry_t;
 
 /**
@@ -170,10 +173,10 @@ METHOD(trap_manager_t, install, u_int32_t,
 	}
 
 	reqid = child_sa->get_reqid(child_sa);
-	entry = malloc_thing(entry_t);
-	entry->child_sa = child_sa;
-	entry->peer_cfg = peer->get_ref(peer);
-	entry->pending = NULL;
+	INIT(entry,
+		.child_sa = child_sa,
+		.peer_cfg = peer->get_ref(peer),
+	);
 
 	this->lock->write_lock(this->lock);
 	this->traps->insert_last(this->traps, entry);
@@ -263,35 +266,46 @@ METHOD(trap_manager_t, acquire, void,
 	if (!found)
 	{
 		DBG1(DBG_CFG, "trap not found, unable to acquire reqid %d",reqid);
+		this->lock->unlock(this->lock);
+		return;
 	}
-	else if (found->pending)
+	if (!cas_bool(&found->pending, FALSE, TRUE))
 	{
 		DBG1(DBG_CFG, "ignoring acquire, connection attempt pending");
+		this->lock->unlock(this->lock);
+		return;
 	}
-	else
+	peer = found->peer_cfg->get_ref(found->peer_cfg);
+	child = found->child_sa->get_config(found->child_sa);
+	child = child->get_ref(child);
+	reqid = found->child_sa->get_reqid(found->child_sa);
+	/* don't hold the lock while checking out the IKE_SA */
+	this->lock->unlock(this->lock);
+
+	ike_sa = charon->ike_sa_manager->checkout_by_config(
+											charon->ike_sa_manager, peer);
+	if (ike_sa->get_peer_cfg(ike_sa) == NULL)
 	{
-		child = found->child_sa->get_config(found->child_sa);
-		peer = found->peer_cfg;
-		ike_sa = charon->ike_sa_manager->checkout_by_config(
-												charon->ike_sa_manager, peer);
-		if (ike_sa->get_peer_cfg(ike_sa) == NULL)
-		{
-			ike_sa->set_peer_cfg(ike_sa, peer);
-		}
-		child->get_ref(child);
-		reqid = found->child_sa->get_reqid(found->child_sa);
-		if (ike_sa->initiate(ike_sa, child, reqid, src, dst) != DESTROY_ME)
-		{
-			found->pending = ike_sa;
-			charon->ike_sa_manager->checkin(charon->ike_sa_manager, ike_sa);
-		}
-		else
+		ike_sa->set_peer_cfg(ike_sa, peer);
+	}
+	if (ike_sa->initiate(ike_sa, child, reqid, src, dst) != DESTROY_ME)
+	{
+		/* make sure the entry is still there */
+		this->lock->read_lock(this->lock);
+		if (this->traps->find_first(this->traps, NULL,
+									(void**)&found) == SUCCESS)
 		{
-			charon->ike_sa_manager->checkin_and_destroy(
-												charon->ike_sa_manager, ike_sa);
+			found->ike_sa = ike_sa;
 		}
+		this->lock->unlock(this->lock);
+		charon->ike_sa_manager->checkin(charon->ike_sa_manager, ike_sa);
 	}
-	this->lock->unlock(this->lock);
+	else
+	{
+		charon->ike_sa_manager->checkin_and_destroy(
+											charon->ike_sa_manager, ike_sa);
+	}
+	peer->destroy(peer);
 }
 
 /**
@@ -307,7 +321,7 @@ static void complete(private_trap_manager_t *this, ike_sa_t *ike_sa,
 	enumerator = this->traps->create_enumerator(this->traps);
 	while (enumerator->enumerate(enumerator, &entry))
 	{
-		if (entry->pending != ike_sa)
+		if (entry->ike_sa != ike_sa)
 		{
 			continue;
 		}
@@ -316,7 +330,8 @@ static void complete(private_trap_manager_t *this, ike_sa_t *ike_sa,
 		{
 			continue;
 		}
-		entry->pending = NULL;
+		entry->ike_sa = NULL;
+		entry->pending = FALSE;
 	}
 	enumerator->destroy(enumerator);
 	this->lock->unlock(this->lock);