Move establish/inherit of rekeyed IKE_SAs to delete messages

Having the inherit() function delayed to the IKE_SA establish procedure
was problematic. The task destroy function was never a good place and
results in locking/cleanup problems. After establishing the SA, it
should be really checked in ASAP to avoid any triggered DPD checks
to get lost.

diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c
index 2d51786ca457bd658587255e781583597a094172..4a284e4f64198b7897b8bc03cec8e79d52fcf5ac 100644 (file)
--- a/src/libcharon/sa/ike_sa.c
+++ b/src/libcharon/sa/ike_sa.c
@@ -1896,7 +1896,7 @@ METHOD(ike_sa_t, create_task_enumerator, enumerator_t*,
        return this->task_manager->create_task_enumerator(this->task_manager, queue);
 }
 
-METHOD(ike_sa_t, inherit, status_t,
+METHOD(ike_sa_t, inherit, void,
        private_ike_sa_t *this, ike_sa_t *other_public)
 {
        private_ike_sa_t *other = (private_ike_sa_t*)other_public;
@@ -1977,8 +1977,6 @@ METHOD(ike_sa_t, inherit, status_t,
                lib->scheduler->schedule_job(lib->scheduler,
                                (job_t*)delete_ike_sa_job_create(this->ike_sa_id, TRUE), delete);
        }
-       /* we have to initate here, there may be new tasks to handle */
-       return this->task_manager->initiate(this->task_manager);
 }
 
 METHOD(ike_sa_t, destroy, void,

diff --git a/src/libcharon/sa/ike_sa.h b/src/libcharon/sa/ike_sa.h
index 988100bcc18e28ae5775c2496a04fca6d2aca278..69a74d8b73b914dd9ca4221aee99bf8b2c99d0a0 100644 (file)
--- a/src/libcharon/sa/ike_sa.h
+++ b/src/libcharon/sa/ike_sa.h
@@ -912,9 +912,8 @@ struct ike_sa_t {
         * As this call may initiate inherited tasks, a status is returned.
         *
         * @param other                 other task to inherit from
-        * @return                              DESTROY_ME if initiation of inherited task failed
         */
-       status_t (*inherit) (ike_sa_t *this, ike_sa_t *other);
+       void (*inherit) (ike_sa_t *this, ike_sa_t *other);
 
        /**
         * Reset the IKE_SA, useable when initiating fails

diff --git a/src/libcharon/sa/task_manager.c b/src/libcharon/sa/task_manager.c
index 7b1fef2b785a25ea2ab029a81852e33c32443f17..f07d2e38431f4429449c97cde7007ff5b570e36b 100644 (file)
--- a/src/libcharon/sa/task_manager.c
+++ b/src/libcharon/sa/task_manager.c
@@ -545,7 +545,7 @@ static status_t process_response(private_task_manager_t *this,
 /**
  * handle exchange collisions
  */
-static void handle_collisions(private_task_manager_t *this, task_t *task)
+static bool handle_collisions(private_task_manager_t *this, task_t *task)
 {
        iterator_t *iterator;
        task_t *active;
@@ -584,12 +584,11 @@ static void handle_collisions(private_task_manager_t *this, task_t *task)
                                        continue;
                        }
                        iterator->destroy(iterator);
-                       return;
+                       return TRUE;
                }
                iterator->destroy(iterator);
        }
-       /* destroy task if not registered in any active task */
-       task->destroy(task);
+       return FALSE;
 }
 
 /**
@@ -623,9 +622,17 @@ static status_t build_response(private_task_manager_t *this, message_t *request)
                        case SUCCESS:
                                /* task completed, remove it */
                                iterator->remove(iterator);
-                               handle_collisions(this, task);
+                               if (!handle_collisions(this, task))
+                               {
+                                       task->destroy(task);
+                               }
+                               break;
                        case NEED_MORE:
                                /* processed, but task needs another exchange */
+                               if (handle_collisions(this, task))
+                               {
+                                       iterator->remove(iterator);
+                               }
                                break;
                        case FAILED:
                        default:

diff --git a/src/libcharon/sa/tasks/child_rekey.c b/src/libcharon/sa/tasks/child_rekey.c
index e74ca4eef51b01b0170c62ba5ba0d0931561776e..33b5b52153728ae0f0e269eb13f5395a81d8433d 100644 (file)
--- a/src/libcharon/sa/tasks/child_rekey.c
+++ b/src/libcharon/sa/tasks/child_rekey.c
@@ -412,6 +412,8 @@ static void collide(private_child_rekey_t *this, task_t *other)
                other->destroy(other);
                return;
        }
+       DBG1(DBG_IKE, "detected %N collision with %N", task_type_names, CHILD_REKEY,
+                task_type_names, other->get_type(other));
        DESTROY_IF(this->collision);
        this->collision = other;
 }

diff --git a/src/libcharon/sa/tasks/ike_rekey.c b/src/libcharon/sa/tasks/ike_rekey.c
index edaca0c0bb6df92dd7a0e5318e719dfffbfd2741..c055dabc188aeb858204a4c76b2f2f3e5c861489 100644 (file)
--- a/src/libcharon/sa/tasks/ike_rekey.c
+++ b/src/libcharon/sa/tasks/ike_rekey.c
@@ -67,9 +67,35 @@ struct private_ike_rekey_t {
        task_t *collision;
 };
 
+/**
+ * Establish the new replacement IKE_SA
+ */
+static void establish_new(private_ike_rekey_t *this)
+{
+       if (this->new_sa)
+       {
+               this->new_sa->set_state(this->new_sa, IKE_ESTABLISHED);
+               DBG0(DBG_IKE, "IKE_SA %s[%d] rekeyed between %H[%Y]...%H[%Y]",
+                        this->new_sa->get_name(this->new_sa),
+                        this->new_sa->get_unique_id(this->new_sa),
+                        this->ike_sa->get_my_host(this->ike_sa),
+                        this->ike_sa->get_my_id(this->ike_sa),
+                        this->ike_sa->get_other_host(this->ike_sa),
+                        this->ike_sa->get_other_id(this->ike_sa));
+
+               this->new_sa->inherit(this->new_sa, this->ike_sa);
+               charon->bus->ike_rekey(charon->bus, this->ike_sa, this->new_sa);
+               charon->ike_sa_manager->checkin(charon->ike_sa_manager, this->new_sa);
+               this->new_sa = NULL;
+               /* set threads active IKE_SA after checkin */
+               charon->bus->set_sa(charon->bus, this->ike_sa);
+       }
+}
+
 METHOD(task_t, process_r_delete, status_t,
        private_ike_rekey_t *this, message_t *message)
 {
+       establish_new(this);
        return this->ike_delete->task.process(&this->ike_delete->task, message);
 }
 
@@ -176,14 +202,6 @@ METHOD(task_t, build_r, status_t,
        }
 
        this->ike_sa->set_state(this->ike_sa, IKE_REKEYING);
-       this->new_sa->set_state(this->new_sa, IKE_ESTABLISHED);
-       DBG0(DBG_IKE, "IKE_SA %s[%d] established between %H[%Y]...%H[%Y]",
-                this->new_sa->get_name(this->new_sa),
-                this->new_sa->get_unique_id(this->new_sa),
-                this->ike_sa->get_my_host(this->ike_sa),
-                this->ike_sa->get_my_id(this->ike_sa),
-                this->ike_sa->get_other_host(this->ike_sa),
-                this->ike_sa->get_other_id(this->ike_sa));
 
        /* rekeying successful, delete the IKE_SA using a subtask */
        this->ike_delete = ike_delete_create(this->ike_sa, FALSE);
@@ -233,15 +251,6 @@ METHOD(task_t, process_i, status_t,
                        break;
        }
 
-       this->new_sa->set_state(this->new_sa, IKE_ESTABLISHED);
-       DBG0(DBG_IKE, "IKE_SA %s[%d] established between %H[%Y]...%H[%Y]",
-                this->new_sa->get_name(this->new_sa),
-                this->new_sa->get_unique_id(this->new_sa),
-                this->ike_sa->get_my_host(this->ike_sa),
-                this->ike_sa->get_my_id(this->ike_sa),
-                this->ike_sa->get_other_host(this->ike_sa),
-                this->ike_sa->get_other_id(this->ike_sa));
-
        /* check for collisions */
        if (this->collision &&
                this->collision->get_type(this->collision) == IKE_REKEY)
@@ -280,21 +289,20 @@ METHOD(task_t, process_i, status_t,
                                host = this->ike_sa->get_other_host(this->ike_sa);
                                this->new_sa->set_other_host(this->new_sa, host->clone(host));
                                this->ike_sa->set_state(this->ike_sa, IKE_ESTABLISHED);
+                               this->new_sa->set_state(this->new_sa, IKE_REKEYING);
                                if (this->new_sa->delete(this->new_sa) == DESTROY_ME)
                                {
-                                       charon->ike_sa_manager->checkin_and_destroy(
-                                                               charon->ike_sa_manager, this->new_sa);
+                                       this->new_sa->destroy(this->new_sa);
                                }
                                else
                                {
                                        charon->ike_sa_manager->checkin(
                                                                charon->ike_sa_manager, this->new_sa);
+                                       /* set threads active IKE_SA after checkin */
+                                       charon->bus->set_sa(charon->bus, this->ike_sa);
                                }
-                               /* set threads active IKE_SA after checkin */
-                               charon->bus->set_sa(charon->bus, this->ike_sa);
-                               /* inherit to other->new_sa in destroy() */
-                               this->new_sa = other->new_sa;
-                               other->new_sa = NULL;
+                               this->new_sa = NULL;
+                               establish_new(other);
                                return SUCCESS;
                        }
                }
@@ -302,6 +310,8 @@ METHOD(task_t, process_i, status_t,
                charon->bus->set_sa(charon->bus, this->ike_sa);
        }
 
+       establish_new(this);
+
        /* rekeying successful, delete the IKE_SA using a subtask */
        this->ike_delete = ike_delete_create(this->ike_sa, TRUE);
        this->public.task.build = _build_i_delete;
@@ -319,6 +329,8 @@ METHOD(task_t, get_type, task_type_t,
 METHOD(ike_rekey_t, collide, void,
        private_ike_rekey_t* this, task_t *other)
 {
+       DBG1(DBG_IKE, "detected %N collision with %N", task_type_names, IKE_REKEY,
+                task_type_names, other->get_type(other));
        DESTROY_IF(this->collision);
        this->collision = other;
 }
@@ -334,13 +346,7 @@ METHOD(task_t, migrate, void,
        {
                this->ike_delete->task.destroy(&this->ike_delete->task);
        }
-       if (this->new_sa)
-       {
-               charon->ike_sa_manager->checkin_and_destroy(charon->ike_sa_manager,
-                                                                                                       this->new_sa);
-               /* set threads active IKE_SA after checkin */
-               charon->bus->set_sa(charon->bus, this->ike_sa);
-       }
+       DESTROY_IF(this->new_sa);
        DESTROY_IF(this->collision);
 
        this->collision = NULL;
@@ -353,23 +359,6 @@ METHOD(task_t, migrate, void,
 METHOD(task_t, destroy, void,
        private_ike_rekey_t *this)
 {
-       if (this->new_sa)
-       {
-               if (this->new_sa->get_state(this->new_sa) == IKE_ESTABLISHED &&
-                       this->new_sa->inherit(this->new_sa, this->ike_sa) != DESTROY_ME)
-               {
-                       /* invoke hook if rekeying was successful */
-                       charon->bus->ike_rekey(charon->bus, this->ike_sa, this->new_sa);
-                       charon->ike_sa_manager->checkin(charon->ike_sa_manager, this->new_sa);
-               }
-               else
-               {
-                       charon->ike_sa_manager->checkin_and_destroy(charon->ike_sa_manager,
-                                                                                                               this->new_sa);
-               }
-               /* set threads active IKE_SA after checkin */
-               charon->bus->set_sa(charon->bus, this->ike_sa);
-       }
        if (this->ike_init)
        {
                this->ike_init->task.destroy(&this->ike_init->task);
@@ -378,6 +367,7 @@ METHOD(task_t, destroy, void,
        {
                this->ike_delete->task.destroy(&this->ike_delete->task);
        }
+       DESTROY_IF(this->new_sa);
        DESTROY_IF(this->collision);
        free(this);
 }