Re: [PATCH nf] netfilter: flowtable: infer TCP state and timeout before flow teardown

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

On Thu, Apr 11, 2024 at 01:05:04PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I can also see IP_CT_TCP_FLAG_CLOSE_INIT is not set on when ct->state
> > is adjusted to _FIN_WAIT state in the fixup routine.
> 
> Unrelated to this patch, but I think that there is an increasing and
> disturbing amount of code that attempts to 'fix' the ct state.

I originally started this infrastructure without the fixup, and there
was a bit of fuzz about it because people complained that TCP
connection somewhat becomes stateless and would get stuck too after
fin packet is seen.

There is also connlimit, which would be distorted after the TCP flow
becomes stateless when handing it over to the flowtable.

Another reason is that hardware offload folks need this, otherwise
hardware entry remains too long in the hardware flowtable.

> I don't think its right and I intend to remove all of these "fixups"
> of the conntrack state from flowtable infra.
> 
> I see no reason whatsoever why we need to do this, fin can be passed up
> to conntrack and conntrack can and should handle this without any extra
> mucking with the nf_conn state fields from flowtable infra.

You mean, just let the fin packet go without tearing down the flow
entry?

> The only cases where I see why we need to take action from
> flowtable layer are:
> 
> 1. timeout extensions of nf_conn from gc worker to prevent eviction
> 2. removal of the flowtable entry on RST reception. Don't see why that
>    needs state fixup of nf_conn.

Remove it right away then is what you propose?

> 3. removal of the flowtable entry on hard failure of
>    output routines, e.g. because route is stale.
>    Don't see why that needs any nf_conn changes either.

if dst is stale, packet needs to go back to classic path to get a
fresh route.

> My impression is that all these conditionals paper over some other
> bugs, for example gc_worker extending timeout is racing with the
> datapath, this needs to be fixed first.

That sounds good. But I am afraid some folks will not be happy if TCP
flow becomes stateless again.

> I plan to work on this after the selftest fixups.

Thanks.