From: Alexandr Nedvedicky <sashan@fastmail.net>
Subject: Re: pf: u_int32_t conn counter underflow in pf_src_tree_remove_state()
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Tue, 18 Aug 2026 17:01:58 +0200

Hello,

On Tue, Aug 18, 2026 at 04:16:51PM +0200, Alexander Bluhm wrote:
> 
> Is it sufficient to have one unlocked atomic operation together
> with a locked non-atomic?
> 
> thread A pf_src_connlimit
> thread B pf_src_tree_remove_state
> 
> B PF_LOCK
> B read sn->conn
> A atomic increment sn->conn
> B write sn->conn - 1
> B PF_UNLOCK
> 
> This does not work, you need atomic everywhere.

    of course you are right. I kind of forgot about race
    when there are two states where when one does transition
    to ESTABLSIHED state while the other state is being purged.

> 
> > +     atomic_inc_int((int *)&sn->conn);
> 
> Do we need the cast?  Address of u_int32_t should work with
> atomic_inc_int().

    perhaps not. I dropped the cast here.

> 
> The overflow check is reading the sn->conn value independent from the
> atomic increment.  Something like this is better:
> 
> 	uint32_t sn_conn;
> 
> 	sn_conn = atomic_inc_int_nv(&sn->conn);
    do you mind if I do '(u_int32_t)atomic_inc_int_nv(..);'


updated diff is below.

thanks and
regards
sashan

--------8<---------------8<-----------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 0fd00c0dbf3..5ce7c1226a1 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -756,16 +756,23 @@ pf_src_connlimit(struct pf_state **stp)
 {
 	int			 bad = 0;
 	struct pf_src_node	*sn;
+	u_int32_t		 sn_conn;
 
 	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.
+	 */
+	sn_conn = (u_int32_t) atomic_inc_int_nv(&sn->conn);
+
 	(*stp)->src.tcp_est = 1;
 	pf_add_threshold(&sn->conn_rate);
 
 	if ((*stp)->rule.ptr->max_src_conn &&
-	    (*stp)->rule.ptr->max_src_conn < sn->conn) {
+	    (*stp)->rule.ptr->max_src_conn < sn_conn) {
 		pf_status.lcounters[LCNT_SRCCONN]++;
 		bad++;
 	}
@@ -2054,7 +2061,7 @@ pf_src_tree_remove_state(struct pf_state *st)
 	while ((sni = SLIST_FIRST(&st->src_nodes)) != NULL) {
 		SLIST_REMOVE_HEAD(&st->src_nodes, next);
 		if (st->src.tcp_est)
-			--sni->sn->conn;
+			atomic_dec_int(&sni->sn->conn);
 		if (--sni->sn->states == 0) {
 			timeout = st->rule.ptr->timeout[PFTM_SRC_NODE];
 			if (!timeout)