From: Tobias Brunner <tobias@strongswan.org>
Date: Fri, 12 Jun 2020 09:24:18 +0000 (+0200)
Subject: ikev2: Let ike/child-rekey tasks indicate if the passive task was adopted
X-Git-Tag: 5.9.7dr2~1^2~14
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b6652ababed3c200df9f8ab764cdaec872dc4c33;p=thirdparty%2Fstrongswan.git

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.
---

diff --git a/src/libcharon/sa/ikev2/task_manager_v2.c b/src/libcharon/sa/ikev2/task_manager_v2.c
index 2fd7a48bb9..62707cb465 100644
--- a/src/libcharon/sa/ikev2/task_manager_v2.c
+++ b/src/libcharon/sa/ikev2/task_manager_v2.c
@@ -872,13 +872,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);
 
@@ -896,7 +898,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;
@@ -904,7 +906,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;
@@ -912,11 +914,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 1448189f1c..9281c6339d 100644
--- a/src/libcharon/sa/ikev2/tasks/child_rekey.c
+++ b/src/libcharon/sa/ikev2/tasks/child_rekey.c
@@ -528,7 +528,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
@@ -540,16 +540,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)
@@ -558,26 +556,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 4b584274e0..849df3a2b8 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
  *
  * Copyright (C) secunet Security Networks AG
@@ -59,9 +59,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 f539ae2198..aac908ee6b 100644
--- a/src/libcharon/sa/ikev2/tasks/ike_rekey.c
+++ b/src/libcharon/sa/ikev2/tasks/ike_rekey.c
@@ -430,7 +430,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,
@@ -440,7 +440,6 @@ METHOD(ike_rekey_t, collide, void,
 	{
 		case TASK_IKE_DELETE:
 			conclude_undetected_collision(this);
-			other->destroy(other);
 			break;
 		case TASK_IKE_REKEY:
 		{
@@ -450,17 +449,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 30f9919db3..5fab3491c1 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
  *
  * Copyright (C) secunet Security Networks AG
@@ -47,15 +47,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);
 };
 
 /**