Download raw body.
ipsec: move ipsec-enc-alg, ipsec-auth-alg and ipsec-comp-alg sysctl variables out of netlock
ipsec: move ipsec-enc-alg, ipsec-auth-alg and ipsec-comp-alg sysctl variables out of netlock
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
ipsec: move ipsec-enc-alg, ipsec-auth-alg and ipsec-comp-alg sysctl variables out of netlock
ipsec: move ipsec-enc-alg, ipsec-auth-alg and ipsec-comp-alg sysctl variables out of netlock