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

[Roi Dayan <roid@nvidia.com>]

On 2021-02-01 1:50 PM, Florian Westphal wrote:
> Roi Dayan <roid@nvidia.com> wrote:
>>>> 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?
>>>
>>
>> yes. the flows being removed are newly offloaded connections.
> 
> If they are new, how can they be timed out already?
> 
> TCP initial timeout is one minute, UDP 30 seconds.
> That should surely be enough to do flow_offload_add (which extends
> the timeout)?

Yes, flow_offload_add() extends the timeout. but it needs to finish.

> 
> Maybe something is doing flow_offload_add() for unconfirmed conntrack?
> 
> In unconfirmed conntrack case, ct->timeout is absolute timeout value, e.g. for
> tcp it will be set to 60 * HZ.

When I hit the issue I printed jiffies and ct->timeout and saw they are
the same or very close but not an absolute number.

> 
> conntrack confirmation adds jiffies32 to it to make it relative
> to current time (this is before insertion into the conntrack table,
> so GC isn't supposed to happen before this).
> 

We hit this issue before more easily and pushed this fix

4203b19c2796 netfilter: flowtable: Set offload timeout when adding flow

That commit changed flow_offload_add() to extend ct timeout because on
we noticed on busy systems GC didn't finish a full iteration on all
conns and conns were cleaned.
I think we might have the same issue.

tcf_ct_flow_table_add() set the offload bit and calls flow_offload_add()

We do know the offload bit is set when conn it deleted, so we hit the
issue where timeout being tested after tcf_ct_flow_table_add() was 
called but before ct timeout was fixed. so flow_offload_add() didn't
finish and GC didn't start, or did start but did not finish full
iteration.

> In any case adding test for the offload bit seems to be papering over
> invalid/broken ct->timeout value.
>