From: Alexander Bluhm Subject: Re: ipsec: move bunch of `ipsecctl_vars' variables out of netlock To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Mon, 12 May 2025 14:34:47 +0200 On Sat, May 10, 2025 at 04:17:22AM +0300, Vitaliy Makkoveev wrote: > The `ipsec_soft_allocations', `ipsec_exp_allocations', > `ipsec_soft_bytes', `ipsec_exp_bytes', `ipsec_soft_timeout', > `ipsec_exp_timeout', `ipsec_soft_first_use' and `ipsec_exp_first_use' > are local to pfkeyv2_acquire(). They are loaded together during > `sadb_comb' initialization, so make sense to unlock them together too. > > It looks reasonable to drop 'ipsec_' prefix for local variables, > otherwise the names are too long, and I think this should made code > reading worse. For consistency I dropped 'ipsec_' prefix for > `ipsec_def_*_local' too. > > The naming is discussable. OK bluhm@ > Index: sys/net/pfkeyv2.c > =================================================================== > RCS file: /cvs/src/sys/net/pfkeyv2.c,v > retrieving revision 1.265 > diff -u -p -r1.265 pfkeyv2.c > --- sys/net/pfkeyv2.c 9 May 2025 19:53:41 -0000 1.265 > +++ sys/net/pfkeyv2.c 10 May 2025 00:40:50 -0000 > @@ -2159,11 +2159,27 @@ pfkeyv2_acquire(struct ipsec_policy *ipo > int i, j, registered; > > #ifdef IPSEC > - int ipsec_def_enc_local, ipsec_def_comp_local, ipsec_def_auth_local; > + int def_enc_local, def_comp_local, def_auth_local; > + int soft_allocations_local, exp_allocations_local; > + int soft_bytes_local, exp_bytes_local; > + int soft_timeout_local, exp_timeout_local; > + int soft_first_use_local, exp_first_use_local; > + > + def_enc_local = atomic_load_int(&ipsec_def_enc); > + def_comp_local = atomic_load_int(&ipsec_def_comp); > + def_auth_local = atomic_load_int(&ipsec_def_auth); > + > + soft_allocations_local = atomic_load_int(&ipsec_soft_allocations); > + exp_allocations_local = atomic_load_int(&ipsec_exp_allocations); > > - 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); > + soft_bytes_local = atomic_load_int(&ipsec_soft_bytes); > + exp_bytes_local = atomic_load_int(&ipsec_exp_bytes); > + > + soft_timeout_local = atomic_load_int(&ipsec_soft_timeout); > + exp_timeout_local = atomic_load_int(&ipsec_exp_timeout); > + > + soft_first_use_local = atomic_load_int(&ipsec_soft_first_use); > + exp_first_use_local = atomic_load_int(&ipsec_exp_first_use); > #endif > > mtx_enter(&pfkeyv2_mtx); > @@ -2256,7 +2272,7 @@ pfkeyv2_acquire(struct ipsec_policy *ipo > > if (ipo->ipo_sproto == IPPROTO_ESP) { > /* Set the encryption algorithm */ > - switch(ipsec_def_enc_local) { > + switch(def_enc_local) { > case IPSEC_ENC_AES: > sadb_comb->sadb_comb_encrypt = SADB_X_EALG_AES; > sadb_comb->sadb_comb_encrypt_minbits = 128; > @@ -2288,7 +2304,7 @@ pfkeyv2_acquire(struct ipsec_policy *ipo > } > } else if (ipo->ipo_sproto == IPPROTO_IPCOMP) { > /* Set the compression algorithm */ > - switch(ipsec_def_comp_local) { > + switch(def_comp_local) { > case IPSEC_COMP_DEFLATE: > sadb_comb->sadb_comb_encrypt = > SADB_X_CALG_DEFLATE; > @@ -2299,7 +2315,7 @@ pfkeyv2_acquire(struct ipsec_policy *ipo > } > > /* Set the authentication algorithm */ > - switch(ipsec_def_auth_local) { > + switch(def_auth_local) { > case IPSEC_AUTH_HMAC_SHA1: > sadb_comb->sadb_comb_auth = SADB_AALG_SHA1HMAC; > sadb_comb->sadb_comb_auth_minbits = 160; > @@ -2332,17 +2348,17 @@ pfkeyv2_acquire(struct ipsec_policy *ipo > break; > } > > - sadb_comb->sadb_comb_soft_allocations = ipsec_soft_allocations; > - sadb_comb->sadb_comb_hard_allocations = ipsec_exp_allocations; > + sadb_comb->sadb_comb_soft_allocations = soft_allocations_local; > + sadb_comb->sadb_comb_hard_allocations = exp_allocations_local; > > - sadb_comb->sadb_comb_soft_bytes = ipsec_soft_bytes; > - sadb_comb->sadb_comb_hard_bytes = ipsec_exp_bytes; > + sadb_comb->sadb_comb_soft_bytes = soft_bytes_local; > + sadb_comb->sadb_comb_hard_bytes = exp_bytes_local; > > - sadb_comb->sadb_comb_soft_addtime = ipsec_soft_timeout; > - sadb_comb->sadb_comb_hard_addtime = ipsec_exp_timeout; > + sadb_comb->sadb_comb_soft_addtime = soft_timeout_local; > + sadb_comb->sadb_comb_hard_addtime = exp_timeout_local; > > - sadb_comb->sadb_comb_soft_usetime = ipsec_soft_first_use; > - sadb_comb->sadb_comb_hard_usetime = ipsec_exp_first_use; > + sadb_comb->sadb_comb_soft_usetime = soft_first_use_local; > + sadb_comb->sadb_comb_hard_usetime = exp_first_use_local; > #endif > sadb_comb++; > } > Index: sys/netinet/ipsec_input.c > =================================================================== > RCS file: /cvs/src/sys/netinet/ipsec_input.c,v > retrieving revision 1.210 > diff -u -p -r1.210 ipsec_input.c > --- sys/netinet/ipsec_input.c 9 May 2025 19:53:41 -0000 1.210 > +++ sys/netinet/ipsec_input.c 10 May 2025 00:40:50 -0000 > @@ -108,14 +108,14 @@ void ipsec_common_ctlinput(u_int, int, s > int encdebug = 0; > int ipsec_keep_invalid = IPSEC_DEFAULT_EMBRYONIC_SA_TIMEOUT; > int ipsec_require_pfs = IPSEC_DEFAULT_PFS; > -int ipsec_soft_allocations = IPSEC_DEFAULT_SOFT_ALLOCATIONS; > -int ipsec_exp_allocations = IPSEC_DEFAULT_EXP_ALLOCATIONS; > -int ipsec_soft_bytes = IPSEC_DEFAULT_SOFT_BYTES; > -int ipsec_exp_bytes = IPSEC_DEFAULT_EXP_BYTES; > -int ipsec_soft_timeout = IPSEC_DEFAULT_SOFT_TIMEOUT; > -int ipsec_exp_timeout = IPSEC_DEFAULT_EXP_TIMEOUT; > -int ipsec_soft_first_use = IPSEC_DEFAULT_SOFT_FIRST_USE; > -int ipsec_exp_first_use = IPSEC_DEFAULT_EXP_FIRST_USE; > +int ipsec_soft_allocations = IPSEC_DEFAULT_SOFT_ALLOCATIONS; /* [a] */ > +int ipsec_exp_allocations = IPSEC_DEFAULT_EXP_ALLOCATIONS; /* [a] */ > +int ipsec_soft_bytes = IPSEC_DEFAULT_SOFT_BYTES; /* [a] */ > +int ipsec_exp_bytes = IPSEC_DEFAULT_EXP_BYTES; /* [a] */ > +int ipsec_soft_timeout = IPSEC_DEFAULT_SOFT_TIMEOUT; /* [a] */ > +int ipsec_exp_timeout = IPSEC_DEFAULT_EXP_TIMEOUT; /* [a] */ > +int ipsec_soft_first_use = IPSEC_DEFAULT_SOFT_FIRST_USE; /* [a] */ > +int ipsec_exp_first_use = IPSEC_DEFAULT_EXP_FIRST_USE; /* [a] */ > int ipsec_expire_acquire = IPSEC_DEFAULT_EXPIRE_ACQUIRE; > > int esp_enable = 1; > @@ -172,17 +172,20 @@ int ipsec_def_enc = IPSEC_ENC_AES; /* [ > int ipsec_def_auth = IPSEC_AUTH_HMAC_SHA1; /* [a] */ > int ipsec_def_comp = IPSEC_COMP_DEFLATE; /* [a] */ > > -const struct sysctl_bounded_args ipsecctl_vars[] = { > +const struct sysctl_bounded_args ipsecctl_vars_locked[] = { > { IPSEC_ENCDEBUG, &encdebug, 0, 1 }, > { IPSEC_EXPIRE_ACQUIRE, &ipsec_expire_acquire, 0, INT_MAX }, > { IPSEC_EMBRYONIC_SA_TIMEOUT, &ipsec_keep_invalid, 0, INT_MAX }, > { IPSEC_REQUIRE_PFS, &ipsec_require_pfs, 0, 1 }, > +}; > + > +const struct sysctl_bounded_args ipsecctl_vars[] = { > { IPSEC_SOFT_ALLOCATIONS, &ipsec_soft_allocations, 0, INT_MAX }, > { IPSEC_ALLOCATIONS, &ipsec_exp_allocations, 0, INT_MAX }, > { IPSEC_SOFT_BYTES, &ipsec_soft_bytes, 0, INT_MAX }, > { IPSEC_BYTES, &ipsec_exp_bytes, 0, INT_MAX }, > { IPSEC_TIMEOUT, &ipsec_exp_timeout, 0, INT_MAX }, > - { IPSEC_SOFT_TIMEOUT, &ipsec_soft_timeout,0, INT_MAX }, > + { IPSEC_SOFT_TIMEOUT, &ipsec_soft_timeout, 0, INT_MAX }, > { IPSEC_SOFT_FIRSTUSE, &ipsec_soft_first_use, 0, INT_MAX }, > { IPSEC_FIRSTUSE, &ipsec_exp_first_use, 0, INT_MAX }, > }; > @@ -645,12 +648,19 @@ ipsec_sysctl(int *name, u_int namelen, v > newp, newlen)); > case IPCTL_IPSEC_STATS: > return (ipsec_sysctl_ipsecstat(oldp, oldlenp, newp)); > - default: > + case IPSEC_ENCDEBUG: > + case IPSEC_EXPIRE_ACQUIRE: > + case IPSEC_EMBRYONIC_SA_TIMEOUT: > + case IPSEC_REQUIRE_PFS: > NET_LOCK(); > - error = sysctl_bounded_arr(ipsecctl_vars, nitems(ipsecctl_vars), > - name, namelen, oldp, oldlenp, newp, newlen); > + error = sysctl_bounded_arr(ipsecctl_vars_locked, > + nitems(ipsecctl_vars_locked), name, namelen, > + oldp, oldlenp, newp, newlen); > NET_UNLOCK(); > return (error); > + default: > + return (sysctl_bounded_arr(ipsecctl_vars, nitems(ipsecctl_vars), > + name, namelen, oldp, oldlenp, newp, newlen)); > } > } >