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

[Florian Westphal <fw@strlen.de>]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 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?

Yes, thats what I meant.  RST would still remove 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?

Isn't that what is happeing right now?  We set TEARDOWN bit and
remove OFFLOAD bit from nf_conn.

I think we should NOT do it for FIN and let conntrack handle this,
but we should still do it for RST.

Technically I think we could also skip it for RST but I don't see
a big advantage.  For FIN it will keep offloading in place which is
a win for connetions where one end still sends more data.

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

Yes, sure.  But I would keep the teardown in place that we have now,
I meant to say that the current code makes sense to me, i.e.:

if (!nf_flow_dst_check(&tuplehash->tuple)) {
	flow_offload_teardown(flow);
	return 0;
}

> > 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 do not know what that means.  There can never be a flowtable entry
without a backing nf_conn, so I don't know what stateless means in this
context.