Keep the mutex locked as long as possible when deleting policies.

This change tries to prevent a race condition where a thread tries to
install the same policy another thread is currently deleting. If the
second thread releases the mutex in del_policy too early the first
thread could assume the policy does not exist (as it is not cached
anymore) but would not be able to actually install it if the second
thread was not yet able to delete it.

diff --git a/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c b/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
index 00ab3e9..6a92c4d 100644 (file)
--- a/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
+++ b/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
@@ -2209,10 +2209,13 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
        traffic_selector_t *dst_ts, policy_dir_t direction, u_int32_t reqid,
        mark_t mark, bool unrouted)
 {
-       policy_entry_t *current, policy, *to_delete = NULL;
+       policy_entry_t *current, policy;
+       enumerator_t *enumerator;
+       policy_sa_t *sa;
        netlink_buf_t request;
        struct nlmsghdr *hdr;
        struct xfrm_userpolicy_id *policy_id;
+       bool is_installed = TRUE;
 
        if (mark.value)
        {
@@ -2235,78 +2238,69 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
        /* find the policy */
        this->mutex->lock(this->mutex);
        current = this->policies->get(this->policies, &policy);
-       if (current)
+       if (!current)
        {
-               enumerator_t *enumerator;
-               policy_sa_t *sa;
-               bool is_installed = TRUE;
-               /* remove cached SA */
-               enumerator = current->sas->create_enumerator(current->sas);
-               while (enumerator->enumerate(enumerator, (void**)&sa))
+               if (mark.value)
                {
-                       if (reqid == sa->cfg.reqid)
-                       {
-                               current->sas->remove_at(current->sas, enumerator);
-                               break;
-                       }
-                       is_installed = FALSE;
+                       DBG1(DBG_KNL, "deleting policy %R === %R %N  (mark %u/0x%8x) "
+                                                 "failed, not found", src_ts, dst_ts, policy_dir_names,
+                                                  direction, mark.value, mark.mask);
                }
-               enumerator->destroy(enumerator);
-
-               if (current->sas->get_count(current->sas) > 0)
-               {       /* policy is used by more SAs, keep in kernel */
-                       DBG2(DBG_KNL, "policy still used by another CHILD_SA, not removed");
-                       if (!is_installed)
-                       {       /* no need to update the policy as it was not installed */
-                               this->mutex->unlock(this->mutex);
-                               policy_sa_destroy(sa);
-                               return SUCCESS;
-                       }
-                       policy_sa_destroy(sa);
+               else
+               {
+                       DBG1(DBG_KNL, "deleting policy %R === %R %N failed, not found",
+                                                  src_ts, dst_ts, policy_dir_names, direction);
+               }
+               this->mutex->unlock(this->mutex);
+               return NOT_FOUND;
+       }
 
-                       if (mark.value)
-                       {
-                               DBG2(DBG_KNL, "updating policy %R === %R %N  (mark %u/0x%8x)",
-                                                          src_ts, dst_ts, policy_dir_names, direction,
-                                                          mark.value, mark.mask);
-                       }
-                       else
-                       {
-                               DBG2(DBG_KNL, "updating policy %R === %R %N",
-                                                          src_ts, dst_ts, policy_dir_names, direction);
-                       }
+       /* remove cached SA by reqid */
+       enumerator = current->sas->create_enumerator(current->sas);
+       while (enumerator->enumerate(enumerator, (void**)&sa))
+       {
+               if (reqid == sa->cfg.reqid)
+               {
+                       current->sas->remove_at(current->sas, enumerator);
+                       break;
+               }
+               is_installed = FALSE;
+       }
+       enumerator->destroy(enumerator);
 
-                       current->sas->get_first(current->sas, (void**)&sa);
-                       if (add_policy_internal(this, current, sa, TRUE) != SUCCESS)
-                       {
-                               DBG1(DBG_KNL, "unable to update policy %R === %R %N",
-                                                          src_ts, dst_ts, policy_dir_names, direction);
-                               return FAILED;
-                       }
+       if (current->sas->get_count(current->sas) > 0)
+       {       /* policy is used by more SAs, keep in kernel */
+               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_sa_destroy(sa);
                        return SUCCESS;
                }
-               /* remove if last reference */
                policy_sa_destroy(sa);
-               this->policies->remove(this->policies, current);
-               to_delete = current;
-       }
-       this->mutex->unlock(this->mutex);
 
-       if (!to_delete)
-       {
                if (mark.value)
                {
-                       DBG1(DBG_KNL, "deleting policy %R === %R %N  (mark %u/0x%8x) "
-                                                 "failed, not found", src_ts, dst_ts, policy_dir_names,
-                                                  direction, mark.value, mark.mask);
+                       DBG2(DBG_KNL, "updating policy %R === %R %N  (mark %u/0x%8x)",
+                                                  src_ts, dst_ts, policy_dir_names, direction,
+                                                  mark.value, mark.mask);
                }
                else
                {
-                       DBG1(DBG_KNL, "deleting policy %R === %R %N failed, not found",
+                       DBG2(DBG_KNL, "updating policy %R === %R %N",
                                                   src_ts, dst_ts, policy_dir_names, direction);
                }
-               return NOT_FOUND;
+
+               current->sas->get_first(current->sas, (void**)&sa);
+               if (add_policy_internal(this, current, sa, TRUE) != SUCCESS)
+               {
+                       DBG1(DBG_KNL, "unable to update policy %R === %R %N",
+                                                  src_ts, dst_ts, policy_dir_names, direction);
+                       return FAILED;
+               }
+               return SUCCESS;
        }
+       policy_sa_destroy(sa);
 
        memset(&request, 0, sizeof(request));
 
@@ -2316,7 +2310,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
        hdr->nlmsg_len = NLMSG_LENGTH(sizeof(struct xfrm_userpolicy_id));
 
        policy_id = (struct xfrm_userpolicy_id*)NLMSG_DATA(hdr);
-       policy_id->sel = to_delete->sel;
+       policy_id->sel = current->sel;
        policy_id->dir = direction;
 
        if (mark.value)
@@ -2337,9 +2331,9 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
                mrk->m = mark.mask;
        }
 
-       if (to_delete->route)
+       if (current->route)
        {
-               route_entry_t *route = to_delete->route;
+               route_entry_t *route = current->route;
                if (hydra->kernel_interface->del_route(hydra->kernel_interface,
                                route->dst_net, route->prefixlen, route->gateway,
                                route->src_ip, route->if_name) != SUCCESS)
@@ -2350,6 +2344,10 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
                }
        }
 
+       this->policies->remove(this->policies, current);
+       policy_entry_destroy(current);
+       this->mutex->unlock(this->mutex);
+
        if (this->socket_xfrm->send_ack(this->socket_xfrm, hdr) != SUCCESS)
        {
                if (mark.value)
@@ -2363,10 +2361,8 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
                        DBG1(DBG_KNL, "unable to delete policy %R === %R %N",
                                                   src_ts, dst_ts, policy_dir_names, direction);
                }
-               policy_entry_destroy(to_delete);
                return FAILED;
        }
-       policy_entry_destroy(to_delete);
        return SUCCESS;
 }