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 6384162..670e2b5 100644 (file)
--- 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 551a589..5146dcb 100644 (file)
--- 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 bb8b1a6..f8ccdbf 100644 (file)
--- 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 b850216..499bea2 100644 (file)
--- 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);
 }