From: bluhm Date: Wed, 8 Dec 2021 14:24:18 +0000 (+0000) Subject: Start documenting the locking strategy of struct tdb fields. Note X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=8cdd540df05cac055e318e9355117b0c35a93f94;p=openbsd Start documenting the locking strategy of struct tdb fields. Note that gettdb_dir() is MP safe now. Add the tdb_sadb_mtx mutex in udpencap_ctlinput() to protect the access to tdb_snext. Make the braces consistently for all these TDB loops. Move NET_ASSERT_LOCKED() into the functions where the read access happens. OK mvs@ --- diff --git a/sys/net/pfkeyv2.c b/sys/net/pfkeyv2.c index 6384162d0e4..670e2b53ab4 100644 --- a/sys/net/pfkeyv2.c +++ b/sys/net/pfkeyv2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pfkeyv2.c,v 1.226 2021/12/03 19:04:49 tobhe Exp $ */ +/* $OpenBSD: pfkeyv2.c,v 1.227 2021/12/08 14:24:18 bluhm Exp $ */ /* * @(#)COPYRIGHT 1.1 (NRL) 17 January 1995 @@ -800,6 +800,8 @@ pfkeyv2_get(struct tdb *tdb, void **headers, void **buffer, int *lenp, 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); @@ -2348,6 +2350,8 @@ pfkeyv2_expire(struct tdb *tdb, u_int16_t type) int rval = 0; int i; + NET_ASSERT_LOCKED(); + switch (tdb->tdb_sproto) { case IPPROTO_AH: case IPPROTO_ESP: diff --git a/sys/netinet/ip_ipsp.c b/sys/netinet/ip_ipsp.c index 551a589631e..5146dcbf248 100644 --- a/sys/netinet/ip_ipsp.c +++ b/sys/netinet/ip_ipsp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_ipsp.c,v 1.262 2021/12/07 17:28:46 bluhm Exp $ */ +/* $OpenBSD: ip_ipsp.c,v 1.263 2021/12/08 14:24:18 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr), @@ -243,8 +243,6 @@ reserve_spi(u_int rdomain, u_int32_t sspi, u_int32_t tspi, 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, union sockaddr_union *dst, 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_int32_t spi, union sockaddr_union *src, 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_int32_t spi, union sockaddr_union *src, !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_int32_t spi, union sockaddr_union *src, 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_int32_t spi, union sockaddr_union *src, !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 sockaddr_union *src, u_int8_t sproto, 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 sockaddr_union *src, u_int8_t sproto, 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) { @@ -1032,8 +1031,6 @@ tdb_alloc(u_int rdomain) { struct tdb *tdbp; - NET_ASSERT_LOCKED(); - tdbp = pool_get(&tdb_pool, PR_WAITOK | PR_ZERO); refcnt_init(&tdbp->tdb_refcnt); diff --git a/sys/netinet/ip_ipsp.h b/sys/netinet/ip_ipsp.h index bb8b1a6fb29..f8ccdbf5c24 100644 --- a/sys/netinet/ip_ipsp.h +++ b/sys/netinet/ip_ipsp.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_ipsp.h,v 1.228 2021/12/07 17:28:46 bluhm Exp $ */ +/* $OpenBSD: ip_ipsp.h,v 1.229 2021/12/08 14:24:18 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr), @@ -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 block */ * 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 block */ 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 block */ 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 */ diff --git a/sys/netinet/ipsec_input.c b/sys/netinet/ipsec_input.c index b850216bbc3..499bea22f79 100644 --- a/sys/netinet/ipsec_input.c +++ b/sys/netinet/ipsec_input.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ipsec_input.c,v 1.196 2021/12/02 13:46:42 bluhm Exp $ */ +/* $OpenBSD: ipsec_input.c,v 1.197 2021/12/08 14:24:18 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr) and @@ -934,15 +934,16 @@ udpencap_ctlinput(int cmd, struct sockaddr *sa, u_int rdomain, void *v) 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); }