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

[Florian Westphal <fw@strlen.de>]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> Connlimit expression can be used for all kind of packets and not only
> for packets with connection state new. See this ruleset as example:
> 
> table ip filter {
>         chain input {
>                 type filter hook input priority filter; policy accept;
>                 tcp dport 22 ct count over 4 counter
>         }
> }
> 
> Currently, if the connection count goes over the limit the counter will
> count the packets. When a connection is closed, the connection count
> won't decrement as it should because it is only updated for new
> connections due to an optimization on __nf_conncount_add() that prevents
> updating the list if the connection is duplicated.
> 
> In addition, since commit d265929930e2 ("netfilter: nf_conncount: reduce
> unnecessary GC") there can be situations where a duplicated connection
> is added to the list. This is caused by two packets from the same
> connection being processed during the same jiffy.
> +	if (!ct || !nf_ct_is_confirmed(ct)) {
> +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> +			regs->verdict.code = NF_DROP;
> +			return;
> +		}

This means the bug fix won't work when this is hooked before
conntrack.

> diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
> index 0189f8b6b0bd..5c90e1929d86 100644
> --- a/net/netfilter/xt_connlimit.c
> +++ b/net/netfilter/xt_connlimit.c
> @@ -69,8 +69,18 @@ connlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		key[1] = zone->id;
>  	}
>  
> -	connections = nf_conncount_count(net, info->data, key, tuple_ptr,
> -					 zone);
> +	if (!ct || !nf_ct_is_confirmed(ct)) {
> +		connections = nf_conncount_count(net, info->data, key, tuple_ptr,
> +						 zone);

Same here, but usage in -t raw is legal/allowed.

I would suggest to rework this api so that this always passes in
struct nf_conn *ct, either derived from sk_buff or obtained via
nf_conntrack_find_get().

The net, tuple_ptr, and zone argument would be obsoleted and
replaced with nf_conn *ct arg.

This allows the nf_conncount internals to always skip the
insertion for !confirmed case, and the existing suppression
for same-jiffy collect could remain in place as well.

Its more work but since this has been broken forever I don't
think we need a urgent/small fix for this.