From: sashan <sashan@openbsd.org>
Date: Thu, 10 Nov 2022 16:29:20 +0000 (+0000)
Subject: revert pf_state mtx commit, because it breaks tree.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=e0fbf0e28dd12cf1f98854a5f92dacc317245e3c;p=openbsd

revert pf_state mtx commit, because it breaks tree.
pfctl does not build

OK dlg@
---

diff --git a/sys/net/pf.c b/sys/net/pf.c
index d5a6f090655..06599d38009 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf.c,v 1.1147 2022/11/10 14:22:43 sashan Exp $ */
+/*	$OpenBSD: pf.c,v 1.1148 2022/11/10 16:29:20 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -185,8 +185,7 @@ int			 pf_translate_icmp_af(struct pf_pdesc*, int, void *);
 void			 pf_send_icmp(struct mbuf *, u_int8_t, u_int8_t, int,
 			    sa_family_t, struct pf_rule *, u_int);
 void			 pf_detach_state(struct pf_state *);
-void			 pf_state_key_detach(struct pf_state *,
-			    struct pf_state_key *);
+void			 pf_state_key_detach(struct pf_state *, int);
 u_int32_t		 pf_tcp_iss(struct pf_pdesc *);
 void			 pf_rule_to_actions(struct pf_rule *,
 			    struct pf_rule_actions *);
@@ -777,8 +776,7 @@ pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int idx)
 		s->key[idx] = sk;
 
 	if ((si = pool_get(&pf_state_item_pl, PR_NOWAIT)) == NULL) {
-		pf_state_key_detach(s, s->key[idx]);
-		s->key[idx] = NULL;
+		pf_state_key_detach(s, idx);
 		return (-1);
 	}
 	si->s = s;
@@ -798,50 +796,42 @@ pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int idx)
 void
 pf_detach_state(struct pf_state *s)
 {
-	struct pf_state_key *key[2];
-
-	mtx_enter(&s->mtx);
-	key[PF_SK_WIRE] = s->key[PF_SK_WIRE];
-	key[PF_SK_STACK] = s->key[PF_SK_STACK];
-	s->key[PF_SK_WIRE] = NULL;
-	s->key[PF_SK_STACK] = NULL;
-	mtx_leave(&s->mtx);
-
-	if (key[PF_SK_WIRE] == key[PF_SK_STACK])
-		key[PF_SK_WIRE] = NULL;
+	if (s->key[PF_SK_WIRE] == s->key[PF_SK_STACK])
+		s->key[PF_SK_WIRE] = NULL;
 
-	if (key[PF_SK_STACK] != NULL)
-		pf_state_key_detach(s, key[PF_SK_STACK]);
+	if (s->key[PF_SK_STACK] != NULL)
+		pf_state_key_detach(s, PF_SK_STACK);
 
-	if (key[PF_SK_WIRE] != NULL)
-		pf_state_key_detach(s, key[PF_SK_WIRE]);
+	if (s->key[PF_SK_WIRE] != NULL)
+		pf_state_key_detach(s, PF_SK_WIRE);
 }
 
 void
-pf_state_key_detach(struct pf_state *s, struct pf_state_key *key)
+pf_state_key_detach(struct pf_state *s, int idx)
 {
 	struct pf_state_item	*si;
+	struct pf_state_key	*sk;
 
-	PF_STATE_ASSERT_LOCKED();
-
-	if (key == NULL)
+	if (s->key[idx] == NULL)
 		return;
 
-	si = TAILQ_FIRST(&key->states);
+	si = TAILQ_FIRST(&s->key[idx]->states);
 	while (si && si->s != s)
 	    si = TAILQ_NEXT(si, entry);
 
 	if (si) {
-		TAILQ_REMOVE(&key->states, si, entry);
+		TAILQ_REMOVE(&s->key[idx]->states, si, entry);
 		pool_put(&pf_state_item_pl, si);
 	}
 
-	if (TAILQ_EMPTY(&key->states)) {
-		RB_REMOVE(pf_state_tree, &pf_statetbl, key);
-		key->removed = 1;
-		pf_state_key_unlink_reverse(key);
-		pf_state_key_unlink_inpcb(key);
-		pf_state_key_unref(key);
+	sk = s->key[idx];
+	s->key[idx] = NULL;
+	if (TAILQ_EMPTY(&sk->states)) {
+		RB_REMOVE(pf_state_tree, &pf_statetbl, sk);
+		sk->removed = 1;
+		pf_state_key_unlink_reverse(sk);
+		pf_state_key_unlink_inpcb(sk);
+		pf_state_key_unref(sk);
 	}
 }
 
@@ -1004,9 +994,7 @@ pf_state_insert(struct pfi_kif *kif, struct pf_state_key **skw,
 		}
 		*skw = s->key[PF_SK_WIRE];
 		if (pf_state_key_attach(*sks, s, PF_SK_STACK)) {
-			pf_state_key_detach(s, s->key[PF_SK_WIRE]);
-			s->key[PF_SK_WIRE] = NULL;
-			*skw = NULL;
+			pf_state_key_detach(s, PF_SK_WIRE);
 			PF_STATE_EXIT_WRITE();
 			return (-1);
 		}
@@ -1438,7 +1426,6 @@ pf_state_import(const struct pfsync_state *sp, int flags)
 	st->sync_state = PFSYNC_S_NONE;
 
 	refcnt_init(&st->refcnt);
-	mtx_init(&st->mtx, IPL_NET);
 
 	/* XXX when we have anchors, use STATE_INC_COUNTERS */
 	r->states_cur++;
@@ -4332,7 +4319,6 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a,
 	 * pf_state_inserts() grabs reference for pfsync!
 	 */
 	refcnt_init(&s->refcnt);
-	mtx_init(&s->mtx, IPL_NET);
 
 	switch (pd->proto) {
 	case IPPROTO_TCP:
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 800970e119d..385263bbcc2 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfvar.h,v 1.516 2022/11/10 14:22:43 sashan Exp $ */
+/*	$OpenBSD: pfvar.h,v 1.517 2022/11/10 16:29:20 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -741,49 +741,37 @@ struct pf_state_cmp {
 	u_int8_t		 pad[3];
 };
 
-/*
- * pf_state structure members require protection
- * either by state lock (a global rw-lock) or
- * need to be protected by mutex which is part
- * of state. The annotation goes as follows:
- *	- stateRW: means protected by exclusive state lock
- *	- mtx: protected by mutex (pf_state::state_mtx)
- *	- syncMTX: protected by mutex in pfsync
- *	- I: immutable since creation
- *	- ?: needs to be revisited
- */
 struct pf_state {
-	u_int64_t		 id;		/* I */
-	u_int32_t		 creatorid;	/* I */
-	u_int8_t		 direction;	/* I */
+	u_int64_t		 id;
+	u_int32_t		 creatorid;
+	u_int8_t		 direction;
 	u_int8_t		 pad[3];
 
-	TAILQ_ENTRY(pf_state)	 sync_list;	/* syncMTX */
-	TAILQ_ENTRY(pf_state)	 sync_snap;	/* syncMTX */
-	TAILQ_ENTRY(pf_state)	 entry_list;	/* stateRW */
-	SLIST_ENTRY(pf_state)	 gc_list;	/* syncMTX */
-	RB_ENTRY(pf_state)	 entry_id;	/* stateRW */
+	TAILQ_ENTRY(pf_state)	 sync_list;
+	TAILQ_ENTRY(pf_state)	 sync_snap;
+	TAILQ_ENTRY(pf_state)	 entry_list;
+	SLIST_ENTRY(pf_state)	 gc_list;
+	RB_ENTRY(pf_state)	 entry_id;
 	struct pf_state_peer	 src;
 	struct pf_state_peer	 dst;
-	struct pf_rule_slist	 match_rules;	/* I */
-	union pf_rule_ptr	 rule;		/* I */
-	union pf_rule_ptr	 anchor;	/* I */
-	union pf_rule_ptr	 natrule;	/* I */
-	struct pf_addr		 rt_addr;	/* I */
-	struct pf_sn_head	 src_nodes;	/* I */
-	struct pf_state_key	*key[2];	/* mtx, stack and wire  */
-	struct pfi_kif		*kif;		/* I */
-	struct mutex		 mtx;
-	u_int64_t		 packets[2];	/* ? */
-	u_int64_t		 bytes[2];	/* ? */
-	int32_t			 creation;	/* I */
-	int32_t			 expire;	/* ? */
-	int32_t			 pfsync_time;	/* ? */
-	int			 rtableid[2];	/* I rtables stack and wire */
-	u_int16_t		 qid;		/* I */
-	u_int16_t		 pqid;		/* I */
-	u_int16_t		 tag;		/* I */
-	u_int16_t		 state_flags;	/* ? */
+	struct pf_rule_slist	 match_rules;
+	union pf_rule_ptr	 rule;
+	union pf_rule_ptr	 anchor;
+	union pf_rule_ptr	 natrule;
+	struct pf_addr		 rt_addr;
+	struct pf_sn_head	 src_nodes;
+	struct pf_state_key	*key[2];	/* addresses stack and wire  */
+	struct pfi_kif		*kif;
+	u_int64_t		 packets[2];
+	u_int64_t		 bytes[2];
+	int32_t			 creation;
+	int32_t			 expire;
+	int32_t			 pfsync_time;
+	int			 rtableid[2];	/* rtables stack and wire */
+	u_int16_t		 qid;
+	u_int16_t		 pqid;
+	u_int16_t		 tag;
+	u_int16_t		 state_flags;
 #define	PFSTATE_ALLOWOPTS	0x0001
 #define	PFSTATE_SLOPPY		0x0002
 #define	PFSTATE_PFLOW		0x0004
@@ -797,20 +785,20 @@ struct pf_state {
 #define	PFSTATE_INP_UNLINKED	0x0400
 #define	PFSTATE_SCRUBMASK (PFSTATE_NODF|PFSTATE_RANDOMID|PFSTATE_SCRUB_TCP)
 #define	PFSTATE_SETMASK   (PFSTATE_SETTOS|PFSTATE_SETPRIO)
-	u_int8_t		 log;		/* I */
-	u_int8_t		 timeout;	/* ? */
-	u_int8_t		 sync_state;	/* ? PFSYNC_S_x */
-	u_int8_t		 sync_updates;	/* ? */
-	u_int8_t		 min_ttl;	/* I */
-	u_int8_t		 set_tos;	/* I */
-	u_int8_t		 set_prio[2];	/* I */
-	u_int16_t		 max_mss;	/* I */
-	u_int16_t		 if_index_in;	/* I */
-	u_int16_t		 if_index_out;	/* I */
+	u_int8_t		 log;
+	u_int8_t		 timeout;
+	u_int8_t		 sync_state; /* PFSYNC_S_x */
+	u_int8_t		 sync_updates;
+	u_int8_t		 min_ttl;
+	u_int8_t		 set_tos;
+	u_int8_t		 set_prio[2];
+	u_int16_t		 max_mss;
+	u_int16_t		 if_index_in;
+	u_int16_t		 if_index_out;
 	pf_refcnt_t		 refcnt;
-	u_int16_t		 delay;		/* I */
-	u_int8_t		 rt;		/* I */
-	u_int8_t		 snapped;	/* syncMTX */
+	u_int16_t		 delay;
+	u_int8_t		 rt;
+	u_int8_t		 snapped;
 };
 
 /*