From d66d11b478f9a50073031ddedae40d5169d68237 Mon Sep 17 00:00:00 2001
From: kn <kn@openbsd.org>
Date: Fri, 26 May 2023 12:13:26 +0000
Subject: [PATCH] Remove net lock from DIOC{S,G}ETLIMIT

Grab the pf lock for pf_pool_limits[] in pfsync such that all access is
covered by the pf lock;  document accordingly.

Hard memory pool limits don't need the net lock for protection, pool(9)s
have their own internal lock and the pf lock fully covers limit values.

(pf_pool_limits[] access in DIOCXCOMMIT remains under pf *and net* lock
 until the rest in there gets pulled out of the net lock.)

OK sashan
---
 sys/net/if_pfsync.c |  6 +++++-
 sys/net/pf_ioctl.c  |  9 +--------
 sys/net/pfvar.h     | 14 ++++++++++----
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/sys/net/if_pfsync.c b/sys/net/if_pfsync.c
index 71ca9f23132..2bf93306da2 100644
--- a/sys/net/if_pfsync.c
+++ b/sys/net/if_pfsync.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_pfsync.c,v 1.315 2023/05/18 12:10:04 sashan Exp $	*/
+/*	$OpenBSD: if_pfsync.c,v 1.316 2023/05/26 12:13:26 kn Exp $	*/
 
 /*
  * Copyright (c) 2002 Michael Shalayeff
@@ -1009,10 +1009,12 @@ pfsync_in_bus(caddr_t buf, int len, int count, int flags)
 
 	switch (bus->status) {
 	case PFSYNC_BUS_START:
+		PF_LOCK();
 		timeout_add(&sc->sc_bulkfail_tmo, 4 * hz +
 		    pf_pool_limits[PF_LIMIT_STATES].limit /
 		    ((sc->sc_if.if_mtu - PFSYNC_MINPKT) /
 		    sizeof(struct pfsync_state)));
+		PF_UNLOCK();
 		DPFPRINTF(LOG_INFO, "received bulk update start");
 		break;
 
@@ -2037,10 +2039,12 @@ pfsync_request_full_update(struct pfsync_softc *sc)
 #endif
 		pfsync_sync_ok = 0;
 		DPFPRINTF(LOG_INFO, "requesting bulk update");
+		PF_LOCK();
 		timeout_add(&sc->sc_bulkfail_tmo, 4 * hz +
 		    pf_pool_limits[PF_LIMIT_STATES].limit /
 		    ((sc->sc_if.if_mtu - PFSYNC_MINPKT) /
 		    sizeof(struct pfsync_state)));
+		PF_UNLOCK();
 		pfsync_request_update(0, 0);
 	}
 }
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index f70796cdd2e..e5b930a978a 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf_ioctl.c,v 1.404 2023/05/11 12:36:22 kn Exp $ */
+/*	$OpenBSD: pf_ioctl.c,v 1.405 2023/05/26 12:13:26 kn Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -2094,44 +2094,37 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 			error = EINVAL;
 			goto fail;
 		}
-		NET_LOCK();
 		PF_LOCK();
 		pl->limit = pf_pool_limits[pl->index].limit;
 		PF_UNLOCK();
-		NET_UNLOCK();
 		break;
 	}
 
 	case DIOCSETLIMIT: {
 		struct pfioc_limit	*pl = (struct pfioc_limit *)addr;
 
-		NET_LOCK();
 		PF_LOCK();
 		if (pl->index < 0 || pl->index >= PF_LIMIT_MAX) {
 			error = EINVAL;
 			PF_UNLOCK();
-			NET_UNLOCK();
 			goto fail;
 		}
 		if (((struct pool *)pf_pool_limits[pl->index].pp)->pr_nout >
 		    pl->limit) {
 			error = EBUSY;
 			PF_UNLOCK();
-			NET_UNLOCK();
 			goto fail;
 		}
 		/* Fragments reference mbuf clusters. */
 		if (pl->index == PF_LIMIT_FRAGS && pl->limit > nmbclust) {
 			error = EINVAL;
 			PF_UNLOCK();
-			NET_UNLOCK();
 			goto fail;
 		}
 
 		pf_pool_limits[pl->index].limit_new = pl->limit;
 		pl->limit = pf_pool_limits[pl->index].limit;
 		PF_UNLOCK();
-		NET_UNLOCK();
 		break;
 	}
 
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 2e808e1fcd7..c37b5f8ea2e 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfvar.h,v 1.530 2023/04/28 14:08:38 sashan Exp $ */
+/*	$OpenBSD: pfvar.h,v 1.531 2023/05/26 12:13:26 kn Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -1795,10 +1795,16 @@ const struct pfq_ops
 extern struct pf_status	pf_status;
 extern struct pool	pf_frent_pl, pf_frag_pl;
 
+/*
+ * Protection/ownership of pf_pool_limit:
+ *	I	immutable after pfattach()
+ *	p	pf_lock
+ */
+
 struct pf_pool_limit {
-	void		*pp;
-	unsigned	 limit;
-	unsigned	 limit_new;
+	void		*pp;		/* [I] */
+	unsigned	 limit;		/* [p] */
+	unsigned	 limit_new;	/* [p] */
 };
 extern struct pf_pool_limit	pf_pool_limits[PF_LIMIT_MAX];
 
-- 
2.20.1