[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.
We have cases where we do assignment only once, like `unp_addr’
when we bind(2)ing socket and we don’t modify if until socket’s
destruction. Since we could connect to successfully bind(2)ed
socket only we could say within unp_connect() that `unp_addr’
could be accessed without lock.
We also have the case where we could bind(2) already connected
socket. I such case we could say `unp_addr’ is atomic because
this is the pointer assignment which points to read-only data
and this data immutable until socket’s destruction.
But `unp_addr’ is still mutable.
> - union sockaddr_union tdb_dst; /* Destination address */
> - union sockaddr_union tdb_src; /* Source address */
> + union sockaddr_union tdb_dst; /* [IN] Destination address */
> + union sockaddr_union tdb_src; /* [IN] Source address */
> On 8 Dec 2021, at 02:06, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
>
> Hi,
>
> I have started documenting the locking strategy of struct tdb fields.
> Note that gettdb_dir() is MP safe now.
>
> In udpencap_ctlinput() we had an unprotected access to tdb_snext.
> Grab the tdb_sadb_mtx mutex there. Make the braces consistently
> for all these loops.
>
> Move NET_ASSERT_LOCKED() into the functions where the read access
> happens.
>
> 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 7 Dec 2021 22:39:50 -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; /* [IN] Destination address */
> + union sockaddr_union tdb_src; /* [IN] 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);
> }
>
>
