From bdf4635962d13ca33cf9b5a9907b6cdc067c01f7 Mon Sep 17 00:00:00 2001 From: bluhm Date: Tue, 14 Dec 2021 17:50:37 +0000 Subject: [PATCH] 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@ --- sys/net/pfkeyv2.c | 5 ++- sys/netinet/ip_ipsp.c | 4 ++- sys/netinet/ip_ipsp.h | 16 +++++++--- sys/netinet/ip_spd.c | 73 ++++++++++++++++++++++++++++++++++++------- 4 files changed, 79 insertions(+), 19 deletions(-) diff --git a/sys/net/pfkeyv2.c b/sys/net/pfkeyv2.c index 670e2b53ab4..289243bfdf3 100644 --- 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 971112ea86b..f13566b2d87 100644 --- 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 fd822cfb9f8..b04b8dd9d73 100644 --- 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 b1c49b21247..087d63ef6c2 100644 --- 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; -- 2.20.1