From: Sven Auhagen <sven.auhagen@voleatech.de>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Florian Westphal <fw@strlen.de>,
netfilter-devel@vger.kernel.org, cratiu@nvidia.com,
ozsh@nvidia.com, vladbu@nvidia.com, gal@nvidia.com
Subject: Re: [PATCH nf] netfilter: flowtable: infer TCP state and timeout before flow teardown
Date: Fri, 19 Apr 2024 09:47:56 +0200 [thread overview]
Message-ID: <yjjoc762gpm3gsdwyqpsbgw6wsybl7lhk3pqeygmv3acy76u3w@znzu42tkxosa> (raw)
In-Reply-To: <ZhgGxoJyo_1vhPN_@calendula>
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@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.
>
> 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
prev parent reply other threads:[~2024-04-19 7:48 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-18 9:39 [PATCH nf] netfilter: flowtable: infer TCP state and timeout before flow teardown Pablo Neira Ayuso
2024-03-18 10:05 ` Sven Auhagen
2024-03-20 8:39 ` Sven Auhagen
2024-03-20 8:45 ` Pablo Neira Ayuso
2024-03-20 8:49 ` Sven Auhagen
2024-03-20 9:07 ` Pablo Neira Ayuso
2024-03-20 9:20 ` Sven Auhagen
2024-03-20 9:27 ` Pablo Neira Ayuso
2024-03-20 9:31 ` Sven Auhagen
2024-03-20 9:51 ` Pablo Neira Ayuso
2024-03-20 10:13 ` Sven Auhagen
2024-03-20 10:36 ` Pablo Neira Ayuso
2024-03-20 10:38 ` Sven Auhagen
2024-03-20 10:29 ` Sven Auhagen
2024-03-20 10:47 ` Pablo Neira Ayuso
2024-03-20 11:15 ` Sven Auhagen
2024-03-20 12:37 ` Pablo Neira Ayuso
2024-03-20 13:37 ` Sven Auhagen
2024-04-08 5:24 ` Sven Auhagen
2024-04-09 11:11 ` Pablo Neira Ayuso
2024-04-09 11:35 ` Sven Auhagen
2024-04-11 9:27 ` Pablo Neira Ayuso
2024-04-11 11:05 ` Florian Westphal
2024-04-11 11:40 ` Pablo Neira Ayuso
2024-04-11 12:13 ` Florian Westphal
2024-04-11 15:50 ` Pablo Neira Ayuso
2024-04-19 7:47 ` Sven Auhagen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=yjjoc762gpm3gsdwyqpsbgw6wsybl7lhk3pqeygmv3acy76u3w@znzu42tkxosa \
--to=sven.auhagen@voleatech.de \
--cc=cratiu@nvidia.com \
--cc=fw@strlen.de \
--cc=gal@nvidia.com \
--cc=netfilter-devel@vger.kernel.org \
--cc=ozsh@nvidia.com \
--cc=pablo@netfilter.org \
--cc=vladbu@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).