From: Martin Willi Date: Tue, 15 Mar 2011 14:20:09 +0000 (+0100) Subject: Move establish/inherit of rekeyed IKE_SAs to delete messages X-Git-Tag: 4.5.2~223 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3ced6b51e462201c32954ae9cb23449a377be3fa;p=thirdparty%2Fstrongswan.git 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 2d51786ca4..4a284e4f64 100644 --- 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 988100bcc1..69a74d8b73 100644 --- 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 7b1fef2b78..f07d2e3843 100644 --- 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 e74ca4eef5..33b5b52153 100644 --- 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 edaca0c0bb..c055dabc18 100644 --- 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); }