kernel-netlink: Let only a single thread work on a specific policy

Other threads are free to add/update/delete other policies.

This tries to prevent race conditions caused by releasing the mutex while
sending messages to the kernel.  For instance, if break-before-make
reauthentication is used and one thread on the responder is delayed in
deleting the policies that another thread is concurrently adding for the
new SA.  This could have resulted in no policies being installed
eventually.

Fixes #1400.

diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c
index a27e70b9b430ffa27bbb6d1933172a60324408a8..e78e13e40815dbc900c7c694cd098f3a0d14c8b8 100644 (file)
--- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c
+++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c
@@ -41,6 +41,7 @@
 #include <daemon.h>
 #include <utils/debug.h>
 #include <threading/mutex.h>
+#include <threading/condvar.h>
 #include <collections/array.h>
 #include <collections/hashtable.h>
 #include <collections/linked_list.h>
@@ -289,6 +290,11 @@ struct private_kernel_netlink_ipsec_t {
         */
        mutex_t *mutex;
 
+       /**
+        * Condvar to synchronize access to individual policies
+        */
+       condvar_t *condvar;
+
        /**
         * Hash table of installed policies (policy_entry_t)
         */
@@ -575,6 +581,12 @@ struct policy_entry_t {
 
        /** reqid for this policy */
        uint32_t reqid;
+
+       /** Number of threads waiting to work on this policy */
+       int waiting;
+
+       /** TRUE if a thread is working on this policy */
+       bool working;
 };
 
 /**
@@ -2148,6 +2160,20 @@ METHOD(kernel_ipsec_t, flush_sas, status_t,
        return SUCCESS;
 }
 
+/**
+ * Unlock the mutex and signal waiting threads
+ */
+static void policy_change_done(private_kernel_netlink_ipsec_t *this,
+                                                          policy_entry_t *policy)
+{
+       policy->working = FALSE;
+       if (policy->waiting)
+       {       /* don't need to wake threads waiting for other policies */
+               this->condvar->broadcast(this->condvar);
+       }
+       this->mutex->unlock(this->mutex);
+}
+
 /**
  * Add or update a policy in the kernel.
  *
@@ -2219,7 +2245,7 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this,
                                                           count * sizeof(*tmpl));
                if (!tmpl)
                {
-                       this->mutex->unlock(this->mutex);
+                       policy_change_done(this, policy);
                        return FAILED;
                }
 
@@ -2252,7 +2278,7 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this,
 
        if (!add_mark(hdr, sizeof(request), ipsec->mark))
        {
-               this->mutex->unlock(this->mutex);
+               policy_change_done(this, policy);
                return FAILED;
        }
        this->mutex->unlock(this->mutex);
@@ -2264,22 +2290,13 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this,
                hdr->nlmsg_type = XFRM_MSG_UPDPOLICY;
                status = this->socket_xfrm->send_ack(this->socket_xfrm, hdr);
        }
+
+       this->mutex->lock(this->mutex);
        if (status != SUCCESS)
        {
+               policy_change_done(this, policy);
                return FAILED;
        }
-
-       /* find the policy again */
-       this->mutex->lock(this->mutex);
-       policy = this->policies->get(this->policies, &clone);
-       if (!policy ||
-                policy->used_by->find_first(policy->used_by,
-                                                                        NULL, (void**)&mapping) != SUCCESS)
-       {       /* policy or mapping is already gone, ignore */
-               this->mutex->unlock(this->mutex);
-               return SUCCESS;
-       }
-
        /* install a route, if:
         * - this is a inbound policy (to just get one for each child)
         * - we are in tunnel/BEET mode or install a bypass policy
@@ -2328,7 +2345,7 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this,
                        if (!charon->kernel->get_interface(charon->kernel, iface,
                                                                                           &route->if_name))
                        {
-                               this->mutex->unlock(this->mutex);
+                               policy_change_done(this, policy);
                                route_entry_destroy(route);
                                return SUCCESS;
                        }
@@ -2338,7 +2355,7 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this,
                                route_entry_t *old = policy->route;
                                if (route_entry_equals(old, route))
                                {
-                                       this->mutex->unlock(this->mutex);
+                                       policy_change_done(this, policy);
                                        route_entry_destroy(route);
                                        return SUCCESS;
                                }
@@ -2380,7 +2397,7 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this,
                        free(route);
                }
        }
-       this->mutex->unlock(this->mutex);
+       policy_change_done(this, policy);
        return SUCCESS;
 }
 
@@ -2428,6 +2445,14 @@ METHOD(kernel_ipsec_t, add_policy, status_t,
                policy_entry_destroy(this, policy);
                policy = current;
                found = TRUE;
+
+               policy->waiting++;
+               while (policy->working)
+               {
+                       this->condvar->wait(this->condvar, this->mutex);
+               }
+               policy->waiting--;
+               policy->working = TRUE;
        }
        else
        {       /* use the new one, if we have no such policy */
@@ -2478,7 +2503,7 @@ METHOD(kernel_ipsec_t, add_policy, status_t,
        if (!update)
        {       /* we don't update the policy if the priority is lower than that of
                 * the currently installed one */
-               this->mutex->unlock(this->mutex);
+               policy_change_done(this, policy);
                DBG2(DBG_KNL, "not updating policy %R === %R %N%s [priority %u,"
                         "refcount %d]", id->src_ts, id->dst_ts, policy_dir_names,
                         id->dir, markstr, cur_priority, use_count);
@@ -2606,6 +2631,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
        };
        char markstr[32] = "";
        int use_count;
+       status_t status = SUCCESS;
 
        format_mark(markstr, sizeof(markstr), id->mark);
 
@@ -2628,6 +2654,13 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
                this->mutex->unlock(this->mutex);
                return NOT_FOUND;
        }
+       current->waiting++;
+       while (current->working)
+       {
+               this->condvar->wait(this->condvar, this->mutex);
+       }
+       current->working = TRUE;
+       current->waiting--;
 
        /* remove mapping to SA by reqid and priority */
        auto_priority = get_priority(current, data->prio,id->interface);
@@ -2661,7 +2694,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
                DBG2(DBG_KNL, "policy still used by another CHILD_SA, not removed");
                if (!is_installed)
                {       /* no need to update as the policy was not installed for this SA */
-                       this->mutex->unlock(this->mutex);
+                       policy_change_done(this, current);
                        DBG2(DBG_KNL, "not updating policy %R === %R %N%s [priority %u, "
                                 "refcount %d]", id->src_ts, id->dst_ts, policy_dir_names,
                                 id->dir, markstr, cur_priority, use_count);
@@ -2695,7 +2728,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
 
        if (!add_mark(hdr, sizeof(request), id->mark))
        {
-               this->mutex->unlock(this->mutex);
+               policy_change_done(this, current);
                return FAILED;
        }
 
@@ -2711,18 +2744,27 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
                                 id->dir, markstr);
                }
        }
-
-       this->policies->remove(this->policies, current);
-       policy_entry_destroy(this, current);
        this->mutex->unlock(this->mutex);
 
        if (this->socket_xfrm->send_ack(this->socket_xfrm, hdr) != SUCCESS)
        {
                DBG1(DBG_KNL, "unable to delete policy %R === %R %N%s", id->src_ts,
                         id->dst_ts, policy_dir_names, id->dir, markstr);
-               return FAILED;
+               status = FAILED;
        }
-       return SUCCESS;
+
+       this->mutex->lock(this->mutex);
+       if (!current->waiting)
+       {       /* only if no other thread still needs the policy */
+               this->policies->remove(this->policies, current);
+               policy_entry_destroy(this, current);
+               this->mutex->unlock(this->mutex);
+       }
+       else
+       {
+               policy_change_done(this, current);
+       }
+       return status;
 }
 
 METHOD(kernel_ipsec_t, flush_policies, status_t,
@@ -2978,6 +3020,7 @@ METHOD(kernel_ipsec_t, destroy, void,
        enumerator->destroy(enumerator);
        this->policies->destroy(this->policies);
        this->sas->destroy(this->sas);
+       this->condvar->destroy(this->condvar);
        this->mutex->destroy(this->mutex);
        free(this);
 }
@@ -3017,6 +3060,7 @@ kernel_netlink_ipsec_t *kernel_netlink_ipsec_create()
                                                                (hashtable_equals_t)ipsec_sa_equals, 32),
                .bypass = array_create(sizeof(bypass_t), 0),
                .mutex = mutex_create(MUTEX_TYPE_DEFAULT),
+               .condvar = condvar_create(CONDVAR_TYPE_DEFAULT),
                .get_priority = dlsym(RTLD_DEFAULT,
                                                          "kernel_netlink_get_priority_custom"),
                .policy_update = lib->settings->get_bool(lib->settings,