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

On Tue, Aug 18, 2026 at 05:01:58PM +0200, Alexandr Nedvedicky wrote:
> 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(..);'

The compiler does the right thing.  The cast is unnecessary.

> updated diff is below.

OK bluhm@

> 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)
>