Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Oct 28, 2025 at 06:06:47PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > -	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > > > -		regs->verdict.code = NF_DROP;
> > > > -		return;
> > > > +	if (ctinfo == IP_CT_NEW) {
> > > 
> > > Quick question: Is this good enough to deal with retransmitted packets
> > > while in NEW state?
> > 
> > Good point, I did not consider retransmit case.
> > What about if (!nf_ct_is_confirmed(ct)) { ..?
> > 
> > Would need a small comment that this is there instead of NEW
> > check due to rexmit.
> > 
> > > > +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > > > +			regs->verdict.code = NF_DROP;
> > > > +			return;
> > > > +		}
> > > > +	} else {
> > > > +		local_bh_disable();
> > > > +		nf_conncount_gc_list(nft_net(pkt), priv->list);
> > > > +		local_bh_enable();
> > > >  	}
> > 
> > As this needs a respin anyway, what about removing the else
> > clause altogether?  Resp. what was the rationale for doing a gc call
> > here?
> 
> You mean, call gc for both cases, correct?

No, i meant this:

/* comment that explains this */
if (nf_ct_is_confirmed(ct))
	return;

if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
	regs->verdict.code = NF_DROP;
	return;
}

As we don't add anything for 'is confirmed' case, I don't see why
we would need to prune the existing list.

> > And, do we want same check in xt_connlimit?
> 
> Are you referring to the !nf_ct_is_confirmed() check?

Yes, AFAICS xt_connlimit has same issue as nft_conncount given
they use same backend.

Backend doesn't have access to nf_conn/ctinfo so no way to solve it there.