To cache lookups, the policy ipo is linked to its SA tdb. There

is also a list of SAs that belong to a policy.  To make it MP safe,
protect these pointers with a mutex.
tested by Hrvoje Popovski; OK mvs@

diff --git a/sys/net/pfkeyv2.c b/sys/net/pfkeyv2.c
index 670e2b5..289243b 100644 (file)
--- a/sys/net/pfkeyv2.c
+++ b/sys/net/pfkeyv2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pfkeyv2.c,v 1.227 2021/12/08 14:24:18 bluhm Exp $ */
+/* $OpenBSD: pfkeyv2.c,v 1.228 2021/12/14 17:50:37 bluhm Exp $ */
 
 /*
  *     @(#)COPYRIGHT   1.1 (NRL) 17 January 1995
@@ -2004,12 +2004,15 @@ pfkeyv2_send(struct socket *so, void *message, int len)
                                (caddr_t)&ipo->ipo_mask, rnh,
                                ipo->ipo_nodes, 0)) == NULL) {
                                /* Remove from linked list of policies on TDB */
+                               mtx_enter(&ipo_tdb_mtx);
                                if (ipo->ipo_tdb != NULL) {
                                        TAILQ_REMOVE(
                                            &ipo->ipo_tdb->tdb_policy_head,
                                            ipo, ipo_tdb_next);
                                        tdb_unref(ipo->ipo_tdb);
+                                       ipo->ipo_tdb = NULL;
                                }
+                               mtx_leave(&ipo_tdb_mtx);
                                if (ipo->ipo_ids)
                                        ipsp_ids_free(ipo->ipo_ids);
                                pool_put(&ipsec_policy_pool, ipo);

diff --git a/sys/netinet/ip_ipsp.c b/sys/netinet/ip_ipsp.c
index 971112e..f13566b 100644 (file)
--- a/sys/netinet/ip_ipsp.c
+++ b/sys/netinet/ip_ipsp.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ip_ipsp.c,v 1.264 2021/12/11 16:33:47 bluhm Exp $     */
+/*     $OpenBSD: ip_ipsp.c,v 1.265 2021/12/14 17:50:37 bluhm Exp $     */
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -934,12 +934,14 @@ tdb_cleanspd(struct tdb *tdbp)
 {
        struct ipsec_policy *ipo;
 
+       mtx_enter(&ipo_tdb_mtx);
        while ((ipo = TAILQ_FIRST(&tdbp->tdb_policy_head)) != NULL) {
                TAILQ_REMOVE(&tdbp->tdb_policy_head, ipo, ipo_tdb_next);
                tdb_unref(ipo->ipo_tdb);
                ipo->ipo_tdb = NULL;
                ipo->ipo_last_searched = 0; /* Force a re-search. */
        }
+       mtx_leave(&ipo_tdb_mtx);
 }
 
 void

diff --git a/sys/netinet/ip_ipsp.h b/sys/netinet/ip_ipsp.h
index fd822cf..b04b8dd 100644 (file)
--- a/sys/netinet/ip_ipsp.h
+++ b/sys/netinet/ip_ipsp.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ip_ipsp.h,v 1.230 2021/12/11 16:33:47 bluhm Exp $     */
+/*     $OpenBSD: ip_ipsp.h,v 1.231 2021/12/14 17:50:37 bluhm Exp $     */
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -257,6 +257,10 @@ struct ipsec_acquire {
        TAILQ_ENTRY(ipsec_acquire)      ipa_next;
 };
 
+/*
+ * Locks used to protect struct members in this file:
+ *     p       ipo_tdb_mtx             link policy to TDB global mutex
+ */
 struct ipsec_policy {
        struct radix_node       ipo_nodes[2];   /* radix tree glue */
        struct sockaddr_encap   ipo_addr;
@@ -274,7 +278,7 @@ struct ipsec_policy {
                                                 * mode was used.
                                                 */
 
-       u_int64_t               ipo_last_searched;      /* Timestamp of last lookup */
+       u_int64_t       ipo_last_searched;      /* [p] Timestamp of lookup */
 
        u_int8_t                ipo_flags;      /* See IPSP_POLICY_* definitions */
        u_int8_t                ipo_type;       /* USE/ACQUIRE/... */
@@ -283,7 +287,7 @@ struct ipsec_policy {
 
        int                     ipo_ref_count;
 
-       struct tdb              *ipo_tdb;               /* Cached entry */
+       struct tdb              *ipo_tdb;       /* [p] Cached TDB entry */
 
        struct ipsec_ids        *ipo_ids;
 
@@ -313,8 +317,9 @@ struct ipsec_policy {
  * Locks used to protect struct members in this file:
  *     I       immutable after creation
  *     N       net lock
- *     s       tdb_sadb_mtx
  *     m       tdb_mtx
+ *     p       ipo_tdb_mtx             link policy to TDB global mutex
+ *     s       tdb_sadb_mtx            SA database global mutex
  */
 struct tdb {                           /* tunnel descriptor block */
        /*
@@ -438,7 +443,7 @@ struct tdb {                                /* tunnel descriptor block */
        struct sockaddr_encap   tdb_filter; /* What traffic is acceptable */
        struct sockaddr_encap   tdb_filtermask; /* And the mask */
 
-       TAILQ_HEAD(tdb_policy_head, ipsec_policy)       tdb_policy_head;
+       TAILQ_HEAD(tdb_policy_head, ipsec_policy) tdb_policy_head; /* [p] */
        TAILQ_ENTRY(tdb)        tdb_sync_entry;
 };
 #define tdb_ipackets           tdb_data.tdd_ipackets
@@ -546,6 +551,7 @@ extern char ipsec_def_comp[];
 extern TAILQ_HEAD(ipsec_policy_head, ipsec_policy) ipsec_policy_head;
 
 extern struct mutex tdb_sadb_mtx;
+extern struct mutex ipo_tdb_mtx;
 
 struct cryptop;
 

diff --git a/sys/netinet/ip_spd.c b/sys/netinet/ip_spd.c
index b1c49b2..087d63e 100644 (file)
--- a/sys/netinet/ip_spd.c
+++ b/sys/netinet/ip_spd.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ip_spd.c,v 1.108 2021/12/03 17:18:34 bluhm Exp $ */
+/* $OpenBSD: ip_spd.c,v 1.109 2021/12/14 17:50:37 bluhm Exp $ */
 /*
  * The author of this code is Angelos D. Keromytis (angelos@cis.upenn.edu)
  *
@@ -53,6 +53,12 @@ void ipsp_delete_acquire(struct ipsec_acquire *);
 struct pool ipsec_policy_pool;
 struct pool ipsec_acquire_pool;
 
+/*
+ * For tdb_walk() calling tdb_delete_locked() we need lock order
+ * tdb_sadb_mtx before ipo_tdb_mtx.
+ */
+struct mutex ipo_tdb_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);
+
 /* Protected by the NET_LOCK(). */
 struct radix_node_head **spd_tables;
 unsigned int spd_table_max;
@@ -360,6 +366,7 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
        }
 
        /* Do we have a cached entry ? If so, check if it's still valid. */
+       mtx_enter(&ipo_tdb_mtx);
        if (ipo->ipo_tdb != NULL &&
            (ipo->ipo_tdb->tdb_flags & TDBF_INVALID)) {
                TAILQ_REMOVE(&ipo->ipo_tdb->tdb_policy_head, ipo,
@@ -367,6 +374,7 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                tdb_unref(ipo->ipo_tdb);
                ipo->ipo_tdb = NULL;
        }
+       mtx_leave(&ipo_tdb_mtx);
 
        /* Outgoing packet policy check. */
        if (direction == IPSP_DIRECTION_OUT) {
@@ -393,6 +401,7 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                        ids = ipsp_ids_lookup(ipsecflowinfo);
 
                /* Check that the cached TDB (if present), is appropriate. */
+               mtx_enter(&ipo_tdb_mtx);
                if (ipo->ipo_tdb != NULL) {
                        if ((ipo->ipo_last_searched <= ipsec_last_added) ||
                            (ipo->ipo_sproto != ipo->ipo_tdb->tdb_sproto) ||
@@ -407,7 +416,9 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                                goto nomatchout;
 
                        /* Cached entry is good. */
-                       return ipsp_spd_inp(m, inp, ipo, tdbout);
+                       error = ipsp_spd_inp(m, inp, ipo, tdbout);
+                       mtx_leave(&ipo_tdb_mtx);
+                       return error;
 
   nomatchout:
                        /* Cached TDB was not good. */
@@ -428,23 +439,38 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                 * explosion in the number of acquired SAs).
                 */
                if (ipo->ipo_last_searched <= ipsec_last_added) {
+                       struct tdb *tdbp_new;
+
                        /* "Touch" the entry. */
                        if (dignore == 0)
                                ipo->ipo_last_searched = getuptime();
 
+                       /* gettdb() takes tdb_sadb_mtx, preserve lock order */
+                       mtx_leave(&ipo_tdb_mtx);
                        /* Find an appropriate SA from the existing ones. */
-                       ipo->ipo_tdb = gettdbbydst(rdomain,
+                       tdbp_new = gettdbbydst(rdomain,
                            dignore ? &sdst : &ipo->ipo_dst,
                            ipo->ipo_sproto, ids ? ids: ipo->ipo_ids,
                            &ipo->ipo_addr, &ipo->ipo_mask);
+                       mtx_enter(&ipo_tdb_mtx);
+                       if (ipo->ipo_tdb != NULL) {
+                               /* Remove cached TDB from parallel thread. */
+                               TAILQ_REMOVE(&ipo->ipo_tdb->tdb_policy_head,
+                                   ipo, ipo_tdb_next);
+                               tdb_unref(ipo->ipo_tdb);
+                       }
+                       ipo->ipo_tdb = tdbp_new;
                        if (ipo->ipo_tdb != NULL) {
                                /* gettdbbydst() has already refcounted tdb */
                                TAILQ_INSERT_TAIL(
                                    &ipo->ipo_tdb->tdb_policy_head,
                                    ipo, ipo_tdb_next);
-                               return ipsp_spd_inp(m, inp, ipo, tdbout);
+                               error = ipsp_spd_inp(m, inp, ipo, tdbout);
+                               mtx_leave(&ipo_tdb_mtx);
+                               return error;
                        }
                }
+               mtx_leave(&ipo_tdb_mtx);
 
                /* So, we don't have an SA -- just a policy. */
                switch (ipo->ipo_type) {
@@ -490,8 +516,13 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                                tdbp = tdbp->tdb_inext;
 
                        /* Direct match in the cache. */
-                       if (ipo->ipo_tdb == tdbp)
-                               return ipsp_spd_inp(m, inp, ipo, tdbout);
+                       mtx_enter(&ipo_tdb_mtx);
+                       if (ipo->ipo_tdb == tdbp) {
+                               error = ipsp_spd_inp(m, inp, ipo, tdbout);
+                               mtx_leave(&ipo_tdb_mtx);
+                               return error;
+                       }
+                       mtx_leave(&ipo_tdb_mtx);
 
                        if (memcmp(dignore ? &ssrc : &ipo->ipo_dst,
                            &tdbp->tdb_src, tdbp->tdb_src.sa.sa_len) ||
@@ -505,6 +536,7 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                                        goto nomatchin;
 
                        /* Add it to the cache. */
+                       mtx_enter(&ipo_tdb_mtx);
                        if (ipo->ipo_tdb != NULL) {
                                TAILQ_REMOVE(&ipo->ipo_tdb->tdb_policy_head,
                                    ipo, ipo_tdb_next);
@@ -513,13 +545,16 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                        ipo->ipo_tdb = tdb_ref(tdbp);
                        TAILQ_INSERT_TAIL(&tdbp->tdb_policy_head, ipo,
                            ipo_tdb_next);
-                       return ipsp_spd_inp(m, inp, ipo, tdbout);
+                       error = ipsp_spd_inp(m, inp, ipo, tdbout);
+                       mtx_leave(&ipo_tdb_mtx);
+                       return error;
 
   nomatchin: /* Nothing needed here, falling through */
        ;
                }
 
                /* Check whether cached entry applies. */
+               mtx_enter(&ipo_tdb_mtx);
                if (ipo->ipo_tdb != NULL) {
                        /*
                         * We only need to check that the correct
@@ -543,13 +578,25 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
 
                /* Find whether there exists an appropriate SA. */
                if (ipo->ipo_last_searched <= ipsec_last_added) {
+                       struct tdb *tdbp_new;
+
                        if (dignore == 0)
                                ipo->ipo_last_searched = getuptime();
 
-                       ipo->ipo_tdb = gettdbbysrc(rdomain,
+                       /* gettdb() takes tdb_sadb_mtx, preserve lock order */
+                       mtx_leave(&ipo_tdb_mtx);
+                       tdbp_new = gettdbbysrc(rdomain,
                            dignore ? &ssrc : &ipo->ipo_dst,
                            ipo->ipo_sproto, ipo->ipo_ids,
                            &ipo->ipo_addr, &ipo->ipo_mask);
+                       mtx_enter(&ipo_tdb_mtx);
+                       if (ipo->ipo_tdb != NULL) {
+                               /* Remove cached TDB from parallel thread. */
+                               TAILQ_REMOVE(&ipo->ipo_tdb->tdb_policy_head,
+                                   ipo, ipo_tdb_next);
+                               tdb_unref(ipo->ipo_tdb);
+                       }
+                       ipo->ipo_tdb = tdbp_new;
                        if (ipo->ipo_tdb != NULL) {
                                /* gettdbbysrc() has already refcounted tdb */
                                TAILQ_INSERT_TAIL(
@@ -558,6 +605,7 @@ ipsp_spd_lookup(struct mbuf *m, int af, int hlen, int direction,
                        }
                }
   skipinputsearch:
+               mtx_leave(&ipo_tdb_mtx);
 
                switch (ipo->ipo_type) {
                case IPSP_IPSEC_REQUIRE:
@@ -615,12 +663,14 @@ ipsec_delete_policy(struct ipsec_policy *ipo)
            rn_delete(&ipo->ipo_addr, &ipo->ipo_mask, rnh, rn) == NULL)
                return (ESRCH);
 
+       mtx_enter(&ipo_tdb_mtx);
        if (ipo->ipo_tdb != NULL) {
                TAILQ_REMOVE(&ipo->ipo_tdb->tdb_policy_head, ipo,
                    ipo_tdb_next);
                tdb_unref(ipo->ipo_tdb);
                ipo->ipo_tdb = NULL;
        }
+       mtx_leave(&ipo_tdb_mtx);
 
        while ((ipa = TAILQ_FIRST(&ipo->ipo_acquires)) != NULL)
                ipsp_delete_acquire(ipa);
@@ -825,10 +875,9 @@ ipsp_spd_inp(struct mbuf *m, struct inpcb *inp, struct ipsec_policy *ipo,
 
  justreturn:
        if (tdbout != NULL) {
-               if (ipo != NULL) {
-                       tdb_ref(ipo->ipo_tdb);
-                       *tdbout = ipo->ipo_tdb;
-               } else
+               if (ipo != NULL)
+                       *tdbout = tdb_ref(ipo->ipo_tdb);
+               else
                        *tdbout = NULL;
        }
        return 0;