On Thu, Apr 11, 2024 at 02:13:20PM +0200, Florian Westphal wrote:
> 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.
So flow entry would remain in place if fin packet is seen. Then, an
ack packet will follow fastpath while another fin packet in the other
direction will follow classic.
> > > 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.
Conntrack will have to deal then with an entry that is completely
out-of-sync, will that work? At least a fixup to disable sequence
tracking will be required?
> 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.
I see.
> > > 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;
> }
I see, I misunderstood.
> > > 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.
If fin does not tear down the flow entry, then the flow entry remains
in place and it holds a reference to the conntrack object, which will
not be release until 30 seconds of no traffic activity, right?
Maybe I am just missing part of what you have in mind, I still don't
see the big picture.
Would you make a quick summary of the proposed new logic?
Thanks!
