On Thu, Apr 11, 2024 at 05:50:30PM +0200, Pablo Neira Ayuso wrote:
> 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!
Hi Pablo,
I tested your last patch but it makes no difference:
[NEW] tcp 6 120 SYN_SENT src=192.168.7.126 dst=157.240.223.63 sport=63461 dport=443 [UNREPLIED] src=157.240.223.63 dst=87.138.198.79 sport=443 dport=13354 mark=25165825
[UPDATE] tcp 6 60 SYN_RECV src=192.168.7.126 dst=157.240.223.63 sport=63461 dport=443 src=157.240.223.63 dst=87.138.198.79 sport=443 dport=13354 mark=25165825
[UPDATE] tcp 6 86400 ESTABLISHED src=192.168.7.126 dst=157.240.223.63 sport=63461 dport=443 src=157.240.223.63 dst=87.138.198.79 sport=443 dport=13354 [OFFLOAD] mark=25165825
[UPDATE] tcp 6 120 FIN_WAIT src=192.168.7.126 dst=157.240.223.63 sport=63461 dport=443 src=157.240.223.63 dst=87.138.198.79 sport=443 dport=13354 [OFFLOAD] mark=25165825
[UPDATE] tcp 6 30 LAST_ACK src=192.168.7.126 dst=157.240.223.63 sport=63461 dport=443 src=157.240.223.63 dst=87.138.198.79 sport=443 dport=13354 [OFFLOAD] mark=25165825
[DESTROY] tcp 6 LAST_ACK src=192.168.7.126 dst=157.240.223.63 sport=63461 dport=443 packets=10 bytes=790 src=157.240.223.63 dst=87.138.198.79 sport=443 dport=13354 packets=19 bytes=5061 [ASSURED] mark=25165825 delta-time=138
Best
Sven
