libreswan/libreswan-5.1-opt-protoport.patch

From b24a5b718303683420d37d0b88e7f52999195e48 Mon Sep 17 00:00:00 2001
From: Ilya Maximets <i.maximets@ovn.org>
Date: Thu, 2 Jan 2025 12:50:38 +0100
Subject: [PATCH 1/2] ttoport: check numbers before the service name lookup

getservbyname() is a very slow function.  It seems to linearly scan all
the values in /etc/services (11473 lines on my system) and compare the
provided string to each and every one of them.  One such lookup takes
around 1.8 milliseconds.  Which is huge in CPU time.

If the ipsec.conf contains a lot of connections, the time multiplies.
For example, a single 'addconn --checkconfig' with an ipsec.conf
containing 1000 connections takes 1.8 seconds.  Most of that time is
just getservbyname() calls.

Let's first check if the port string is a number before looking up the
service.  This brings the total time of parsing a large config from
1.8 seconds down to 40-ish milliseconds in case the port is actually
just a number.

Tested with an ipsec.conf containing 1000 connections with the left
protoport=udp and the right protoport=udp/6081 or udp/geneve.  The
table below shows how much time it takes to execute the
'addconn --checkconfig'.

    protoport      Before     After     SpeedUp
    -------------------------------------------
    udp/6081      1.77 sec   0.04 sec     44x
    udp/geneve    1.37 sec   1.37 sec     --

Times for 'geneve' are very similar before and after, so the change
doesn't affect performance in cases where getservbyname() is needed.

This change allows to save extra 8-10% on 'ipsec add <conn>' calls as
well.  Below are the times for 1000 consequent calls:

    protoport      Before     After     SpeedUp
    -------------------------------------------
    udp/6081      24.5 sec   22.5 sec     8.2%
    udp/geneve    23.9 sec   23.9 sec      --

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Andrew Cagney <cagney@gnu.org>
---
 lib/libswan/ttoport.c | 45 ++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/lib/libswan/ttoport.c b/lib/libswan/ttoport.c
index b1e38a4cd2..7a7083b5d2 100644
--- a/lib/libswan/ttoport.c
+++ b/lib/libswan/ttoport.c
@@ -28,7 +28,27 @@ err_t ttoport(shunk_t service_name, ip_port *port)
 	*port = unset_port;
 
 	/*
-	 * Extract port by trying to resolve it by name.
+	 * Try converting it to a number; use SHUNK's variant of strtoul()
+	 * as it is more strict around using the full string.
+	 *
+	 * Number conversion should be checked first, because the service
+	 * name lookup is very expensive in comparision.
+	 */
+	uintmax_t l;
+	err_t e = shunk_to_uintmax(service_name,
+				   NULL/*trailing-chars-not-allowed*/,
+				   0/*any-base*/, &l);
+	if (e == NULL) {
+		if (l > 0xffff)
+			return "must be between 0 and 65535";
+
+		/* success */
+		*port = ip_hport(l);
+		return NULL;
+	}
+
+	/*
+	 * It's not a number, so trying to resolve it by name.
 	 *
 	 * XXX: the getservbyname() call requires a NUL terminated
 	 * string but SERVICE_NAME, being a shunk_t may not include
@@ -43,25 +63,6 @@ err_t ttoport(shunk_t service_name, ip_port *port)
 		return NULL;
 	}
 
-	/*
-	 * Now try converting it to a number; use SHUNK's variant of
-	 * strtoul() as it is more strict around using the full
-	 * string.
-	 */
-	uintmax_t l;
-	err_t e = shunk_to_uintmax(service_name,
-				   NULL/*trailing-chars-not-allowed*/,
-				   0/*any-base*/, &l);
-	if (e != NULL) {
-		*port = unset_port;
-		return e;
-	}
-
-	if (l > 0xffff) {
-		*port = unset_port;
-		return "must be between 0 and 65535";
-	}
-
-	*port = ip_hport(l);
-	return NULL;
+	/* 'e' still holds a number conversion error. */
+	return e;
 }
-- 
2.48.1


From 8a7043f497e8ea2fb8d066d04ac85a51c99e8056 Mon Sep 17 00:00:00 2001
From: Ilya Maximets <i.maximets@ovn.org>
Date: Wed, 15 Jan 2025 14:17:53 -0500
Subject: [PATCH 2/2] ipsecconf: move protoport parsing to set_whack_end

ttoprotoport() is using a very heavy getservbyname() underneath to
convert protocol names to port numbers.  And this is done for every
connection being loaded.  Move the logic down to set_whack_end(), so
the full validation is only done for connections we're about to add.
This significantly reduces the time required to load large config
files.  For example, running 'addconn --cehckcinfig' on a file with
a 1000 connections with udp/geneve protoport takes 1.4 seconds without
this change and just 0.04 seconds with this change applied.

The downside is that addconn --checkconfig will no longer fully
validate the protocols, but it's already not validating many other
things, and it seems to be a general direction for moving validation
to a single centralized place, which is pluto.

Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Andrew Cagney <cagney@gnu.org>
---
 include/ipsecconf/confread.h    |  1 -
 lib/libipsecconf/confread.c     | 10 ----------
 lib/libipsecconf/confwrite.c    |  6 ++----
 lib/libipsecconf/starterwhack.c |  9 ++++++++-
 4 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/include/ipsecconf/confread.h b/include/ipsecconf/confread.h
index 6e4fcb6136..d290d847ab 100644
--- a/include/ipsecconf/confread.h
+++ b/include/ipsecconf/confread.h
@@ -65,7 +65,6 @@ struct starter_end {
 	ip_address addr;
 	ip_address nexthop;
 	ip_cidr vti_ip;
-	ip_protoport protoport;
 
 	ksf strings;
 	knf options;
diff --git a/lib/libipsecconf/confread.c b/lib/libipsecconf/confread.c
index df8ca3d456..8e681fc2df 100644
--- a/lib/libipsecconf/confread.c
+++ b/lib/libipsecconf/confread.c
@@ -383,16 +383,6 @@ static bool validate_end(struct starter_conn *conn_st,
 		}
 	}
 
-	/* copy certificate path name */
-
-	if (end->set[KSCF_PROTOPORT]) {
-		char *value = end->strings[KSCF_PROTOPORT];
-		err_t ugh = ttoprotoport(value, &end->protoport);
-		if (ugh != NULL)
-			ERR_FOUND("bad %sprotoport=%s [%s]", leftright, value,
-				  ugh);
-	}
-
 	return err;
 #  undef ERR_FOUND
 }
diff --git a/lib/libipsecconf/confwrite.c b/lib/libipsecconf/confwrite.c
index 698091ac2e..354e318e77 100644
--- a/lib/libipsecconf/confwrite.c
+++ b/lib/libipsecconf/confwrite.c
@@ -302,11 +302,9 @@ static void confwrite_side(FILE *out, struct starter_end *end)
 			str_cidr(&end->vti_ip, &as));
 	}
 
-	if (end->protoport.is_set) {
-		protoport_buf buf;
+	if (end->set[KSCF_PROTOPORT])
 		fprintf(out, "\t%sprotoport=%s\n", side,
-			str_protoport(&end->protoport, &buf));
-	}
+			end->strings[KSCF_PROTOPORT]);
 
 	confwrite_int(out, side,
 		      kv_conn | kv_leftright,
diff --git a/lib/libipsecconf/starterwhack.c b/lib/libipsecconf/starterwhack.c
index 3ec055bcb5..365494b7e8 100644
--- a/lib/libipsecconf/starterwhack.c
+++ b/lib/libipsecconf/starterwhack.c
@@ -148,7 +148,14 @@ static bool set_whack_end(struct whack_end *w,
 
 	w->subnets = l->strings[KSCF_SUBNETS];
 	w->host_ikeport = l->options[KNCF_IKEPORT];
-	w->protoport = l->protoport;
+
+	if (l->set[KSCF_PROTOPORT]) {
+		char *value = l->strings[KSCF_PROTOPORT];
+		err_t ugh = ttoprotoport(value, &w->protoport);
+		if (ugh != NULL)
+			llog(RC_LOG, logger, "bad %sprotoport=%s [%s]",
+			     lr, value, ugh);
+	}
 
 	w->cert = l->strings[KSCF_CERT];
 	w->ckaid = l->strings[KSCF_CKAID];
-- 
2.48.1