Fernando Fernandez Mancera <[email protected]> wrote: > > > On 11/6/25 2:39 AM, Florian Westphal wrote: > > Fernando Fernandez Mancera <[email protected]> wrote: > >> Since commit d265929930e2 ("netfilter: nf_conncount: reduce unnecessary > >> GC") if nftables/iptables connlimit is used without a check for new > >> connections there can be duplicated entries tracked. > >> > >> Pass the nf_conn struct directly to the nf_conncount API and check > >> whether the connection is confirmed or not inside nf_conncount_add(). If > >> the connection is confirmed, skip it. > > > > I think there is a bit too much noise here, can this be > > split in several chunks? > > > > Not sure, I could try but the noise comes from removing zone and tuple > which requires many changes around. Otherwise this would compile with > warnings/errors. I am not sure I can split this more.
Ok. > > I also see that this shared copypaste with xtables. > > Would it be possible to pass 'const struct sk_buff *' > > and let the nf_conncount core handle this internally? > > > > nf_conncount_add(net, pf, skb, priv->list); > > > > which does: > > ct = nf_ct_get(skb, &ctinfo); > > if (ct && !nf_ct_is_template(ct)) > > return __nf_conncount_add(ct, list); > > > > if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), pf, net, > > &tuple)) > > return -ERR; > > > > if (ct) /* its a template, so do lookup in right zone */ > > zone = nf_ct_zone(ct); > > else > > zone = &nf_ct_zone_dflt; > > > > h = nf_conntrack_find_get(nft_net(pkt), zone, &tuple); > > if (!h) > > return -ERR; > > > > ct = nf_ct_tuplehash_to_ctrack(h); > > > > err = __nf_conncount_add(ct, list); > > > > nf_ct_put(ct): > > > > return err; > > > > I.e., the existing nf_conncount_add() becomes a helper that takes > > a ct, as you have already proposed, but its renamed and turned into > > an internal helper so frontends don't need to duplicate tuple lookup. > > > > Alternatively, no need to rename it and instead add a new API call > > that takes the ct argument, e.g. 'nf_conncount_add_ct' or whatever, > > and then make nf_conncount_add() internal in a followup patch. > > > > Unfortunately, I do not think this is possible. xt_connlimit is using > the rbtree with nf_conncount_count() while nft_connlimit isn't. I > believe we do not want to change that. OK. I had hoped one could start with refactoring nf_conncount_count() and then after that nf_conncount_count(). > In addition, for the rbtree we > need to calculate the key.. Right but AFAICS due to the missing 'template check' we pass all-zero tuple_ptr if there is a template attached to the skb. Not catastrophic but its not correct either. I had hoped it was possible so s/tuple, zone/sk_buff/ in the arguments and then handle that internally (first in count_tree and then later in a converted nf_conncount_count(), i.e. push the sk_buff -> ct handling down in followup patches. > I would leave this code as duplicated given > that is shared only between xt_connlimit and nft_connlimit. Openvswitch > doesn't care about this as they always call nf_conncount_count() while > holding a reference to a ct.. [..] > No worries at all, I think there is some benefit from this change even > with the copy-paste. Maybe we can create a helper function to just get > the ct from the sk_buff.. what about "nf_conntrack_get_or_find()"? I > accept suggestions for a better name :) OK, but the problem is that you need to know when you need to put the reference and when you don't have to. Task is: given skb, return a 'ct' that is not a template ... but if its a template we need the zone to make a lookup ourselves ... and we have to bump (and put) refcount when we do that lookup ourselves. Maybe you could try adding a new api, e.g. call it 'nf_conncount_count_skb()' that then calls nf_conncount_count() internally just to see how bad it looks? If its not too bad, then all callers could be converted one after another. And then same with nf_conncount_add(). Just a suggestion, alternatively give your nf_conntrack_get_or_find() a try, it would need a 'bool *refcounted' or similar arg and a conditional 'if (refcounted) nf_ct_put(ct)' to be done by the callers. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
