ok mvs@
> On 8 Dec 2021, at 16:41, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
>
> On Wed, Dec 08, 2021 at 02:41:47AM +0300, Vitaliy Makkoveev wrote:
>> [IN] looks strange. If this field modified after creation it is
>> mutable. There are cases when such field could modified only once,
>> but it still is atomic.
>
> You are right. tdb_dst and tdb_src are initialized before the TDB
> is linked to the list. They may be modified under the exclusive
> net lock. So from a user perspective, net lock is needed to access
> these fields and [N] is the correct documentation.
>
> ok?
>
> bluhm
>
> Index: net/pfkeyv2.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/pfkeyv2.c,v
> retrieving revision 1.226
> diff -u -p -r1.226 pfkeyv2.c
> --- net/pfkeyv2.c 3 Dec 2021 19:04:49 -0000 1.226
> +++ net/pfkeyv2.c 7 Dec 2021 22:16:20 -0000
> @@ -800,6 +800,8 @@ pfkeyv2_get(struct tdb *tdb, void **head
> int rval, i;
> void *p;
>
> + NET_ASSERT_LOCKED();
> +
> /* Find how much space we need */
> i = sizeof(struct sadb_sa) + sizeof(struct sadb_lifetime) +
> sizeof(struct sadb_x_counter);
> @@ -2347,6 +2349,8 @@ pfkeyv2_expire(struct tdb *tdb, u_int16_
> struct sadb_msg *smsg;
> int rval = 0;
> int i;
> +
> + NET_ASSERT_LOCKED();
>
> switch (tdb->tdb_sproto) {
> case IPPROTO_AH:
> Index: netinet/ip_ipsp.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v
> retrieving revision 1.262
> diff -u -p -r1.262 ip_ipsp.c
> --- netinet/ip_ipsp.c 7 Dec 2021 17:28:46 -0000 1.262
> +++ netinet/ip_ipsp.c 7 Dec 2021 22:39:59 -0000
> @@ -243,8 +243,6 @@ reserve_spi(u_int rdomain, u_int32_t ssp
> u_int32_t spi;
> int nums;
>
> - NET_ASSERT_LOCKED();
> -
> /* Don't accept ranges only encompassing reserved SPIs. */
> if (sproto != IPPROTO_IPCOMP &&
> (tspi < sspi || tspi <= SPI_RESERVED_MAX)) {
> @@ -343,6 +341,8 @@ gettdb_dir(u_int rdomain, u_int32_t spi,
> u_int32_t hashval;
> struct tdb *tdbp;
>
> + NET_ASSERT_LOCKED();
> +
> mtx_enter(&tdb_sadb_mtx);
> hashval = tdb_hash(spi, dst, proto);
>
> @@ -374,7 +374,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> mtx_enter(&tdb_sadb_mtx);
> hashval = tdb_hash(0, src, proto);
>
> - for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
> + for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
> if (tdbp->tdb_sproto == proto &&
> (spi == 0 || tdbp->tdb_spi == spi) &&
> ((!reverse && tdbp->tdb_rdomain == rdomain) ||
> @@ -384,7 +384,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> !memcmp(&tdbp->tdb_dst, dst, dst->sa.sa_len)) &&
> !memcmp(&tdbp->tdb_src, src, src->sa.sa_len))
> break;
> -
> + }
> if (tdbp != NULL) {
> tdb_ref(tdbp);
> mtx_leave(&tdb_sadb_mtx);
> @@ -395,7 +395,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> su_null.sa.sa_len = sizeof(struct sockaddr);
> hashval = tdb_hash(0, &su_null, proto);
>
> - for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
> + for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
> if (tdbp->tdb_sproto == proto &&
> (spi == 0 || tdbp->tdb_spi == spi) &&
> ((!reverse && tdbp->tdb_rdomain == rdomain) ||
> @@ -405,7 +405,7 @@ gettdbbysrcdst_dir(u_int rdomain, u_int3
> !memcmp(&tdbp->tdb_dst, dst, dst->sa.sa_len)) &&
> tdbp->tdb_src.sa.sa_family == AF_UNSPEC)
> break;
> -
> + }
> tdb_ref(tdbp);
> mtx_leave(&tdb_sadb_mtx);
> return tdbp;
> @@ -494,7 +494,7 @@ gettdbbysrc(u_int rdomain, union sockadd
> mtx_enter(&tdb_sadb_mtx);
> hashval = tdb_hash(0, src, sproto);
>
> - for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext)
> + for (tdbp = tdbsrc[hashval]; tdbp != NULL; tdbp = tdbp->tdb_snext) {
> if ((tdbp->tdb_sproto == sproto) &&
> (tdbp->tdb_rdomain == rdomain) &&
> ((tdbp->tdb_flags & TDBF_INVALID) == 0) &&
> @@ -504,7 +504,7 @@ gettdbbysrc(u_int rdomain, union sockadd
> continue;
> break;
> }
> -
> + }
> tdb_ref(tdbp);
> mtx_leave(&tdb_sadb_mtx);
> return tdbp;
> @@ -900,8 +900,7 @@ tdb_unlink_locked(struct tdb *tdbp)
>
> if (tdbsrc[hashval] == tdbp) {
> tdbsrc[hashval] = tdbp->tdb_snext;
> - }
> - else {
> + } else {
> for (tdbpp = tdbsrc[hashval]; tdbpp != NULL;
> tdbpp = tdbpp->tdb_snext) {
> if (tdbpp->tdb_snext == tdbp) {
> @@ -1031,8 +1030,6 @@ struct tdb *
> tdb_alloc(u_int rdomain)
> {
> struct tdb *tdbp;
> -
> - NET_ASSERT_LOCKED();
>
> tdbp = pool_get(&tdb_pool, PR_WAITOK | PR_ZERO);
>
> Index: netinet/ip_ipsp.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.h,v
> retrieving revision 1.228
> diff -u -p -r1.228 ip_ipsp.h
> --- netinet/ip_ipsp.h 7 Dec 2021 17:28:46 -0000 1.228
> +++ netinet/ip_ipsp.h 8 Dec 2021 13:37:16 -0000
> @@ -309,6 +309,12 @@ struct ipsec_policy {
> #define IPSP_IDENTITY_USERFQDN 3
> #define IPSP_IDENTITY_ASN1_DN 4
>
> +/*
> + * Locks used to protect struct members in this file:
> + * I immutable after creation
> + * N net lock
> + * s tdb_sadb_mtx
> + */
> struct tdb { /* tunnel descriptor block */
> /*
> * Each TDB is on three hash tables: one keyed on dst/spi/sproto,
> @@ -318,9 +324,9 @@ struct tdb { /* tunnel
> descriptor blo
> * policy matching. The following three fields maintain the hash
> * queues in those three tables.
> */
> - struct tdb *tdb_hnext; /* dst/spi/sproto table */
> - struct tdb *tdb_dnext; /* dst/sproto table */
> - struct tdb *tdb_snext; /* src/sproto table */
> + struct tdb *tdb_hnext; /* [s] dst/spi/sproto table */
> + struct tdb *tdb_dnext; /* [s] dst/sproto table */
> + struct tdb *tdb_snext; /* [s] src/sproto table */
> struct tdb *tdb_inext;
> struct tdb *tdb_onext;
>
> @@ -390,17 +396,17 @@ struct tdb { /* tunnel
> descriptor blo
> struct tdb_data tdb_data; /* stats about this TDB */
> u_int64_t tdb_cryptoid; /* Crypto session ID */
>
> - u_int32_t tdb_spi; /* SPI */
> + u_int32_t tdb_spi; /* [I] SPI */
> u_int16_t tdb_amxkeylen; /* Raw authentication key length */
> u_int16_t tdb_emxkeylen; /* Raw encryption key length */
> u_int16_t tdb_ivlen; /* IV length */
> - u_int8_t tdb_sproto; /* IPsec protocol */
> + u_int8_t tdb_sproto; /* [I] IPsec protocol */
> u_int8_t tdb_wnd; /* Replay window */
> u_int8_t tdb_satype; /* SA type (RFC2367, PF_KEY) */
> u_int8_t tdb_updates; /* pfsync update counter */
>
> - union sockaddr_union tdb_dst; /* Destination address */
> - union sockaddr_union tdb_src; /* Source address */
> + union sockaddr_union tdb_dst; /* [N] Destination address */
> + union sockaddr_union tdb_src; /* [N] Source address */
>
> u_int8_t *tdb_amxkey; /* Raw authentication key */
> u_int8_t *tdb_emxkey; /* Raw encryption key */
> @@ -424,8 +430,8 @@ struct tdb { /* tunnel
> descriptor blo
> u_int16_t tdb_tag; /* Packet filter tag */
> u_int32_t tdb_tap; /* Alternate enc(4) interface */
>
> - u_int tdb_rdomain; /* Routing domain */
> - u_int tdb_rdomain_post; /* Change domain */
> + u_int tdb_rdomain; /* [I] Routing domain */
> + u_int tdb_rdomain_post; /* [I] Change domain */
>
> struct sockaddr_encap tdb_filter; /* What traffic is acceptable */
> struct sockaddr_encap tdb_filtermask; /* And the mask */
> Index: netinet/ipsec_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ipsec_input.c,v
> retrieving revision 1.196
> diff -u -p -r1.196 ipsec_input.c
> --- netinet/ipsec_input.c 2 Dec 2021 13:46:42 -0000 1.196
> +++ netinet/ipsec_input.c 7 Dec 2021 21:09:49 -0000
> @@ -934,15 +934,16 @@ udpencap_ctlinput(int cmd, struct sockad
>
> first = gettdbbysrcdst_rev(rdomain, 0, su_src, su_dst, IPPROTO_ESP);
>
> + mtx_enter(&tdb_sadb_mtx);
> for (tdbp = first; tdbp != NULL; tdbp = tdbp->tdb_snext) {
> if (tdbp->tdb_sproto == IPPROTO_ESP &&
> ((tdbp->tdb_flags & (TDBF_INVALID|TDBF_UDPENCAP)) ==
> TDBF_UDPENCAP) &&
> !memcmp(&tdbp->tdb_dst, &dst, su_dst->sa.sa_len) &&
> - !memcmp(&tdbp->tdb_src, &src, su_src->sa.sa_len)) {
> + !memcmp(&tdbp->tdb_src, &src, su_src->sa.sa_len))
> ipsec_set_mtu(tdbp, mtu);
> - }
> }
> + mtx_leave(&tdb_sadb_mtx);
> tdb_unref(first);
> }
>
>
