On Mon, May 09, 2022 at 06:47:33PM +0200, Florian Westphal wrote:
> Sven Auhagen <Sven.Auhagen@xxxxxxxxxxxx> wrote:
> > + if (unlikely(!test_bit(IPS_ASSURED_BIT, &flow->ct->status))) {
> > + spin_lock_bh(&flow->ct->lock);
> > + flow->ct->proto.tcp.state = TCP_CONNTRACK_ESTABLISHED;
> > + spin_unlock_bh(&flow->ct->lock);
> > + set_bit(IPS_ASSURED_BIT, &flow->ct->status);
>
> Uh. Whats going on here? ASSURED bit prevents early-eviction,
> it should not be set at random.
It is not set at random but when the TCP state is set to established.
The problem with the flow offload code at the moment is that it
is setting the TCP state to established on flow teardown disregarding
the current TCP state which might be CLOSED or FIN WAIT and therefore
creating a lot of long living dead state entries.
I need the tcp state to be ESTABLISHED at this point to distinguish
the right cases at flow teardown, because the TCP state at
flow creation is SYN_RECV and it will most likely stay like that
during offload.
It can transition to a different state though if the flow offload code
bumps up a packet to the nftables slow path in case of a processing
error or after flow teardown and before flow delete, because there is
a race condition at the moment.
After talking to Oz today, he rightfully mentioned that the offload
should not be allocated if the TCP state is not established to avoid
the hack here.
I will send a v2 with that implementation.
Best
Sven
