From: Alexander Bluhm Subject: Re: ipsec: move ipsec-enc-alg, ipsec-auth-alg and ipsec-comp-alg sysctl variables out of netlock To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Fri, 9 May 2025 17:59:50 +0200 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; > > mtx_enter(&pfkeyv2_mtx); > *seq = pfkeyv2_seq++; > @@ -2239,6 +2240,10 @@ pfkeyv2_acquire(struct ipsec_policy *ipo > > sadb_comb = p; > > + 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); > + > /* XXX Should actually ask the crypto layer what's supported */ > for (j = 0; j < sa_prop->sadb_prop_num; j++) { > sadb_comb->sadb_comb_flags = 0; > @@ -2246,75 +2251,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 2 May 2025 17:44:18 -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 2 May 2025 17:44:18 -0000 > @@ -139,9 +139,44 @@ 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}, > +}; > + > +#define IPSEC_SYSCTL_ENC_ALGS_NUM \ > + (sizeof(ipsec_sysctl_enc_algs) / sizeof(ipsec_sysctl_enc_algs[0])) > + > +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}, > +}; > + > +#define IPSEC_SYSCTL_AUTH_ALGS_NUM \ > + (sizeof(ipsec_sysctl_auth_algs) / sizeof(ipsec_sysctl_auth_algs[0])) > + > +const struct ipsec_sysctl_algorithm ipsec_sysctl_comp_algs[] = { > + {"deflate", IPSEC_COMP_DEFLATE}, > +}; > + > +#define IPSEC_SYSCTL_COMP_ALGS_NUM \ > + (sizeof(ipsec_sysctl_comp_algs) / sizeof(ipsec_sysctl_comp_algs[0])) I would terminate the arrays with a NULL name. Then we don't have to track the index and length. > + > +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 +193,7 @@ const struct sysctl_bounded_args ipsecct > { IPSEC_FIRSTUSE, &ipsec_exp_first_use, 0, INT_MAX }, > }; > > +int ipsec_sysctl_algorythm(int, void *, size_t *, void *, size_t); Typo: algorythm -> algorithm > 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 +207,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 +645,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_algorythm(name[0], oldp, oldlenp, > + newp, newlen)); > case IPCTL_IPSEC_STATS: > return (ipsec_sysctl_ipsecstat(oldp, oldlenp, newp)); > default: > @@ -639,6 +658,74 @@ ipsec_sysctl(int *name, u_int namelen, v > NET_UNLOCK(); > return (error); > } > +} > + > +int > +ipsec_sysctl_algorythm(int name, void *oldp, size_t *oldlenp, > + void *newp, size_t newlen) > +{ > + int *var, oldval, error; > + size_t i; > + char buf[20]; > + > + const struct ipsec_sysctl_algorithm *algs; > + size_t algs_num; > + > + switch (name) { > + case IPCTL_IPSEC_ENC_ALGORITHM: > + algs = ipsec_sysctl_enc_algs; > + algs_num = IPSEC_SYSCTL_ENC_ALGS_NUM; > + var = &ipsec_def_enc; > + break; > + case IPCTL_IPSEC_AUTH_ALGORITHM: > + algs = ipsec_sysctl_auth_algs; > + algs_num = IPSEC_SYSCTL_AUTH_ALGS_NUM; > + var = &ipsec_def_auth; > + break; > + case IPCTL_IPSEC_IPCOMP_ALGORITHM: > + algs = ipsec_sysctl_comp_algs; > + algs_num = IPSEC_SYSCTL_COMP_ALGS_NUM; > + var = &ipsec_def_comp; > + break; > + default: > + return (EOPNOTSUPP); > + } > + > + oldval = atomic_load_int(var); > + > + for (i=0; i < algs_num; ++i) { > + if (algs[i].val == oldval) { > + strlcpy(buf, algs[i].name, sizeof(buf)); > + break; > + } > + } Here I would use const struct ipsec_sysctl_algorithm *p; for (p = algs; p->name != NULL; p++) { > + > + KASSERT(i < algs_num); > + > + 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 (i = 0; i < algs_num; ++i) { > + if (strncasecmp(buf, algs[i].name, buflen) == 0) > + break; > + } > + > + if (i >= algs_num) > + return (EINVAL); > + > + if (algs[i].val != oldval) > + atomic_store_int(var, algs[i].val); > + } > + > + return (0); > } > > int