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

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > TBH, I added this expression mainly focusing on being used with
> > > dynset, I allowed it too in rules for parity. In the dynset case,
> > > there is a front-end datastructure (set) and this conncount list
> > > is per element. Maybe there high ct count is also possible.
> > > 
> > > With this patch, gc is called more frequently, not only for each new
> > > packet.
> > > 
> > 
> > How is it called more frequently? Before, it was calling nf_conncount_add()
> > for every packet which is indeed performing a GC inside, both
> > nf_conncount_add() and nf_conncount_gc_list() return immediately if a GC was
> > performed during the same jiffy.
> 
> Before this patch, without 'ct state new' in front, this was just
> adding duplicates, then count is wrong, ie. this is broken.

Yep, this was broken.  It did never occur to me to use this
for anything other than 'stop accepting new connections from
address/network when over x'.

> Assuming 'ct state new' in place, then gc is only called when new
> entries for the initial packet of a connection (still broken because
> duplicates due to retransmissions are possible).
> 
> My proposal:
> 
> - Follow a more conservative approach: Perform this gc cycle for
>   confirmed ct only when 'ct count over' evaluates true or 'ct count'
>   evaluates false.
> - For the confirmed ct case, stop gc inmediately when one slot is
>   released to short-circuit the walk.

Its currently ending gc early after 8 reaped entries.

> ... but still long walk could possible.

Yes.

> - More difficult: For the confirmed ct case, add a limit on the
>   maximum entries that are walked over in the gc iteration for each
>   packet. If no connections are found to be released, annotate the
>   entry at which this stops and a jiffy timestamp, to resume from where
>   the gc walk has stopped in the previous gc. The timestamp could be
>   used to decide whether to make a full gc walk or not. I mean, explore
>   a bit more advance gc logic now that this will be alled for every
>   packet.

There is this:
        if ((u32)jiffies == READ_ONCE(list->last_gc))
                return false;

i.e. we never collect more than once per jiffy and list.

(actually we could, if one cpu has sailed past the test
 and another cpu completed the list walk right after,
 but I don't think its a frequent thing to happen).

I don't think its worth playing with further logic here
because even if entry x was checked one jiffy ago there
is always a chance that this connection has been torn
down right now.

If you think its needed to avoid long list walks
(i.e. resume on next packet), then I think we need
a dedicated gc function that does this, e.g. by
relinking scanned-and-valid entries at the tail.

But for insert case we have to scan until we reach
end-of-list or evict at least one entry, else we report
incorrect ct count.