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 20:40:03 +0200 On Fri, May 09, 2025 at 12:48:37PM +0300, Vitaliy Makkoveev wrote: > 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. OK bluhm@ The + default: + break; looks a bit useless. > 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