Re: [PATCH net 1/1] netfilter: conntrack: Check offload bit on table dump

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Roi,

On Sun, Jan 31, 2021 at 03:18:34PM +0200, Roi Dayan wrote:
[...]
> Hi Pablo,
> 
> We did more tests with just updating the timeout in the 2 callers
> and it's not enough. We reproduce the issue of rules being timed
> out just now frim different place.

Thanks for giving it a try to my suggestion, it was not correct.

> There is a 3rd caller nf_ct_gc_expired() which being called by 3
> other callers:
> ____nf_conntrack_find()
> nf_conntrack_tuple_taken()
> early_drop_list()

Hm. I'm not sure yet what path is triggering this bug.

Florian came up with the idea of setting a very large timeout for
offloaded flows (that are refreshed by the garbage collector) to avoid
the extra check from the packet path, so those 3 functions above never
hit the garbage collection path. This also applies for the ctnetlink
(conntrack -L) and the /proc/net/nf_conntrack sysctl paths that the
patch describes, those should not ever see an offloaded flow with a
small timeout.

nf_ct_offload_timeout() is called from:

#1 flow_offload_add() to set a very large timer.
#2 the garbage collector path, to refresh the timeout the very large
   offload timer.

Probably there is a race between setting the IPS_OFFLOAD and when
flow_offload_add() is called? Garbage collector gets in between and
zaps the connection. Is a newly offloaded connection that you observed
that is being removed?

> only early_drop_list() has a check to skip conns with offload bit
> but without extending the timeout.
> I didnt do a dump but the issue could be from the other 2 calls.
> 
> With current commit as is I didn't need to check more callers as I made
> sure all callers will skip the non-offload gc.
>
> Instead of updating more callers and there might be more callers
> later why current commit is not enough?
> We skip offloaded flows and soon gc_worker() will hit and will update
> the timeout anyway.

Another possibility would be to check for the offload bit from
nf_ct_is_expired(), which is coming slighty before nf_ct_should_gc().
But this is also in the ____nf_conntrack_find() path.

Florian?