From: Sven Auhagen <sven.auhagen@voleatech.de>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: netfilter-devel@vger.kernel.org, fw@strlen.de
Subject: Re: Flowtable race condition error
Date: Fri, 15 Mar 2024 14:46:23 +0100 [thread overview]
Message-ID: <36jf7ahr5msa345ugwgi6sf2oghlzlytxmjnp4me4hkuq4rymq@pbnvitdrxehl> (raw)
In-Reply-To: <ZfLz7NEwUnY_BEYZ@calendula>
On Thu, Mar 14, 2024 at 01:56:12PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Mar 14, 2024 at 01:43:52PM +0100, Sven Auhagen wrote:
> > On Thu, Mar 14, 2024 at 01:38:54PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Mar 14, 2024 at 12:30:30PM +0100, Sven Auhagen wrote:
> [...]
> > > > I found this out.
> > > > The state is deleted in the end because the flow_offload_fixup_ct
> > > > function is pulling the FIN_WAIT timeout and deducts the offload_timeout
> > > > from it. This is 0 or very close to 0 and therefore ct gc is deleting the state
> > > > more or less right away after the flow_offload_teardown is called
> > > > (for the second time).
> > >
> > > This used to be set to:
> > >
> > > timeout = tn->timeouts[TCP_CONNTRACK_ESTABLISHED];
> > >
> > > but after:
> > >
> > > commit e5eaac2beb54f0a16ff851125082d9faeb475572
> > > Author: Pablo Neira Ayuso <pablo@netfilter.org>
> > > Date: Tue May 17 10:44:14 2022 +0200
> > >
> > > netfilter: flowtable: fix TCP flow teardown
> > >
> > > it uses the current state.
> >
> > Yes since the TCP state might not be established anymore at that time.
> > Your patch will work but also give my TCP_FIN a very large timeout.
>
> Is that still possible? My patch also fixes up conntrack _before_
> the offload flag is cleared, so the packet in the reply direction
> either sees the already fixed up conntrack or it follows flowtable
> datapath.
>
> > I have successfully tested this version today:
> >
> > - if (timeout < 0)
> > - timeout = 0;
> > + // Have at least some time left on the state
> > + if (timeout < NF_FLOW_TIMEOUT)
> > + timeout = NF_FLOW_TIMEOUT;
> >
> > This makes sure that the timeout is not so big like ESTABLISHED but still enough
> > so the state does not time out right away.
>
> This also seems sensible to me. Currently it is using the last conntrack
> state that we have observed when conntrack handed over this flow to the
> flowtable, which is inaccurate in any case, and which could still be low
> depending on user-defined tcp conntrack timeouts (in case user decided
> to tweaks them).
If I take my patch and the part of your patch that moves up flow_offload_fixup_ct
to the top of the function it works now.
As expected, it still happens that a flow is ending up in FIN_WAIT OFFLOADED but
the state is no longer deleted right away and is eventually removed from the flowoffload
via gc and then goes on until the end.
[NEW] tcp 6 120 SYN_SENT src=192.168.7.100 dst=17.248.209.132 sport=54774 dport=443 [UNREPLIED] src=17.248.209.132 dst=87.138.198.79 sport=443 dport=30121 mark=25165825
[UPDATE] tcp 6 60 SYN_RECV src=192.168.7.100 dst=17.248.209.132 sport=54774 dport=443 src=17.248.209.132 dst=87.138.198.79 sport=443 dport=30121 mark=25165825
[UPDATE] tcp 6 86400 ESTABLISHED src=192.168.7.100 dst=17.248.209.132 sport=54774 dport=443 src=17.248.209.132 dst=87.138.198.79 sport=443 dport=30121 [OFFLOAD] mark=25165825
[DESTROY] tcp 6 TIME_WAIT src=192.168.4.81 dst=192.168.6.8 sport=54774 dport=8080 packets=6 bytes=1388 src=192.168.6.8 dst=192.168.4.81 sport=8080 dport=54774 packets=4 bytes=398 [ASSURED] mark=16777216 delta-time=120
[UPDATE] tcp 6 120 FIN_WAIT src=192.168.7.100 dst=17.248.209.132 sport=54774 dport=443 src=17.248.209.132 dst=87.138.198.79 sport=443 dport=30121 [OFFLOAD] mark=25165825
[UPDATE] tcp 6 30 LAST_ACK src=192.168.7.100 dst=17.248.209.132 sport=54774 dport=443 src=17.248.209.132 dst=87.138.198.79 sport=443 dport=30121 [OFFLOAD] mark=25165825
[UPDATE] tcp 6 10 CLOSE src=192.168.7.100 dst=17.248.209.132 sport=54774 dport=443 src=17.248.209.132 dst=87.138.198.79 sport=443 dport=30121 [ASSURED] mark=25165825
[DESTROY] tcp 6 CLOSE src=192.168.7.100 dst=17.248.209.132 sport=54774 dport=443 packets=31 bytes=5542 src=17.248.209.132 dst=87.138.198.79 sport=443 dport=30121 packets=35 bytes=8168 [ASSURED] mark=25165825 delta-time=50
prev parent reply other threads:[~2024-03-15 13:46 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-12 16:29 Flowtable race condition error Sven Auhagen
2024-03-13 14:55 ` Florian Westphal
2024-03-13 15:02 ` Florian Westphal
2024-03-13 15:06 ` Sven Auhagen
2024-03-13 15:25 ` Florian Westphal
2024-03-13 15:30 ` Sven Auhagen
2024-03-14 7:48 ` Sven Auhagen
2024-03-14 9:25 ` Florian Westphal
2024-03-14 10:08 ` Sven Auhagen
2024-03-14 11:21 ` Florian Westphal
2024-03-14 11:17 ` Pablo Neira Ayuso
2024-03-14 11:30 ` Sven Auhagen
2024-03-14 12:38 ` Pablo Neira Ayuso
2024-03-14 12:43 ` Sven Auhagen
2024-03-14 12:56 ` Pablo Neira Ayuso
2024-03-14 13:56 ` Sven Auhagen
2024-03-15 13:46 ` 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=36jf7ahr5msa345ugwgi6sf2oghlzlytxmjnp4me4hkuq4rymq@pbnvitdrxehl \
--to=sven.auhagen@voleatech.de \
--cc=fw@strlen.de \
--cc=netfilter-devel@vger.kernel.org \
--cc=pablo@netfilter.org \
/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).