Merge branch 'consistent-reqid'

Checks if a trap policy exists when installing a CHILD_SA as responder,
reuse that reqid and keeping the trap untouched. This makes auto=route on
both sides more reliable.

In addition, we no prevent to refcount an existing policy if the reqid differs;
this should not happen anymore. We now can properly reject new CHILD_SAs in
such conflicts, instead of silently breaking an existing policy.

4 files changed, 62 insertions, 9 deletions

diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c
index f50e32ff1..1069b2d91 100644
--- a/src/libcharon/sa/child_sa.c
+++ b/src/libcharon/sa/child_sa.c
@@ -1149,7 +1149,11 @@ child_sa_t * child_sa_create(host_t *me, host_t* other,
 		}
 		else
 		{
-			this->reqid = ref_get(&reqid);
+			this->reqid = charon->traps->find_reqid(charon->traps, config);
+			if (!this->reqid)
+			{
+				this->reqid = ref_get(&reqid);
+			}
 		}
 	}
 
diff --git a/src/libcharon/sa/trap_manager.c b/src/libcharon/sa/trap_manager.c
index 6c0ae19c7..ab638ff3e 100644
--- a/src/libcharon/sa/trap_manager.c
+++ b/src/libcharon/sa/trap_manager.c
@@ -109,6 +109,7 @@ METHOD(trap_manager_t, install, u_int32_t,
 								 0, ike_cfg->get_other_port(ike_cfg));
 	if (!other || other->is_anyaddr(other))
 	{
+		DESTROY_IF(other);
 		DBG1(DBG_CFG, "installing trap failed, remote address unknown");
 		return 0;
 	}
@@ -141,6 +142,8 @@ METHOD(trap_manager_t, install, u_int32_t,
 		}
 	}
 	enumerator->destroy(enumerator);
+	this->lock->unlock(this->lock);
+
 	if (found)
 	{	/* config might have changed so update everything */
 		DBG1(DBG_CFG, "updating already routed CHILD_SA '%s'",
@@ -179,10 +182,11 @@ METHOD(trap_manager_t, install, u_int32_t,
 			.child_sa = child_sa,
 			.peer_cfg = peer->get_ref(peer),
 		);
+		this->lock->write_lock(this->lock);
 		this->traps->insert_last(this->traps, entry);
+		this->lock->unlock(this->lock);
 		reqid = child_sa->get_reqid(child_sa);
 	}
-	this->lock->unlock(this->lock);
 
 	if (status != SUCCESS)
 	{
@@ -251,6 +255,31 @@ METHOD(trap_manager_t, create_enumerator, enumerator_t*,
 									(void*)this->lock->unlock);
 }
 
+METHOD(trap_manager_t, find_reqid, u_int32_t,
+	private_trap_manager_t *this, child_cfg_t *child)
+{
+	enumerator_t *enumerator;
+	child_cfg_t *current;
+	entry_t *entry;
+	u_int32_t reqid = 0;
+
+	this->lock->read_lock(this->lock);
+	enumerator = this->traps->create_enumerator(this->traps);
+	while (enumerator->enumerate(enumerator, &entry))
+	{
+		current = entry->child_sa->get_config(entry->child_sa);
+		if (streq(current->get_name(current), child->get_name(child)))
+		{
+			reqid = entry->child_sa->get_reqid(entry->child_sa);
+			break;
+		}
+	}
+	enumerator->destroy(enumerator);
+	this->lock->unlock(this->lock);
+
+	return reqid;
+}
+
 METHOD(trap_manager_t, acquire, void,
 	private_trap_manager_t *this, u_int32_t reqid,
 	traffic_selector_t *src, traffic_selector_t *dst)
@@ -319,8 +348,7 @@ METHOD(trap_manager_t, acquire, void,
 		}
 		else
 		{
-			charon->ike_sa_manager->checkin_and_destroy(
-												charon->ike_sa_manager, ike_sa);
+			ike_sa->destroy(ike_sa);
 		}
 	}
 	peer->destroy(peer);
@@ -417,6 +445,7 @@ trap_manager_t *trap_manager_create(void)
 			.install = _install,
 			.uninstall = _uninstall,
 			.create_enumerator = _create_enumerator,
+			.find_reqid = _find_reqid,
 			.acquire = _acquire,
 			.flush = _flush,
 			.destroy = _destroy,
@@ -435,4 +464,3 @@ trap_manager_t *trap_manager_create(void)
 
 	return &this->public;
 }
-
diff --git a/src/libcharon/sa/trap_manager.h b/src/libcharon/sa/trap_manager.h
index e3d355662..97de45645 100644
--- a/src/libcharon/sa/trap_manager.h
+++ b/src/libcharon/sa/trap_manager.h
@@ -58,6 +58,14 @@ struct trap_manager_t {
 	enumerator_t* (*create_enumerator)(trap_manager_t *this);
 
 	/**
+	 * Find the reqid of a child config installed as a trap.
+	 *
+	 * @param child		CHILD_SA config to get the reqid for
+	 * @return			reqid of trap, 0 if not found
+	 */
+	u_int32_t (*find_reqid)(trap_manager_t *this, child_cfg_t *child);
+
+	/**
 	 * Acquire an SA triggered by an installed trap.
 	 *
 	 * @param reqid		requid of the triggering CHILD_SA
diff --git a/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c b/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
index a20804507..47e725c1c 100644
--- a/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
+++ b/src/libhydra/plugins/kernel_netlink/kernel_netlink_ipsec.c
@@ -558,6 +558,9 @@ struct policy_entry_t {
 
 	/** List of SAs this policy is used by, ordered by priority */
 	linked_list_t *used_by;
+
+	/** reqid for this policy */
+	u_int32_t reqid;
 };
 
 /**
@@ -2048,7 +2051,7 @@ static status_t add_policy_internal(private_kernel_netlink_ipsec_t *this,
 			{
 				continue;
 			}
-			tmpl->reqid = ipsec->cfg.reqid;
+			tmpl->reqid = policy->reqid;
 			tmpl->id.proto = protos[i].proto;
 			tmpl->aalgos = tmpl->ealgos = tmpl->calgos = ~0;
 			tmpl->mode = mode2kernel(proto_mode);
@@ -2204,6 +2207,7 @@ METHOD(kernel_ipsec_t, add_policy, status_t,
 		.sel = ts2selector(src_ts, dst_ts),
 		.mark = mark.value & mark.mask,
 		.direction = direction,
+		.reqid = sa->reqid,
 	);
 
 	/* find the policy, which matches EXACTLY */
@@ -2211,6 +2215,16 @@ METHOD(kernel_ipsec_t, add_policy, status_t,
 	current = this->policies->get(this->policies, policy);
 	if (current)
 	{
+		if (current->reqid != sa->reqid)
+		{
+			DBG1(DBG_CFG, "unable to install policy %R === %R %N (mark "
+				 "%u/0x%08x) for reqid %u, the same policy for reqid %u exists",
+				 src_ts, dst_ts, policy_dir_names, direction,
+				 mark.value, mark.mask, sa->reqid, current->reqid);
+			policy_entry_destroy(this, policy);
+			this->mutex->unlock(this->mutex);
+			return INVALID_STATE;
+		}
 		/* use existing policy */
 		DBG2(DBG_KNL, "policy %R === %R %N  (mark %u/0x%08x) "
 					  "already exists, increasing refcount",
@@ -2382,7 +2396,7 @@ 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 || current->reqid != reqid)
 	{
 		if (mark.value)
 		{
@@ -2405,8 +2419,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
 		enumerator = current->used_by->create_enumerator(current->used_by);
 		while (enumerator->enumerate(enumerator, (void**)&mapping))
 		{
-			if (reqid == mapping->sa->cfg.reqid &&
-				priority == mapping->priority)
+			if (priority == mapping->priority)
 			{
 				current->used_by->remove_at(current->used_by, enumerator);
 				policy_sa_destroy(mapping, &direction, this);