From: Tobias Brunner <tobias@strongswan.org>
Date: Fri, 1 Jun 2018 09:15:22 +0000 (+0200)
Subject: kernel-pfkey: Avoid updating policies if nothing significant changed
X-Git-Tag: 5.7.0dr5~23
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=50c4c1bb4007923833e9934b8a243de0724949f4;p=thirdparty%2Fstrongswan.git

kernel-pfkey: Avoid updating policies if nothing significant changed

The FreeBSD kernel doesn't update policies atomically, causing
unnecessary traffic loss during simple rekeyings.

Fixes #2677.
---

diff --git a/src/libcharon/plugins/kernel_pfkey/kernel_pfkey_ipsec.c b/src/libcharon/plugins/kernel_pfkey/kernel_pfkey_ipsec.c
index 80c484b479..b937a769ec 100644
--- a/src/libcharon/plugins/kernel_pfkey/kernel_pfkey_ipsec.c
+++ b/src/libcharon/plugins/kernel_pfkey/kernel_pfkey_ipsec.c
@@ -2455,6 +2455,45 @@ static bool install_route(private_kernel_pfkey_ipsec_t *this,
 	}
 }
 
+/**
+ * Check if any significant data has changed to warrant sending an update to
+ * the kernel.
+ */
+static bool policy_update_required(policy_sa_t *current, policy_sa_t *updated)
+{
+	if (current->type != updated->type
+#ifdef HAVE_STRUCT_SADB_X_POLICY_SADB_X_POLICY_PRIORITY
+		|| current->priority != updated->priority
+#endif
+		)
+	{
+		return TRUE;
+	}
+	if (current->type == POLICY_IPSEC)
+	{
+		ipsec_sa_cfg_t *cur = &current->sa->cfg, *upd = &updated->sa->cfg;
+
+		/* we don't use ipsec_sa_cfg_equals() here as e.g. SPIs are not
+		 * relevant for this kernel interface, so we don't have to update the
+		 * policy during a rekeying */
+		if (cur->mode != upd->mode ||
+			cur->reqid != upd->reqid ||
+			cur->esp.use != upd->esp.use ||
+			cur->ah.use != upd->ah.use ||
+			cur->ipcomp.transform != upd->ipcomp.transform)
+		{
+			return TRUE;
+		}
+		if (cur->mode == MODE_TUNNEL &&
+			(!current->sa->src->ip_equals(current->sa->src, updated->sa->src) ||
+			 !current->sa->dst->ip_equals(current->sa->dst, updated->sa->dst)))
+		{
+			return TRUE;
+		}
+	}
+	return FALSE;
+}
+
 /**
  * Add or update a policy in the kernel.
  *
@@ -2629,7 +2668,7 @@ METHOD(kernel_ipsec_t, add_policy, status_t,
 	kernel_ipsec_manage_policy_t *data)
 {
 	policy_entry_t *policy, *found = NULL;
-	policy_sa_t *assigned_sa, *current_sa;
+	policy_sa_t *assigned_sa, *current_sa = NULL;
 	enumerator_t *enumerator;
 	bool update = TRUE;
 
@@ -2692,6 +2731,13 @@ METHOD(kernel_ipsec_t, add_policy, status_t,
 	policy->used_by->insert_before(policy->used_by, enumerator, assigned_sa);
 	enumerator->destroy(enumerator);
 
+	if (update && current_sa)
+	{	/* check if there are actually any relevant changes, if not, we don't
+		 * send an update to the kernel as e.g. FreeBSD doesn't do that
+		 * atomically, causing unecessary traffic loss during rekeyings */
+		update = policy_update_required(current_sa, assigned_sa);
+	}
+
 	if (!update)
 	{	/* we don't update the policy if the priority is lower than that of the
 		 * currently installed one */
@@ -2889,22 +2935,28 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
 		return SUCCESS;
 	}
 	policy->used_by->remove(policy->used_by, to_remove, NULL);
-	mapping = to_remove;
 
 	if (policy->used_by->get_count(policy->used_by) > 0)
 	{	/* policy is used by more SAs, keep in kernel */
 		DBG2(DBG_KNL, "policy still used by another CHILD_SA, not removed");
-		policy_sa_destroy(mapping, id->dir, this);
+
+		if (is_installed)
+		{	/* check if there are actually any relevant changes, if not, we do
+			 * not send an update to the kernel as e.g. FreeBSD doesn't do that
+			 * atomically, causing unecessary traffic loss during rekeyings */
+			policy->used_by->get_first(policy->used_by, (void**)&mapping);
+			is_installed = policy_update_required(mapping, to_remove);
+		}
+		policy_sa_destroy(to_remove, id->dir, this);
 
 		if (!is_installed)
-		{	/* no need to update as the policy was not installed for this SA */
+		{	/* no need to update as the policy */
 			this->mutex->unlock(this->mutex);
 			return SUCCESS;
 		}
 
 		DBG2(DBG_KNL, "updating policy %R === %R %N", id->src_ts, id->dst_ts,
 			 policy_dir_names, id->dir);
-		policy->used_by->get_first(policy->used_by, (void**)&mapping);
 		if (add_policy_internal(this, policy, mapping, TRUE) != SUCCESS)
 		{
 			DBG1(DBG_KNL, "unable to update policy %R === %R %N",
@@ -2926,7 +2978,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
 	pol->sadb_x_policy_exttype = SADB_X_EXT_POLICY;
 	pol->sadb_x_policy_len = PFKEY_LEN(sizeof(struct sadb_x_policy));
 	pol->sadb_x_policy_dir = dir2kernel(id->dir);
-	pol->sadb_x_policy_type = type2kernel(mapping->type);
+	pol->sadb_x_policy_type = type2kernel(to_remove->type);
 	PFKEY_EXT_ADD(msg, pol);
 
 	add_addr_ext(msg, policy->src.net, SADB_EXT_ADDRESS_SRC, policy->src.proto,
@@ -2949,7 +3001,7 @@ METHOD(kernel_ipsec_t, del_policy, status_t,
 	}
 
 	this->policies->remove(this->policies, found, NULL);
-	policy_sa_destroy(mapping, id->dir, this);
+	policy_sa_destroy(to_remove, id->dir, this);
 	policy_entry_destroy(policy, this);
 	this->mutex->unlock(this->mutex);