Hello,

sorry to keep you waiting. I guess this is the fix for leak you've
talked about off-list, correct?

the change looks good. the code reads much butter now. I just could
spot one single nit, see comment in-line.

thanks and
regards
sashan

On Fri, Jan 16, 2026 at 07:26:52AM +1000, David Gwynne wrote:
</snip>
> Index: net/pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> diff -u -p -r1.1230 pf.c
> --- net/pf.c	7 Jan 2026 13:50:05 -0000	1.1230
> +++ net/pf.c	15 Jan 2026 05:34:55 -0000
> @@ -247,15 +247,16 @@ int			 pf_state_insert(struct pfi_kif *,
>  			    struct pf_state_key **, struct pf_state_key **,
>  			    struct pf_state *);
>  
> +int			 pf_state_isvalid(struct pf_state *);
>  int			 pf_state_key_isvalid(struct pf_state_key *);
>  struct pf_state_key	*pf_state_key_ref(struct pf_state_key *);
>  void			 pf_state_key_unref(struct pf_state_key *);
> -void			 pf_state_key_link_reverse(struct pf_state_key *,
> -			    struct pf_state_key *);
> -void			 pf_state_key_unlink_reverse(struct pf_state_key *);
> -void			 pf_state_key_link_inpcb(struct pf_state_key *,
> +void			 pf_state_link_reverse(struct pf_state *,
> +			    struct pf_state *);
> +void			 pf_state_unlink_reverse(struct pf_state *);
> +void			 pf_state_link_inpcb(struct pf_state *,
>  			    struct inpcb *);
> -void			 pf_state_key_unlink_inpcb(struct pf_state_key *);
> +void			 pf_state_unlink_inpcb(struct pf_state *);
>  void			 pf_pktenqueue_delayed(void *);
>  int32_t			 pf_state_expires(const struct pf_state *, uint8_t);
>  
> @@ -1134,8 +1135,6 @@ pf_state_key_detach(struct pf_state *st,
>  	if (TAILQ_EMPTY(&sk->sk_states)) {
>  		RBT_REMOVE(pf_state_tree, &pf_statetbl, sk);
>  		sk->sk_removed = 1;
> -		pf_state_key_unlink_reverse(sk);
> -		pf_state_key_unlink_inpcb(sk);
>  		pf_state_key_unref(sk);
>  	}
>  
> @@ -1316,14 +1315,24 @@ pf_state_insert(struct pfi_kif *kif, str
>  
>  	if (same) {
>  		/* pf_state_key_attach might have swapped skw */
> -		pf_state_key_unref(sks);
> -		st->key[PF_SK_STACK] = sks = pf_state_key_ref(skw);
> +		if (skw != sks) {
                ^^^^^
    the condition here looks odd. I think it can not be met, when we are
    running here. see few lines above where you will find:
	'same = (skw == sks)'
    I would either drop the condition or add KASSERT(sks == skw) here,
    just in case we refactor code in future. 

> +			pf_state_key_unref(sks);
> +			sks = pf_state_key_ref(skw);
> +		}
> +		st->key[PF_SK_STACK] = sks;
>  	} else if (pf_state_key_attach(sks, st, PF_SK_STACK) == NULL) {
>  		pf_state_key_detach(st, PF_SK_WIRE);
> +		pf_state_key_unref(skw);
>  		PF_STATE_EXIT_WRITE();
>  		return (-1);
>  	}
>  
> +	/*
> +	 * st owns the sks and skw refs now. pf_state_unref (maybe
> +	 * via pf_detach_state) is responsible for releasing them in
> +	 * the future.
> +	 */
> +