From: mvs <mvs@openbsd.org>
Date: Sat, 30 Apr 2022 13:28:53 +0000 (+0000)
Subject: When performing ipsp_ids_free(), grab `ipsec_flows_mtx' mutex(9) before do
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=2af522804e34e651131b12f41358e75900a7af55;p=openbsd

When performing ipsp_ids_free(), grab `ipsec_flows_mtx' mutex(9) before do
`id_refcount' decrement. This should be consistent with `ipsp_ids_gc_list'
list modifications, otherwise concurrent ipsp_ids_insert() could remove
this dying `ids' from the list before if was placed there by
ipsp_ids_free(). This makes atomic operations with `id_refcount' useless.
Also prevent ipsp_ids_lookup() to return dying `ids'.

ok bluhm@
---

diff --git a/sys/netinet/ip_ipsp.c b/sys/netinet/ip_ipsp.c
index a24885c9971..cf396225e2b 100644
--- a/sys/netinet/ip_ipsp.c
+++ b/sys/netinet/ip_ipsp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_ipsp.c,v 1.269 2022/03/10 15:21:08 bluhm Exp $	*/
+/*	$OpenBSD: ip_ipsp.c,v 1.270 2022/04/30 13:28:53 mvs Exp $	*/
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -1205,7 +1205,7 @@ ipsp_ids_insert(struct ipsec_ids *ids)
 	found = RBT_INSERT(ipsec_ids_tree, &ipsec_ids_tree, ids);
 	if (found) {
 		/* if refcount was zero, then timeout is running */
-		if (atomic_inc_int_nv(&found->id_refcount) == 1) {
+		if ((++found->id_refcount) == 1) {
 			LIST_REMOVE(found, id_gc_list);
 
 			if (LIST_EMPTY(&ipsp_ids_gc_list))
@@ -1248,7 +1248,12 @@ ipsp_ids_lookup(u_int32_t ipsecflowinfo)
 
 	mtx_enter(&ipsec_flows_mtx);
 	ids = RBT_FIND(ipsec_ids_flows, &ipsec_ids_flows, &key);
-	atomic_inc_int(&ids->id_refcount);
+	if (ids != NULL) {
+		if (ids->id_refcount != 0)
+			ids->id_refcount++;
+		else
+			ids = NULL;
+	}
 	mtx_leave(&ipsec_flows_mtx);
 
 	return ids;
@@ -1290,6 +1295,8 @@ ipsp_ids_free(struct ipsec_ids *ids)
 	if (ids == NULL)
 		return;
 
+	mtx_enter(&ipsec_flows_mtx);
+
 	/*
 	 * If the refcount becomes zero, then a timeout is started. This
 	 * timeout must be cancelled if refcount is increased from zero.
@@ -1297,10 +1304,10 @@ ipsp_ids_free(struct ipsec_ids *ids)
 	DPRINTF("ids %p count %d", ids, ids->id_refcount);
 	KASSERT(ids->id_refcount > 0);
 
-	if (atomic_dec_int_nv(&ids->id_refcount) > 0)
+	if ((--ids->id_refcount) > 0) {
+		mtx_leave(&ipsec_flows_mtx);
 		return;
-
-	mtx_enter(&ipsec_flows_mtx);
+	}
 
 	/*
 	 * Add second for the case ipsp_ids_gc() is already running and
diff --git a/sys/netinet/ip_ipsp.h b/sys/netinet/ip_ipsp.h
index 7004a40a579..9eb91628c1c 100644
--- a/sys/netinet/ip_ipsp.h
+++ b/sys/netinet/ip_ipsp.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ip_ipsp.h,v 1.238 2022/04/21 15:22:50 sashan Exp $	*/
+/*	$OpenBSD: ip_ipsp.h,v 1.239 2022/04/30 13:28:53 mvs Exp $	*/
 /*
  * The authors of this code are John Ioannidis (ji@tla.org),
  * Angelos D. Keromytis (kermit@csd.uch.gr),
@@ -241,7 +241,7 @@ struct ipsec_ids {
 	struct ipsec_id		*id_local;	/* [I] */
 	struct ipsec_id		*id_remote;	/* [I] */
 	u_int32_t		id_flow;	/* [I] */
-	u_int			id_refcount;	/* [a] */
+	u_int			id_refcount;	/* [F] */
 	u_int			id_gc_ttl;	/* [F] */
 };
 RBT_HEAD(ipsec_ids_flows, ipsec_ids);