Hrvoje has hit a crash with IPsec acquire while testing the parallel

IP forwarding diff.  Add mutex and refcount to make memory management
of struct ipsec_acquire MP safe.
testing Hrvoje Popovski; input sashan@; OK mvs@

3 files changed, 80 insertions, 35 deletions

diff --git a/sys/net/pfkeyv2.c b/sys/net/pfkeyv2.c
index e578b67b942..9121fad2492 100644
--- a/sys/net/pfkeyv2.c
+++ b/sys/net/pfkeyv2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pfkeyv2.c,v 1.232 2022/03/08 22:30:38 bluhm Exp $ */
+/* $OpenBSD: pfkeyv2.c,v 1.233 2022/03/13 21:38:32 bluhm Exp $ */
 
 /*
  *	@(#)COPYRIGHT	1.1 (NRL) 17 January 1995
@@ -1591,7 +1591,8 @@ pfkeyv2_send(struct socket *so, void *message, int len)
 	case SADB_X_ASKPOLICY:
 		/* Get the relevant policy */
 		NET_LOCK();
-		ipa = ipsec_get_acquire(((struct sadb_x_policy *) headers[SADB_X_EXT_POLICY])->sadb_x_policy_seq);
+		ipa = ipsec_get_acquire(((struct sadb_x_policy *)
+		    headers[SADB_X_EXT_POLICY])->sadb_x_policy_seq);
 		if (ipa == NULL) {
 			rval = ESRCH;
 			NET_UNLOCK();
@@ -1600,6 +1601,7 @@ pfkeyv2_send(struct socket *so, void *message, int len)
 
 		rval = pfkeyv2_policy(ipa, headers, &freeme, &freeme_sz);
 		NET_UNLOCK();
+		ipsec_unref_acquire(ipa);
 		if (rval)
 			mode = PFKEYV2_SENDMESSAGE_UNICAST;
 
diff --git a/sys/netinet/ip_ipsp.h b/sys/netinet/ip_ipsp.h
index 4610727bf04..c697994047b 100644
--- a/sys/netinet/ip_ipsp.h
+++ b/sys/netinet/ip_ipsp.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_ipsp.h,v 1.236 2022/03/08 22:30:38 bluhm Exp $	*/
+/*	$OpenBSD: ip_ipsp.h,v 1.237 2022/03/13 21:38:32 bluhm Exp $	*/
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -45,6 +45,7 @@
  *	I	immutable after creation
  *	a	atomic operations
  *	N	net lock
+ *	A	ipsec_acquire_mtx
  *	F	ipsec_flows_mtx
  *	m	tdb_mtx			fields of struct tdb
  *	p	ipo_tdb_mtx		link policy to TDB global mutex
@@ -251,12 +252,15 @@ struct ipsec_acquire {
 	u_int32_t			ipa_seq;
 	struct sockaddr_encap		ipa_info;
 	struct sockaddr_encap		ipa_mask;
+	struct refcnt			ipa_refcnt;
 	struct timeout			ipa_timeout;
-	struct ipsec_policy		*ipa_policy;
-	TAILQ_ENTRY(ipsec_acquire)	ipa_ipo_next;
-	TAILQ_ENTRY(ipsec_acquire)	ipa_next;
+	struct ipsec_policy		*ipa_policy;	/* [A] back pointer */
+	TAILQ_ENTRY(ipsec_acquire)	ipa_ipo_next;	/* [A] per policy */
+	TAILQ_ENTRY(ipsec_acquire)	ipa_next;	/* [A] global list */
 };
 
+TAILQ_HEAD(ipsec_acquire_head, ipsec_acquire);
+
 struct ipsec_policy {
 	struct radix_node	ipo_nodes[2];	/* radix tree glue */
 	struct sockaddr_encap	ipo_addr;
@@ -287,9 +291,9 @@ struct ipsec_policy {
 
 	struct ipsec_ids	*ipo_ids;
 
-	TAILQ_HEAD(ipo_acquires_head, ipsec_acquire) ipo_acquires; /* List of acquires */
-	TAILQ_ENTRY(ipsec_policy)	ipo_tdb_next;	/* List TDB policies */
-	TAILQ_ENTRY(ipsec_policy)	ipo_list;	/* List of all policies */
+	struct ipsec_acquire_head ipo_acquires;	/* [A] List of acquires */
+	TAILQ_ENTRY(ipsec_policy) ipo_tdb_next;	/* [p] List TDB policies */
+	TAILQ_ENTRY(ipsec_policy) ipo_list;	/* List of all policies */
 };
 
 #define	IPSP_POLICY_NONE	0x0000	/* No flags set */
@@ -682,6 +686,7 @@ ssize_t	ipsec_hdrsz(struct tdb *);
 void	ipsec_adjust_mtu(struct mbuf *, u_int32_t);
 void	ipsec_set_mtu(struct tdb *, u_int32_t);
 struct	ipsec_acquire *ipsec_get_acquire(u_int32_t);
+void	ipsec_unref_acquire(struct ipsec_acquire *);
 int	ipsec_forward_check(struct mbuf *, int, int);
 int	ipsec_local_check(struct mbuf *, int, int, int);
 
diff --git a/sys/netinet/ip_spd.c b/sys/netinet/ip_spd.c
index a3eddb19b5d..70f085f11fa 100644
--- a/sys/netinet/ip_spd.c
+++ b/sys/netinet/ip_spd.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ip_spd.c,v 1.114 2022/03/08 22:30:38 bluhm Exp $ */
+/* $OpenBSD: ip_spd.c,v 1.115 2022/03/13 21:38:32 bluhm Exp $ */
 /*
  * The author of this code is Angelos D. Keromytis (angelos@cis.upenn.edu)
  *
@@ -43,10 +43,11 @@ int	ipsp_spd_inp(struct mbuf *, struct inpcb *, struct ipsec_policy *,
 	    struct tdb **);
 int	ipsp_acquire_sa(struct ipsec_policy *, union sockaddr_union *,
 	    union sockaddr_union *, struct sockaddr_encap *, struct mbuf *);
-struct	ipsec_acquire *ipsp_pending_acquire(struct ipsec_policy *,
-	    union sockaddr_union *);
-void	ipsp_delete_acquire_timo(void *);
+int	ipsp_pending_acquire(struct ipsec_policy *, union sockaddr_union *);
+void	ipsp_delete_acquire_timer(void *);
+void	ipsp_delete_acquire_locked(struct ipsec_acquire *);
 void	ipsp_delete_acquire(struct ipsec_acquire *);
+void	ipsp_unref_acquire_locked(struct ipsec_acquire *);
 
 struct pool ipsec_policy_pool;
 struct pool ipsec_acquire_pool;
@@ -60,7 +61,9 @@ 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;
-TAILQ_HEAD(ipsec_acquire_head, ipsec_acquire) ipsec_acquire_head =
+
+struct mutex ipsec_acquire_mtx = MUTEX_INITIALIZER(IPL_SOFTNET);
+struct ipsec_acquire_head ipsec_acquire_head =
     TAILQ_HEAD_INITIALIZER(ipsec_acquire_head);
 
 struct radix_node_head *
@@ -686,8 +689,10 @@ ipsec_delete_policy(struct ipsec_policy *ipo)
 	}
 	mtx_leave(&ipo_tdb_mtx);
 
+	mtx_enter(&ipsec_acquire_mtx);
 	while ((ipa = TAILQ_FIRST(&ipo->ipo_acquires)) != NULL)
-		ipsp_delete_acquire(ipa);
+		ipsp_delete_acquire_locked(ipa);
+	mtx_leave(&ipsec_acquire_mtx);
 
 	TAILQ_REMOVE(&ipsec_policy_head, ipo, ipo_list);
 
@@ -702,13 +707,11 @@ ipsec_delete_policy(struct ipsec_policy *ipo)
 }
 
 void
-ipsp_delete_acquire_timo(void *v)
+ipsp_delete_acquire_timer(void *v)
 {
 	struct ipsec_acquire *ipa = v;
 
-	NET_LOCK();
 	ipsp_delete_acquire(ipa);
-	NET_UNLOCK();
 }
 
 /*
@@ -717,13 +720,38 @@ ipsp_delete_acquire_timo(void *v)
 void
 ipsp_delete_acquire(struct ipsec_acquire *ipa)
 {
-	NET_ASSERT_LOCKED();
+	mtx_enter(&ipsec_acquire_mtx);
+	ipsp_delete_acquire_locked(ipa);
+	mtx_leave(&ipsec_acquire_mtx);
+}
+
+void
+ipsp_delete_acquire_locked(struct ipsec_acquire *ipa)
+{
+	if (timeout_del(&ipa->ipa_timeout) == 1)
+		refcnt_rele(&ipa->ipa_refcnt);
+	ipsp_unref_acquire_locked(ipa);
+}
+
+void
+ipsec_unref_acquire(struct ipsec_acquire *ipa)
+{
+	mtx_enter(&ipsec_acquire_mtx);
+	ipsp_unref_acquire_locked(ipa);
+	mtx_leave(&ipsec_acquire_mtx);
+}
 
-	timeout_del(&ipa->ipa_timeout);
+void
+ipsp_unref_acquire_locked(struct ipsec_acquire *ipa)
+{
+	MUTEX_ASSERT_LOCKED(&ipsec_acquire_mtx);
+
+	if (refcnt_rele(&ipa->ipa_refcnt) == 0)
+		return;
 	TAILQ_REMOVE(&ipsec_acquire_head, ipa, ipa_next);
-	if (ipa->ipa_policy != NULL)
-		TAILQ_REMOVE(&ipa->ipa_policy->ipo_acquires, ipa,
-		    ipa_ipo_next);
+	TAILQ_REMOVE(&ipa->ipa_policy->ipo_acquires, ipa, ipa_ipo_next);
+	ipa->ipa_policy = NULL;
+
 	pool_put(&ipsec_acquire_pool, ipa);
 }
 
@@ -731,19 +759,21 @@ ipsp_delete_acquire(struct ipsec_acquire *ipa)
  * Find out if there's an ACQUIRE pending.
  * XXX Need a better structure.
  */
-struct ipsec_acquire *
+int
 ipsp_pending_acquire(struct ipsec_policy *ipo, union sockaddr_union *gw)
 {
 	struct ipsec_acquire *ipa;
 
 	NET_ASSERT_LOCKED();
 
-	TAILQ_FOREACH (ipa, &ipo->ipo_acquires, ipa_ipo_next) {
+	mtx_enter(&ipsec_acquire_mtx);
+	TAILQ_FOREACH(ipa, &ipo->ipo_acquires, ipa_ipo_next) {
 		if (!memcmp(gw, &ipa->ipa_addr, gw->sa.sa_len))
-			return ipa;
+			break;
 	}
+	mtx_leave(&ipsec_acquire_mtx);
 
-	return NULL;
+	return (ipa != NULL);
 }
 
 /*
@@ -759,7 +789,7 @@ ipsp_acquire_sa(struct ipsec_policy *ipo, union sockaddr_union *gw,
 	NET_ASSERT_LOCKED();
 
 	/* Check whether request has been made already. */
-	if ((ipa = ipsp_pending_acquire(ipo, gw)) != NULL)
+	if (ipsp_pending_acquire(ipo, gw))
 		return 0;
 
 	/* Add request in cache and proceed. */
@@ -769,7 +799,8 @@ ipsp_acquire_sa(struct ipsec_policy *ipo, union sockaddr_union *gw,
 
 	ipa->ipa_addr = *gw;
 
-	timeout_set_proc(&ipa->ipa_timeout, ipsp_delete_acquire_timo, ipa);
+	refcnt_init(&ipa->ipa_refcnt);
+	timeout_set(&ipa->ipa_timeout, ipsp_delete_acquire_timer, ipa);
 
 	ipa->ipa_info.sen_len = ipa->ipa_mask.sen_len = SENT_LEN;
 	ipa->ipa_info.sen_family = ipa->ipa_mask.sen_family = PF_KEY;
@@ -850,13 +881,15 @@ ipsp_acquire_sa(struct ipsec_policy *ipo, union sockaddr_union *gw,
 		return 0;
 	}
 
+	mtx_enter(&ipsec_acquire_mtx);
 #ifdef IPSEC
-	timeout_add_sec(&ipa->ipa_timeout, ipsec_expire_acquire);
+	if (timeout_add_sec(&ipa->ipa_timeout, ipsec_expire_acquire) == 1)
+		refcnt_take(&ipa->ipa_refcnt);
 #endif
-
 	TAILQ_INSERT_TAIL(&ipsec_acquire_head, ipa, ipa_next);
 	TAILQ_INSERT_TAIL(&ipo->ipo_acquires, ipa, ipa_ipo_next);
 	ipa->ipa_policy = ipo;
+	mtx_leave(&ipsec_acquire_mtx);
 
 	/* PF_KEYv2 notification message. */
 	return pfkeyv2_acquire(ipo, gw, laddr, &ipa->ipa_seq, ddst);
@@ -908,9 +941,14 @@ ipsec_get_acquire(u_int32_t seq)
 
 	NET_ASSERT_LOCKED();
 
-	TAILQ_FOREACH (ipa, &ipsec_acquire_head, ipa_next)
-		if (ipa->ipa_seq == seq)
-			return ipa;
+	mtx_enter(&ipsec_acquire_mtx);
+	TAILQ_FOREACH(ipa, &ipsec_acquire_head, ipa_next) {
+		if (ipa->ipa_seq == seq) {
+			refcnt_take(&ipa->ipa_refcnt);
+			break;
+		}
+	}
+	mtx_leave(&ipsec_acquire_mtx);
 
-	return NULL;
+	return ipa;
 }