From 350f1a81f63b45797b2804215b2950c2bf6e4984 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 12 Jun 2020 11:24:18 +0200 Subject: [PATCH] ikev2: Let ike/child-rekey tasks indicate if the passive task was adopted This gives us more flexibility with tasks that return NEED_MORE (currently none of the colliding tasks do, but that will change with multi-KE rekeyings). The active task has to check itself if the passive task is done and should be removed from the task manager. --- src/libcharon/sa/ikev2/task_manager_v2.c | 12 +++++++----- src/libcharon/sa/ikev2/tasks/child_rekey.c | 20 ++++++++------------ src/libcharon/sa/ikev2/tasks/child_rekey.h | 8 +++++--- src/libcharon/sa/ikev2/tasks/ike_rekey.c | 7 +++---- src/libcharon/sa/ikev2/tasks/ike_rekey.h | 12 +++++++----- 5 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c index e57ed1fb81..f273148c60 100644 --- a/src/libcharon/sa/ikev2/task_manager_v2.c +++ b/src/libcharon/sa/ikev2/task_manager_v2.c @@ -882,13 +882,15 @@ static status_t process_response(private_task_manager_t *this, } /** - * handle exchange collisions + * Handle exchange collisions, returns TRUE if the given passive task was + * adopted by the active task and the task manager lost control over it. */ static bool handle_collisions(private_task_manager_t *this, task_t *task) { enumerator_t *enumerator; task_t *active; task_type_t type; + bool adopted = FALSE; type = task->get_type(task); @@ -906,7 +908,7 @@ static bool handle_collisions(private_task_manager_t *this, task_t *task) if (type == TASK_IKE_REKEY || type == TASK_IKE_DELETE) { ike_rekey_t *rekey = (ike_rekey_t*)active; - rekey->collide(rekey, task); + adopted = rekey->collide(rekey, task); break; } continue; @@ -914,7 +916,7 @@ static bool handle_collisions(private_task_manager_t *this, task_t *task) if (type == TASK_CHILD_REKEY || type == TASK_CHILD_DELETE) { child_rekey_t *rekey = (child_rekey_t*)active; - rekey->collide(rekey, task); + adopted = rekey->collide(rekey, task); break; } continue; @@ -922,11 +924,11 @@ static bool handle_collisions(private_task_manager_t *this, task_t *task) continue; } enumerator->destroy(enumerator); - return TRUE; + return adopted; } enumerator->destroy(enumerator); } - return FALSE; + return adopted; } /** diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.c b/src/libcharon/sa/ikev2/tasks/child_rekey.c index 084c752b7b..b41827d3b5 100644 --- a/src/libcharon/sa/ikev2/tasks/child_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c @@ -518,7 +518,7 @@ METHOD(child_rekey_t, is_redundant, bool, return FALSE; } -METHOD(child_rekey_t, collide, void, +METHOD(child_rekey_t, collide, bool, private_child_rekey_t *this, task_t *other) { /* the task manager only detects exchange collision, but not if @@ -530,16 +530,14 @@ METHOD(child_rekey_t, collide, void, if (rekey->child_sa != this->child_sa) { /* not the same child => no collision */ - other->destroy(other); - return; + return FALSE; } /* ignore passive tasks that did not successfully create a CHILD_SA */ other_child = rekey->child_create->get_child(rekey->child_create); if (!other_child || other_child->get_state(other_child) != CHILD_INSTALLED) { - other->destroy(other); - return; + return FALSE; } } else if (other->get_type(other) == TASK_CHILD_DELETE) @@ -548,26 +546,24 @@ METHOD(child_rekey_t, collide, void, if (is_redundant(this, del->get_child(del))) { this->other_child_destroyed = TRUE; - other->destroy(other); - return; + return FALSE; } if (del->get_child(del) != this->child_sa) { /* not the same child => no collision */ - other->destroy(other); - return; + return FALSE; } } else { - /* any other task is not critical for collisions, ignore */ - other->destroy(other); - return; + /* shouldn't happen */ + return FALSE; } DBG1(DBG_IKE, "detected %N collision with %N", task_type_names, TASK_CHILD_REKEY, task_type_names, other->get_type(other)); DESTROY_IF(this->collision); this->collision = other; + return TRUE; } METHOD(task_t, migrate, void, diff --git a/src/libcharon/sa/ikev2/tasks/child_rekey.h b/src/libcharon/sa/ikev2/tasks/child_rekey.h index 0ad1a062d4..db4d8fbf06 100644 --- a/src/libcharon/sa/ikev2/tasks/child_rekey.h +++ b/src/libcharon/sa/ikev2/tasks/child_rekey.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016 Tobias Brunner + * Copyright (C) 2016-2020 Tobias Brunner * Copyright (C) 2007 Martin Willi * HSR Hochschule fuer Technik Rapperswil * @@ -58,9 +58,11 @@ struct child_rekey_t { * be handled gracefully. The task manager is aware of what exchanges * are going on and notifies the active task by passing the passive. * - * @param other passive task (adopted) + * @param other passive task + * @return whether the task was adopted and should be removed from + * the task manager's control */ - void (*collide)(child_rekey_t* this, task_t *other); + bool (*collide)(child_rekey_t* this, task_t *other); }; /** diff --git a/src/libcharon/sa/ikev2/tasks/ike_rekey.c b/src/libcharon/sa/ikev2/tasks/ike_rekey.c index d49d0f43e0..e9948da1d0 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/ike_rekey.c @@ -421,7 +421,7 @@ METHOD(ike_rekey_t, did_collide, bool, return this->collision != NULL; } -METHOD(ike_rekey_t, collide, void, +METHOD(ike_rekey_t, collide, bool, private_ike_rekey_t* this, task_t *other) { DBG1(DBG_IKE, "detected %N collision with %N", task_type_names, @@ -431,7 +431,6 @@ METHOD(ike_rekey_t, collide, void, { case TASK_IKE_DELETE: conclude_undetected_collision(this); - other->destroy(other); break; case TASK_IKE_REKEY: { @@ -441,17 +440,17 @@ METHOD(ike_rekey_t, collide, void, { DBG1(DBG_IKE, "colliding exchange did not result in an IKE_SA, " "ignore"); - other->destroy(other); break; } DESTROY_IF(&this->collision->public.task); this->collision = rekey; - break; + return TRUE; } default: /* shouldn't happen */ break; } + return FALSE; } /** diff --git a/src/libcharon/sa/ikev2/tasks/ike_rekey.h b/src/libcharon/sa/ikev2/tasks/ike_rekey.h index 86b512c92f..c2157e0c29 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_rekey.h +++ b/src/libcharon/sa/ikev2/tasks/ike_rekey.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016 Tobias Brunner + * Copyright (C) 2016-2020 Tobias Brunner * Copyright (C) 2007 Martin Willi * HSR Hochschule fuer Technik Rapperswil * @@ -46,15 +46,17 @@ struct ike_rekey_t { bool (*did_collide)(ike_rekey_t *this); /** - * Register a rekeying task which collides with this one. + * Register a rekeying/delete task which collides with this one. * * If two peers initiate rekeying at the same time, the collision must * be handled gracefully. The task manager is aware of what exchanges - * are going on and notifies the outgoing task by passing the incoming. + * are going on and notifies the active task by passing the passive. * - * @param other incoming task + * @param other passive task + * @return whether the task was adopted and should be removed from + * the task manager's control */ - void (*collide)(ike_rekey_t* this, task_t *other); + bool (*collide)(ike_rekey_t* this, task_t *other); }; /**