Hello,

resending patch from bugs [1], the mail is part of this thread [2].

the pf(4) needs to use aotmic ops to bump connection counter
at source node. This happens in function pf_src_connlimit() which
is called on behalf of pf_test_state() via pf_tcp_track_*() functions.
the pf_test_state() itself is being called from pf_test() here:

8511
8512                 PF_STATE_ENTER_READ();
8513                 action = pf_find_state(&pd, &key, &st);
8514                 st = pf_state_ref(st);
8515                 PF_STATE_EXIT_READ();
8516
8517                 /* check for syncookies if tcp ack and no active state */
....
8546
8547                 if (action == PF_MATCH)
8548                         action = pf_test_state(&pd, &st, &reason);

the thing is that pf_test_state() uses a reference to state. the function
itself is running without locks. It may happen two packets try to update
connection counter at the same source node entry. If that happens in parallel
then counter may miss update. This later leads to underflow.

The reporter (Mr. Janak Trivedi) confirms patch below works. I think he replied
off-list.

OK to commit diff below?

thanks and
regards
sashan

[1] https://marc.info/?l=openbsd-bugs&m=178627901979767&w=2

[2] https://marc.info/?t=178601035500001&r=1&w=2

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 0fd00c0dbf3..ca28eb0b98f 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -760,7 +760,13 @@ pf_src_connlimit(struct pf_state **stp)
 	if ((sn = pf_get_src_node((*stp), PF_SN_NONE)) == NULL)
 		return (0);
 
-	sn->conn++;
+	/*
+	 * Note: conn limit is bumped on SYN_SENT->ESTBLISHED
+	 * state transition. packet does not hold any locks
+	 * when running here, therefore atomic is needed.
+	 */
+	atomic_inc_int((int *)&sn->conn);
+
 	(*stp)->src.tcp_est = 1;
 	pf_add_threshold(&sn->conn_rate);
 
@@ -2051,10 +2057,16 @@ pf_src_tree_remove_state(struct pf_state *st)
 	u_int32_t		 timeout;
 	struct pf_sn_item	*sni;
 
+	PF_ASSERT_LOCKED();
+
 	while ((sni = SLIST_FIRST(&st->src_nodes)) != NULL) {
 		SLIST_REMOVE_HEAD(&st->src_nodes, next);
-		if (st->src.tcp_est)
+		if (st->src.tcp_est) {
+			/*
+			 * atomic not needed here, because of PF_LOCK()
+			 */
 			--sni->sn->conn;
+		}
 		if (--sni->sn->states == 0) {
 			timeout = st->rule.ptr->timeout[PFTM_SRC_NODE];
 			if (!timeout)