From 5ff2eaede31cf3297904ea7b93a6cb46b25021f7 Mon Sep 17 00:00:00 2001 From: Daiki Ueno Date: Thu, 23 Jan 2025 14:13:48 +0900 Subject: [PATCH] Speed up parsing protoport configuration Resolves: RHEL-74850 Signed-off-by: Daiki Ueno --- libreswan-5.1-opt-protoport.patch | 214 ++++++++++++++++++++++++++++++ libreswan.spec | 1 + 2 files changed, 215 insertions(+) create mode 100644 libreswan-5.1-opt-protoport.patch diff --git a/libreswan-5.1-opt-protoport.patch b/libreswan-5.1-opt-protoport.patch new file mode 100644 index 0000000..bb418bd --- /dev/null +++ b/libreswan-5.1-opt-protoport.patch @@ -0,0 +1,214 @@ +From b24a5b718303683420d37d0b88e7f52999195e48 Mon Sep 17 00:00:00 2001 +From: Ilya Maximets +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 ' 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 +Signed-off-by: Andrew Cagney +--- + 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 +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 +Signed-off-by: Andrew Cagney +--- + 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 + diff --git a/libreswan.spec b/libreswan.spec index f0feec4..c3c3091 100644 --- a/libreswan.spec +++ b/libreswan.spec @@ -46,6 +46,7 @@ Source5: https://download.libreswan.org/cavs/ikev2.fax.bz2 Patch1: libreswan-4.15-ipsec_import.patch Patch2: libreswan-5.1-rereadsecrets.patch +Patch3: libreswan-5.1-opt-protoport.patch BuildRequires: audit-libs-devel BuildRequires: bison