Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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.
