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

From: Florian Westphal <[email-masked]>
Date: Mon, 1 Feb 2021 12:50:36 +0100

Roi Dayan <[email-masked]> 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)?

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.

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).

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