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.

diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c
index c7a8a6e0c62f00492999ee72a8153daae1c3b0b6..86d9f4c22a0f2bbe4ee9966e21d604485cf72656 100644 (file)
--- 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);