On Fri, May 09, 2025 at 05:59:50PM +0200, Alexander Bluhm wrote:
> On Fri, May 02, 2025 at 08:45:49PM +0300, Vitaliy Makkoveev wrote:
> > I propose to use integers instead of strings for in-kernel
> > representation. So we can update them atomically. Also we remove
> > strcmp() from the hot (?) path.
> > 
> > Note, this diff changes behavior for a little. Against previous it
> > denies to set incorrect values. In such case the EINVAL error will be
> > returned to userland.
> 
> I like it, a few comments inline.
> 
> > Index: sys/net/pfkeyv2.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/pfkeyv2.c,v
> > retrieving revision 1.264
> > diff -u -p -r1.264 pfkeyv2.c
> > --- sys/net/pfkeyv2.c	11 Mar 2025 15:31:03 -0000	1.264
> > +++ sys/net/pfkeyv2.c	2 May 2025 17:44:18 -0000
> > @@ -2157,6 +2157,7 @@ pfkeyv2_acquire(struct ipsec_policy *ipo
> >  	struct sadb_msg *smsg;
> >  	int rval = 0;
> >  	int i, j, registered;
> > +	int ipsec_def_enc_local, ipsec_def_comp_local, ipsec_def_auth_local;
> 
> I know that you like the _local suffix.  I usually remove the ipsec_
> prefix.  This results in much shorter names.  But anyway, just a
> suggestion.
> 	int def_enc, def_comp, def_auth;
> 

This makes me 100% sure that this is the cached value of global
variable. I agree, the `ipsec_def_*_local' are long enough, so may be we
will rethink the naming later.

Also added missing "#ifdef IPSEC" around them.

> Here I would use
> 
> 	const struct ipsec_sysctl_algorithm *p;
> 	for (p = algs; p->name != NULL; p++) {
>

No objections for this.

Index: sys/net/pfkeyv2.c
===================================================================
RCS file: /cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.264
diff -u -p -r1.264 pfkeyv2.c
--- sys/net/pfkeyv2.c	11 Mar 2025 15:31:03 -0000	1.264
+++ sys/net/pfkeyv2.c	9 May 2025 17:44:38 -0000
@@ -2158,6 +2158,14 @@ pfkeyv2_acquire(struct ipsec_policy *ipo
 	int rval = 0;
 	int i, j, registered;
 
+#ifdef IPSEC
+	int ipsec_def_enc_local, ipsec_def_comp_local, ipsec_def_auth_local;
+
+	ipsec_def_enc_local = atomic_load_int(&ipsec_def_enc);
+	ipsec_def_comp_local = atomic_load_int(&ipsec_def_comp);
+	ipsec_def_auth_local = atomic_load_int(&ipsec_def_auth);
+#endif
+
 	mtx_enter(&pfkeyv2_mtx);
 	*seq = pfkeyv2_seq++;
 
@@ -2246,75 +2254,88 @@ pfkeyv2_acquire(struct ipsec_policy *ipo
 		if (ipsec_require_pfs)
 			sadb_comb->sadb_comb_flags |= SADB_SAFLAGS_PFS;
 
-		/* Set the encryption algorithm */
 		if (ipo->ipo_sproto == IPPROTO_ESP) {
-			if (!strncasecmp(ipsec_def_enc, "aes",
-			    sizeof("aes"))) {
+			/* Set the encryption algorithm */
+			switch(ipsec_def_enc_local) {
+			case IPSEC_ENC_AES:
 				sadb_comb->sadb_comb_encrypt = SADB_X_EALG_AES;
 				sadb_comb->sadb_comb_encrypt_minbits = 128;
 				sadb_comb->sadb_comb_encrypt_maxbits = 256;
-			} else if (!strncasecmp(ipsec_def_enc, "aesctr",
-			    sizeof("aesctr"))) {
-				sadb_comb->sadb_comb_encrypt = SADB_X_EALG_AESCTR;
-				sadb_comb->sadb_comb_encrypt_minbits = 128+32;
-				sadb_comb->sadb_comb_encrypt_maxbits = 256+32;
-			} else if (!strncasecmp(ipsec_def_enc, "3des",
-			    sizeof("3des"))) {
-				sadb_comb->sadb_comb_encrypt = SADB_EALG_3DESCBC;
+				break;
+			case IPSEC_ENC_AESCTR:
+				sadb_comb->sadb_comb_encrypt =
+				    SADB_X_EALG_AESCTR;
+				sadb_comb->sadb_comb_encrypt_minbits = 128 + 32;
+				sadb_comb->sadb_comb_encrypt_maxbits = 256 + 32;
+				break;
+			case IPSEC_ENC_3DES:
+				sadb_comb->sadb_comb_encrypt =
+				    SADB_EALG_3DESCBC;
 				sadb_comb->sadb_comb_encrypt_minbits = 192;
 				sadb_comb->sadb_comb_encrypt_maxbits = 192;
-			} else if (!strncasecmp(ipsec_def_enc, "blowfish",
-			    sizeof("blowfish"))) {
+				break;
+			case IPSEC_ENC_BLOWFISH:
 				sadb_comb->sadb_comb_encrypt = SADB_X_EALG_BLF;
 				sadb_comb->sadb_comb_encrypt_minbits = 40;
-				sadb_comb->sadb_comb_encrypt_maxbits = BLF_MAXKEYLEN * 8;
-			} else if (!strncasecmp(ipsec_def_enc, "cast128",
-			    sizeof("cast128"))) {
+				sadb_comb->sadb_comb_encrypt_maxbits =
+				    BLF_MAXKEYLEN * 8;
+				break;
+			case IPSEC_ENC_CAST128:
 				sadb_comb->sadb_comb_encrypt = SADB_X_EALG_CAST;
 				sadb_comb->sadb_comb_encrypt_minbits = 40;
 				sadb_comb->sadb_comb_encrypt_maxbits = 128;
+				break;
+			default:
+				break;
 			}
 		} else if (ipo->ipo_sproto == IPPROTO_IPCOMP) {
 			/* Set the compression algorithm */
-			if (!strncasecmp(ipsec_def_comp, "deflate",
-			    sizeof("deflate"))) {
-				sadb_comb->sadb_comb_encrypt = SADB_X_CALG_DEFLATE;
+			switch(ipsec_def_comp_local) {
+			case IPSEC_COMP_DEFLATE:
+				sadb_comb->sadb_comb_encrypt =
+				    SADB_X_CALG_DEFLATE;
 				sadb_comb->sadb_comb_encrypt_minbits = 0;
 				sadb_comb->sadb_comb_encrypt_maxbits = 0;
+				break;
+			default:
+				break;
 			}
 		}
 
 		/* Set the authentication algorithm */
-		if (!strncasecmp(ipsec_def_auth, "hmac-sha1",
-		    sizeof("hmac-sha1"))) {
+		switch(ipsec_def_auth_local) {
+		case IPSEC_AUTH_HMAC_SHA1:
 			sadb_comb->sadb_comb_auth = SADB_AALG_SHA1HMAC;
 			sadb_comb->sadb_comb_auth_minbits = 160;
 			sadb_comb->sadb_comb_auth_maxbits = 160;
-		} else if (!strncasecmp(ipsec_def_auth, "hmac-ripemd160",
-		    sizeof("hmac_ripemd160"))) {
+			break;
+		case IPSEC_AUTH_HMAC_RIPEMD160:
 			sadb_comb->sadb_comb_auth = SADB_X_AALG_RIPEMD160HMAC;
 			sadb_comb->sadb_comb_auth_minbits = 160;
 			sadb_comb->sadb_comb_auth_maxbits = 160;
-		} else if (!strncasecmp(ipsec_def_auth, "hmac-md5",
-		    sizeof("hmac-md5"))) {
+			break;
+		case IPSEC_AUTH_MD5:
 			sadb_comb->sadb_comb_auth = SADB_AALG_MD5HMAC;
 			sadb_comb->sadb_comb_auth_minbits = 128;
 			sadb_comb->sadb_comb_auth_maxbits = 128;
-		} else if (!strncasecmp(ipsec_def_auth, "hmac-sha2-256",
-		    sizeof("hmac-sha2-256"))) {
+			break;
+		case IPSEC_AUTH_SHA2_256:
 			sadb_comb->sadb_comb_auth = SADB_X_AALG_SHA2_256;
 			sadb_comb->sadb_comb_auth_minbits = 256;
 			sadb_comb->sadb_comb_auth_maxbits = 256;
-		} else if (!strncasecmp(ipsec_def_auth, "hmac-sha2-384",
-		    sizeof("hmac-sha2-384"))) {
+			break;
+		case IPSEC_AUTH_SHA2_384:
 			sadb_comb->sadb_comb_auth = SADB_X_AALG_SHA2_384;
 			sadb_comb->sadb_comb_auth_minbits = 384;
 			sadb_comb->sadb_comb_auth_maxbits = 384;
-		} else if (!strncasecmp(ipsec_def_auth, "hmac-sha2-512",
-		    sizeof("hmac-sha2-512"))) {
+			break;
+		case IPSEC_AUTH_SHA2_512:
 			sadb_comb->sadb_comb_auth = SADB_X_AALG_SHA2_512;
 			sadb_comb->sadb_comb_auth_minbits = 512;
 			sadb_comb->sadb_comb_auth_maxbits = 512;
+			break;
+		default:
+			break;
 		}
 
 		sadb_comb->sadb_comb_soft_allocations = ipsec_soft_allocations;
Index: sys/netinet/ip_ipsp.h
===================================================================
RCS file: /cvs/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.248
diff -u -p -r1.248 ip_ipsp.h
--- sys/netinet/ip_ipsp.h	2 Mar 2025 21:28:32 -0000	1.248
+++ sys/netinet/ip_ipsp.h	9 May 2025 17:44:38 -0000
@@ -573,9 +573,30 @@ extern int ipsec_exp_first_use;		/* seco
 #define IPSEC_FIRSTUSE			IPCTL_IPSEC_FIRSTUSE		/* 24 */
 #define IPSEC_MAXID	25
 
-extern char ipsec_def_enc[];
-extern char ipsec_def_auth[];
-extern char ipsec_def_comp[];
+enum {
+	IPSEC_ENC_AES,
+	IPSEC_ENC_AESCTR,
+	IPSEC_ENC_3DES,
+	IPSEC_ENC_BLOWFISH,
+	IPSEC_ENC_CAST128,
+};
+
+enum {
+	IPSEC_AUTH_HMAC_SHA1,
+	IPSEC_AUTH_HMAC_RIPEMD160,
+	IPSEC_AUTH_MD5,
+	IPSEC_AUTH_SHA2_256,
+	IPSEC_AUTH_SHA2_384,
+	IPSEC_AUTH_SHA2_512,
+};
+
+enum {
+	IPSEC_COMP_DEFLATE,
+};
+
+extern int ipsec_def_enc;
+extern int ipsec_def_auth;
+extern int ipsec_def_comp;
 
 extern TAILQ_HEAD(ipsec_policy_head, ipsec_policy) ipsec_policy_head;
 
Index: sys/netinet/ipsec_input.c
===================================================================
RCS file: /cvs/src/sys/netinet/ipsec_input.c,v
retrieving revision 1.209
diff -u -p -r1.209 ipsec_input.c
--- sys/netinet/ipsec_input.c	4 Mar 2025 15:11:30 -0000	1.209
+++ sys/netinet/ipsec_input.c	9 May 2025 17:44:38 -0000
@@ -139,9 +139,38 @@ struct cpumem *ahcounters;
 struct cpumem *ipcompcounters;
 struct cpumem *ipseccounters;
 
-char ipsec_def_enc[20];
-char ipsec_def_auth[20];
-char ipsec_def_comp[20];
+struct ipsec_sysctl_algorithm {
+	const char *name;
+	int val;
+};
+
+const struct ipsec_sysctl_algorithm ipsec_sysctl_enc_algs[] = {
+	{"aes",			IPSEC_ENC_AES},
+	{"aesctr",		IPSEC_ENC_AESCTR},
+	{"3des",		IPSEC_ENC_3DES},
+	{"blowfish",		IPSEC_ENC_BLOWFISH},
+	{"cast128",		IPSEC_ENC_CAST128},
+	{NULL,			-1},
+};
+
+const struct ipsec_sysctl_algorithm ipsec_sysctl_auth_algs[] = {
+	{"hmac-sha1",		IPSEC_AUTH_HMAC_SHA1},
+	{"hmac-ripemd160",	IPSEC_AUTH_HMAC_RIPEMD160},
+	{"hmac-md5",		IPSEC_AUTH_MD5},
+	{"hmac-sha2-256",	IPSEC_AUTH_SHA2_256},
+	{"hmac-sha2-384",	IPSEC_AUTH_SHA2_384},
+	{"hmac-sha2-512",	IPSEC_AUTH_SHA2_512},
+	{NULL,			-1},
+};
+
+const struct ipsec_sysctl_algorithm ipsec_sysctl_comp_algs[] = {
+	{"deflate",		IPSEC_COMP_DEFLATE},
+	{NULL,			-1},
+};
+
+int ipsec_def_enc = IPSEC_ENC_AES;		/* [a] */
+int ipsec_def_auth = IPSEC_AUTH_HMAC_SHA1;	/* [a] */
+int ipsec_def_comp = IPSEC_COMP_DEFLATE;	/* [a] */
 
 const struct sysctl_bounded_args ipsecctl_vars[] = {
 	{ IPSEC_ENCDEBUG, &encdebug, 0, 1 },
@@ -158,6 +187,7 @@ const struct sysctl_bounded_args ipsecct
 	{ IPSEC_FIRSTUSE, &ipsec_exp_first_use, 0, INT_MAX },
 };
 
+int ipsec_sysctl_algorithm(int, void *, size_t *, void *, size_t);
 int esp_sysctl_espstat(void *, size_t *, void *);
 int ah_sysctl_ahstat(void *, size_t *, void *);
 int ipcomp_sysctl_ipcompstat(void *, size_t *, void *);
@@ -171,10 +201,6 @@ ipsec_init(void)
 	ipcompcounters = counters_alloc(ipcomps_ncounters);
 	ipseccounters = counters_alloc(ipsec_ncounters);
 
-	strlcpy(ipsec_def_enc, IPSEC_DEFAULT_DEF_ENC, sizeof(ipsec_def_enc));
-	strlcpy(ipsec_def_auth, IPSEC_DEFAULT_DEF_AUTH, sizeof(ipsec_def_auth));
-	strlcpy(ipsec_def_comp, IPSEC_DEFAULT_DEF_COMP, sizeof(ipsec_def_comp));
-
 	ipsp_init();
 }
 
@@ -613,23 +639,10 @@ ipsec_sysctl(int *name, u_int namelen, v
 
 	switch (name[0]) {
 	case IPCTL_IPSEC_ENC_ALGORITHM:
-		NET_LOCK();
-		error = sysctl_tstring(oldp, oldlenp, newp, newlen,
-		    ipsec_def_enc, sizeof(ipsec_def_enc));
-		NET_UNLOCK();
-		return (error);
 	case IPCTL_IPSEC_AUTH_ALGORITHM:
-		NET_LOCK();
-		error = sysctl_tstring(oldp, oldlenp, newp, newlen,
-		    ipsec_def_auth, sizeof(ipsec_def_auth));
-		NET_UNLOCK();
-		return (error);
 	case IPCTL_IPSEC_IPCOMP_ALGORITHM:
-		NET_LOCK();
-		error = sysctl_tstring(oldp, oldlenp, newp, newlen,
-		    ipsec_def_comp, sizeof(ipsec_def_comp));
-		NET_UNLOCK();
-		return (error);
+		return (ipsec_sysctl_algorithm(name[0], oldp, oldlenp,
+		    newp, newlen));
 	case IPCTL_IPSEC_STATS:
 		return (ipsec_sysctl_ipsecstat(oldp, oldlenp, newp));
 	default:
@@ -639,6 +652,68 @@ ipsec_sysctl(int *name, u_int namelen, v
 		NET_UNLOCK();
 		return (error);
 	}
+}
+
+int
+ipsec_sysctl_algorithm(int name, void *oldp, size_t *oldlenp,
+    void *newp, size_t newlen)
+{
+	const struct ipsec_sysctl_algorithm *algs, *p; 
+	int *var, oldval, error;
+	char buf[20];
+
+	switch (name) {
+	case IPCTL_IPSEC_ENC_ALGORITHM:
+		algs = ipsec_sysctl_enc_algs;
+		var = &ipsec_def_enc;
+		break;
+	case IPCTL_IPSEC_AUTH_ALGORITHM:
+		algs = ipsec_sysctl_auth_algs;
+		var = &ipsec_def_auth;
+		break;
+	case IPCTL_IPSEC_IPCOMP_ALGORITHM:
+		algs = ipsec_sysctl_comp_algs;
+		var = &ipsec_def_comp;
+		break;
+	default:
+		return (EOPNOTSUPP);
+	}
+
+	oldval = atomic_load_int(var);
+
+	for (p = algs; p->name != NULL; p++) {
+		if (p->val == oldval) {
+			strlcpy(buf, p->name, sizeof(buf));
+			break;
+		}
+	}
+
+	KASSERT(p->name != NULL);
+
+	error = sysctl_tstring(oldp, oldlenp, newp, newlen,
+	    buf, sizeof(buf));
+	if (error)
+		return (error);
+
+	if (newp) {
+		size_t buflen;
+
+		if ((buflen = strlen(buf)) == 0)
+			return (EINVAL);
+
+		for (p = algs; p->name != NULL; p++) {
+			if (strncasecmp(buf, p->name, buflen) == 0)
+				break;
+		}
+
+		if (p->name == NULL)
+			return (EINVAL);
+
+		if (p->val != oldval)
+			atomic_store_int(var, p->val);
+	}
+
+	return (0);
 }
 
 int