Re: pf: u_int32_t conn counter underflow in pf_src_tree_remove_state()

On Sat, Aug 15, 2026 at 12:10:03PM +0200, Alexandr Nedvedicky wrote:
> 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?

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.

> +     atomic_inc_int((int *)&sn->conn);

Do we need the cast?  Address of u_int32_t should work with
atomic_inc_int().

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

	if ((*stp)->rule.ptr->max_src_conn &&
	    (*stp)->rule.ptr->max_src_conn < sn_conn) {
		pf_status.lcounters[LCNT_SRCCONN]++;
		bad++;
	}

bluhm

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