Re: [PATCH 2/4 nf-next v2] netfilter: nf_conncount: only track connection if it is not confirmed

[Florian Westphal <fw@strlen.de>]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
> index 0189f8b6b0bd..8c21890e4536 100644
> --- a/net/netfilter/xt_connlimit.c
> +++ b/net/netfilter/xt_connlimit.c
> @@ -29,24 +29,16 @@
>  static bool
>  connlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  {
> -	struct net *net = xt_net(par);
>  	const struct xt_connlimit_info *info = par->matchinfo;
> -	struct nf_conntrack_tuple tuple;
> -	const struct nf_conntrack_tuple *tuple_ptr = &tuple;
> -	const struct nf_conntrack_zone *zone = &nf_ct_zone_dflt;
> -	enum ip_conntrack_info ctinfo;
> -	const struct nf_conn *ct;
> +	struct net *net = xt_net(par);
>  	unsigned int connections;
> +	bool refcounted = false;
> +	struct nf_conn *ct;
>  	u32 key[5];
>  
> -	ct = nf_ct_get(skb, &ctinfo);
> -	if (ct != NULL) {
> -		tuple_ptr = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
> -		zone = nf_ct_zone(ct);
> -	} else if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
> -				      xt_family(par), net, &tuple)) {
> +	ct = nf_ct_get_or_find(net, skb, xt_family(par), &refcounted);
> +	if (!ct)
>  		goto hotdrop;
> -	}

This can't work this way for -t raw use case, which we need
to preserve.

Anyone who uses -t raw ... -m connlimit ...

will now have their packets dropped, so no connection
can make forward progress (not even when using iptables --syn).

We need to get rid of the
        if ((u32)jiffies == list->last_gc)
                goto add_new_node;

check in nf_conncount_add(), or, (to not add perf regress for ovs ...)
apply it when a (confirmed) conntrack entry is present.

Given that limitation, I don't think this nf_ct_get_or_find() helper
makes any sense, since you still need to pass the tupleptr down
to count_tree().

I think passing in the sk_buff is better, so all of this
conntrack/tuple/zone etc. stuff is hidden away in nf_conncount.c.

I think you could start by *adding*

unsigned int nf_conncount_count_skb(struct net *net,
				    const struct sk_buff *skb,
				    struct nf_conncount_data *data,
				    const u32 *key);

As frontend function for nf_conncount_count().
This new function could (re)use some of the code you made
for nf_ct_get_or_find(), the zone usage there looks correct
to me.

Then, in patch 2, convert -m connlimit.
You could send that as an initial patch set already.

Then, in patch 3 (or later followup patch set), convert remaining user
(ovs) and hide old api.

Then, in patch 4, start pushing down the sk_buff more in nf_conncount.c
until its available for nf_conncount_add().

Then, add nf_conncount_add_skb and repeat this process.

Does that make sense?