[PATCH net 1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only

[PATCH net 1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only

[Pablo Neira Ayuso]

From: Florian Westphal <fw@strlen.de>

Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
pinpointed to nf_conntrack tcp_in_window() bug.

tcp trace shows following sequence:

I > R Flags [S], seq 3451342529, win 62580, options [.. tfo [|tcp]>
R > I Flags [S.], seq 2699962254, ack 3451342530, win 65535, options [..]
R > I Flags [P.], seq 1:89, ack 1, [..]

Note 3rd ACK is from responder to initiator so following branch is taken:
    } else if (((state->state == TCP_CONNTRACK_SYN_SENT
               && dir == IP_CT_DIR_ORIGINAL)
               || (state->state == TCP_CONNTRACK_SYN_RECV
               && dir == IP_CT_DIR_REPLY))
               && after(end, sender->td_end)) {

... because state == TCP_CONNTRACK_SYN_RECV and dir is REPLY.
This causes the scaling factor to be reset to 0: window scale option
is only present in syn(ack) packets.  This in turn makes nf_conntrack
mark valid packets as out-of-window.

This was always broken, it exists even in original commit where
window tracking was added to ip_conntrack (nf_conntrack predecessor)
in 2.6.9-rc1 kernel.

Restrict to 'tcph->syn', just like the 3rd condtional added in
commit 82b72cb94666 ("netfilter: conntrack: re-init state for retransmitted syn-ack").

Upon closer look, those conditionals/branches can be merged:

Because earlier checks prevent syn-ack from showing up in
original direction, the 'dir' checks in the conditional quoted above are
redundant, remove them. Return early for pure syn retransmitted in reply
direction (simultaneous open).

Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
Reported-by: Jaco Kroon <jaco@uls.co.za>
Signed-off-by: Florian Westphal <fw@strlen.de>
Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 8ec55cd72572..204a5cdff5b1 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -556,24 +556,14 @@ static bool tcp_in_window(struct nf_conn *ct,
 			}
 
 		}
-	} else if (((state->state == TCP_CONNTRACK_SYN_SENT
-		     && dir == IP_CT_DIR_ORIGINAL)
-		   || (state->state == TCP_CONNTRACK_SYN_RECV
-		     && dir == IP_CT_DIR_REPLY))
-		   && after(end, sender->td_end)) {
+	} else if (tcph->syn &&
+		   after(end, sender->td_end) &&
+		   (state->state == TCP_CONNTRACK_SYN_SENT ||
+		    state->state == TCP_CONNTRACK_SYN_RECV)) {
 		/*
 		 * RFC 793: "if a TCP is reinitialized ... then it need
 		 * not wait at all; it must only be sure to use sequence
 		 * numbers larger than those recently used."
-		 */
-		sender->td_end =
-		sender->td_maxend = end;
-		sender->td_maxwin = (win == 0 ? 1 : win);
-
-		tcp_options(skb, dataoff, tcph, sender);
-	} else if (tcph->syn && dir == IP_CT_DIR_REPLY &&
-		   state->state == TCP_CONNTRACK_SYN_SENT) {
-		/* Retransmitted syn-ack, or syn (simultaneous open).
 		 *
 		 * Re-init state for this direction, just like for the first
 		 * syn(-ack) reply, it might differ in seq, ack or tcp options.
@@ -581,7 +571,8 @@ static bool tcp_in_window(struct nf_conn *ct,
 		tcp_init_sender(sender, receiver,
 				skb, dataoff, tcph,
 				end, win);
-		if (!tcph->ack)
+
+		if (dir == IP_CT_DIR_REPLY && !tcph->ack)
 			return true;
 	}
 
-- 
2.30.2

Re: [PATCH net 1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Pablo Neira Ayuso <pablo@netfilter.org>:

On Thu, 28 Apr 2022 16:21:07 +0200 you wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
> pinpointed to nf_conntrack tcp_in_window() bug.
> 
> tcp trace shows following sequence:
> 
> [...]

Here is the summary with links:
  - [net,1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only
    https://git.kernel.org/netdev/net/c/c7aab4f17021
  - [net,2/3] netfilter: conntrack: fix udp offload timeout sysctl
    https://git.kernel.org/netdev/net/c/626873c446f7
  - [net,3/3] netfilter: nft_socket: only do sk lookups when indev is available
    https://git.kernel.org/netdev/net/c/743b83f15d40

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only

[Neal Cardwell]

On Thu, Apr 28, 2022 at 1:00 PM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This series was applied to netdev/net.git (master)
> by Pablo Neira Ayuso <pablo@netfilter.org>:
>
> On Thu, 28 Apr 2022 16:21:07 +0200 you wrote:
> > From: Florian Westphal <fw@strlen.de>
> >
> > Jaco Kroon reported tcp problems that Eric Dumazet and Neal Cardwell
> > pinpointed to nf_conntrack tcp_in_window() bug.
> >
> > tcp trace shows following sequence:
> >
> > [...]
>
> Here is the summary with links:
>   - [net,1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only
>     https://git.kernel.org/netdev/net/c/c7aab4f17021
>   - [net,2/3] netfilter: conntrack: fix udp offload timeout sysctl
>     https://git.kernel.org/netdev/net/c/626873c446f7
>   - [net,3/3] netfilter: nft_socket: only do sk lookups when indev is available
>     https://git.kernel.org/netdev/net/c/743b83f15d40
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html

This first commit is an important bug fix for a serious bug that causes
TCP connection hangs for users of TCP fast open and nf_conntrack:

  c7aab4f17021b netfilter: nf_conntrack_tcp: re-init for syn packets only

We are continuing to get reports about the bug that this commit fixes.

It seems this fix was only backported to v5.17 stable release, and not further,
due to a cherry-pick conflict, because this fix implicitly depends on a
slightly earlier v5.17 fix in the same spot:

  82b72cb94666 netfilter: conntrack: re-init state for retransmitted syn-ack

I manually verified that the fix c7aab4f17021b can be cleanly cherry-picked
into the oldest (v4.9.325) and newest (v5.15.60) longterm release kernels as
long as we first cherry-pick that related fix that it implicitly depends on:

82b72cb94666b3dbd7152bb9f441b068af7a921b
netfilter: conntrack: re-init state for retransmitted syn-ack

c7aab4f17021b636a0ee75bcf28e06fb7c94ab48
netfilter: nf_conntrack_tcp: re-init for syn packets only

So would it be possible to backport both of those fixes with the following
cherry-picks, to all LTS stable releases?

git cherry-pick 82b72cb94666b3dbd7152bb9f441b068af7a921b
git cherry-pick c7aab4f17021b636a0ee75bcf28e06fb7c94ab48

Thanks!

Best Regards,
neal

Re: [PATCH net 1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only

[Jakub Kicinski]

On Fri, 12 Aug 2022 09:34:14 -0400 Neal Cardwell wrote:
> This first commit is an important bug fix for a serious bug that causes
> TCP connection hangs for users of TCP fast open and nf_conntrack:
> 
>   c7aab4f17021b netfilter: nf_conntrack_tcp: re-init for syn packets only
> 
> We are continuing to get reports about the bug that this commit fixes.
> 
> It seems this fix was only backported to v5.17 stable release, and not further,
> due to a cherry-pick conflict, because this fix implicitly depends on a
> slightly earlier v5.17 fix in the same spot:
> 
>   82b72cb94666 netfilter: conntrack: re-init state for retransmitted syn-ack
> 
> I manually verified that the fix c7aab4f17021b can be cleanly cherry-picked
> into the oldest (v4.9.325) and newest (v5.15.60) longterm release kernels as
> long as we first cherry-pick that related fix that it implicitly depends on:
> 
> 82b72cb94666b3dbd7152bb9f441b068af7a921b
> netfilter: conntrack: re-init state for retransmitted syn-ack
> 
> c7aab4f17021b636a0ee75bcf28e06fb7c94ab48
> netfilter: nf_conntrack_tcp: re-init for syn packets only
> 
> So would it be possible to backport both of those fixes with the following
> cherry-picks, to all LTS stable releases?
> 
> git cherry-pick 82b72cb94666b3dbd7152bb9f441b068af7a921b
> git cherry-pick c7aab4f17021b636a0ee75bcf28e06fb7c94ab48

Thanks a lot Neal! FWIW we have recently changed our process and no
longer handle stable submissions ourselves, so in the future feel free
to talk directly to stable@ (and add CC: stable@ tags to patches).

I'm adding stable@, let's see if Greg & team can pick things up based
on your instructions :)

Re: [PATCH net 1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only

[Thomas Backlund]

Den 2022-08-12 kl. 22:17, skrev Jakub Kicinski:
> On Fri, 12 Aug 2022 09:34:14 -0400 Neal Cardwell wrote:
>> This first commit is an important bug fix for a serious bug that causes
>> TCP connection hangs for users of TCP fast open and nf_conntrack:
>>
>>    c7aab4f17021b netfilter: nf_conntrack_tcp: re-init for syn packets only
>>
>> We are continuing to get reports about the bug that this commit fixes.
>>
>> It seems this fix was only backported to v5.17 stable release, and not further,
>> due to a cherry-pick conflict, because this fix implicitly depends on a
>> slightly earlier v5.17 fix in the same spot:
>>
>>    82b72cb94666 netfilter: conntrack: re-init state for retransmitted syn-ack
>>
>> I manually verified that the fix c7aab4f17021b can be cleanly cherry-picked
>> into the oldest (v4.9.325) and newest (v5.15.60) longterm release kernels as
>> long as we first cherry-pick that related fix that it implicitly depends on:
>>
>> 82b72cb94666b3dbd7152bb9f441b068af7a921b
>> netfilter: conntrack: re-init state for retransmitted syn-ack
>>
>> c7aab4f17021b636a0ee75bcf28e06fb7c94ab48
>> netfilter: nf_conntrack_tcp: re-init for syn packets only
>>
>> So would it be possible to backport both of those fixes with the following
>> cherry-picks, to all LTS stable releases?
>>
>> git cherry-pick 82b72cb94666b3dbd7152bb9f441b068af7a921b
>> git cherry-pick c7aab4f17021b636a0ee75bcf28e06fb7c94ab48
>
> Thanks a lot Neal! FWIW we have recently changed our process and no
> longer handle stable submissions ourselves, so in the future feel free
> to talk directly to stable@ (and add CC: stable@ tags to patches).
>
> I'm adding stable@, let's see if Greg & team can pick things up based
> on your instructions :)
>

besides testing that they apply,
one should also check that the resulting code actually builds...

net/netfilter/nf_conntrack_proto_tcp.c: In function 'tcp_in_window':
net/netfilter/nf_conntrack_proto_tcp.c:560:3: error: implicit
declaration of function 'tcp_init_sender'; did you mean 'tcp_init_cwnd'?
[-Werror=implicit-function-declaration]



So this one is also needed:
cc4f9d62037ebcb811f4908bba2986c01df1bd50
netfilter: conntrack: move synack init code to helper

for it to actually build on 5.15


--
Thomas

Re: [PATCH net 1/3] netfilter: nf_conntrack_tcp: re-init for syn packets only

[Neal Cardwell]

 theOn Fri, Aug 12, 2022 at 9:27 PM Thomas Backlund <tmb@tmb.nu> wrote:
>
> Den 2022-08-12 kl. 22:17, skrev Jakub Kicinski:
> > On Fri, 12 Aug 2022 09:34:14 -0400 Neal Cardwell wrote:
> >> This first commit is an important bug fix for a serious bug that causes
> >> TCP connection hangs for users of TCP fast open and nf_conntrack:
> >>
> >>    c7aab4f17021b netfilter: nf_conntrack_tcp: re-init for syn packets only
> >>
> >> We are continuing to get reports about the bug that this commit fixes.
> >>
> >> It seems this fix was only backported to v5.17 stable release, and not further,
> >> due to a cherry-pick conflict, because this fix implicitly depends on a
> >> slightly earlier v5.17 fix in the same spot:
> >>
> >>    82b72cb94666 netfilter: conntrack: re-init state for retransmitted syn-ack
> >>
> >> I manually verified that the fix c7aab4f17021b can be cleanly cherry-picked
> >> into the oldest (v4.9.325) and newest (v5.15.60) longterm release kernels as
> >> long as we first cherry-pick that related fix that it implicitly depends on:
> >>
> >> 82b72cb94666b3dbd7152bb9f441b068af7a921b
> >> netfilter: conntrack: re-init state for retransmitted syn-ack
> >>
> >> c7aab4f17021b636a0ee75bcf28e06fb7c94ab48
> >> netfilter: nf_conntrack_tcp: re-init for syn packets only
> >>
> >> So would it be possible to backport both of those fixes with the following
> >> cherry-picks, to all LTS stable releases?
> >>
> >> git cherry-pick 82b72cb94666b3dbd7152bb9f441b068af7a921b
> >> git cherry-pick c7aab4f17021b636a0ee75bcf28e06fb7c94ab48
> >
> > Thanks a lot Neal! FWIW we have recently changed our process and no
> > longer handle stable submissions ourselves, so in the future feel free
> > to talk directly to stable@ (and add CC: stable@ tags to patches).
> >
> > I'm adding stable@, let's see if Greg & team can pick things up based
> > on your instructions :)
> >
>
> besides testing that they apply,
> one should also check that the resulting code actually builds...
>
> net/netfilter/nf_conntrack_proto_tcp.c: In function 'tcp_in_window':
> net/netfilter/nf_conntrack_proto_tcp.c:560:3: error: implicit
> declaration of function 'tcp_init_sender'; did you mean 'tcp_init_cwnd'?
> [-Werror=implicit-function-declaration]
>
>
>
> So this one is also needed:
> cc4f9d62037ebcb811f4908bba2986c01df1bd50
> netfilter: conntrack: move synack init code to helper
>
> for it to actually build on 5.15

Thomas – thanks for catching that!

Florian, can you please confirm that the following patch series would
be a correct and sensible set of cherry-picks to backport to stable to
fix this critical nf_conntrack_tcp bug that is black-holing TCP Fast
Open connections?

# netfilter: conntrack: move synack init code to helper
git cherry-pick cc4f9d62037ebcb811f4908bba2986c01df1bd50

# netfilter: conntrack: re-init state for retransmitted syn-ack
git cherry-pick 82b72cb94666b3dbd7152bb9f441b068af7a921b

# netfilter: nf_conntrack_tcp: re-init for syn packets only
git cherry-pick c7aab4f17021b636a0ee75bcf28e06fb7c94ab48

(When applied to v4.9.325 the first one needs a trivial conflict
resolution, but the second two apply cleanly. And the kernel
compiles.)

thanks,
neal