[PATCH] conntrack_tcp: Reset the max ACK flag on SYN in ignore state

[PATCH] conntrack_tcp: Reset the max ACK flag on SYN in ignore state

[Ali Abdallah]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

Dear,

I would like to propose a small patch in order to fix an issue of some
RSTs being marked as invalid.

For an established connection, at some point the server sends a [RST,
ACK], the client reuse the same port and sends a SYN, the SYN packet is
ignored in CLOSE state

nf_ct_tcp: invalid packet ignored in state CLOSE ... SEQ=xxxxxx ACK=0 SYN

The server then answers that SYN packet with an [RST, ACK] SEQ=0,
ACK=xxxxxx+1

This new RST, because of the IP_CT_TCP_FLAG_MAXACK_SET being already set, is
erroneously marked as invalid with 'nf_ct_tcp: "invalid rst"'.

Kind regards,

-- 
Ali Abdallah | SUSE Linux L3 Engineer
GPG fingerprint: 51A0 F4A0 C8CF C98F 842E  A9A8 B945 56F8 1C85 D0D5

---

Here is the PATCH

From e9d4d3a70a19d8a3868d16c93281119797fb54df Mon Sep 17 00:00:00 2001
From: Ali Abdallah <aabdallah@suse.de>
Date: Thu, 8 Apr 2021 07:44:27 +0200
Subject: [PATCH] Reset the max ACK flag on SYN in ignore state

In ignore state, we let SYN goes in original, the server might respond
with RST/ACK, and that RST packet is erroneously dropped because of the
flag IP_CT_TCP_FLAG_MAXACK_SET being already set.
---
 net/netfilter/nf_conntrack_proto_tcp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index ec23330687a5..891a66e35afd 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -963,6 +963,9 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 
 			ct->proto.tcp.last_flags =
 			ct->proto.tcp.last_wscale = 0;
+			/* Reset the max ack flag so in case the server replies
+			 * with RST/ACK it will be marked as an invalid rst */
+			ct->proto.tcp.seen[dir].flags &= ~IP_CT_TCP_FLAG_MAXACK_SET;
 			tcp_options(skb, dataoff, th, &seen);
 			if (seen.flags & IP_CT_TCP_FLAG_WINDOW_SCALE) {
 				ct->proto.tcp.last_flags |=
-- 
2.26.2



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] conntrack_tcp: Reset the max ACK flag on SYN in ignore state

[Florian Westphal]

Ali Abdallah <ali.abdallah@suse.com> wrote:
> In ignore state, we let SYN goes in original, the server might respond
> with RST/ACK, and that RST packet is erroneously dropped because of the
> flag IP_CT_TCP_FLAG_MAXACK_SET being already set.
> ---
>  net/netfilter/nf_conntrack_proto_tcp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index ec23330687a5..891a66e35afd 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -963,6 +963,9 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
>  
>  			ct->proto.tcp.last_flags =
>  			ct->proto.tcp.last_wscale = 0;
> +			/* Reset the max ack flag so in case the server replies
> +			 * with RST/ACK it will be marked as an invalid rst */

"not be marked"?

> +			ct->proto.tcp.seen[dir].flags &= ~IP_CT_TCP_FLAG_MAXACK_SET;
>  			tcp_options(skb, dataoff, th, &seen);
>  			if (seen.flags & IP_CT_TCP_FLAG_WINDOW_SCALE) {

Re: [PATCH] conntrack_tcp: Reset the max ACK flag on SYN in ignore state

[Ali Abdallah]

Hi,

Please find out the updated patch with the fixed comment.

PS: I'm just wondering if isn't better to just reset the MAXACK_SET on
both directions once an RST is observed on the tracked connection, what
do you think?

Thanks,
Ali

---

From e9d4d3a70a19d8a3868d16c93281119797fb54df Mon Sep 17 00:00:00 2001
From: Ali Abdallah <aabdallah@suse.de>
Date: Thu, 13 Apr 2021 14:18:02 +0200
Subject: [PATCH] Reset the max ACK flag on SYN in ignore state

In ignore state, we let SYN goes in original, the server might respond
with RST/ACK, and that RST packet is erroneously dropped because of the
flag IP_CT_TCP_FLAG_MAXACK_SET being already set.
---
 net/netfilter/nf_conntrack_proto_tcp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index ec23330687a5..891a66e35afd 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -963,6 +963,9 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 
             ct->proto.tcp.last_flags =
             ct->proto.tcp.last_wscale = 0;
+            /* Reset the max ack flag so in case the server replies
+             * with RST/ACK it will not be marked as an invalid rst */
+            ct->proto.tcp.seen[dir].flags &= ~IP_CT_TCP_FLAG_MAXACK_SET;
             tcp_options(skb, dataoff, th, &seen);
             if (seen.flags & IP_CT_TCP_FLAG_WINDOW_SCALE) {
                 ct->proto.tcp.last_flags |=
-- 
2.26.2

On 08.04.2021 11:04, Florian Westphal wrote:
> Ali Abdallah <ali.abdallah@suse.com> wrote:
> > In ignore state, we let SYN goes in original, the server might respond
> > with RST/ACK, and that RST packet is erroneously dropped because of the
> > flag IP_CT_TCP_FLAG_MAXACK_SET being already set.
> > ---
> >  net/netfilter/nf_conntrack_proto_tcp.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > index ec23330687a5..891a66e35afd 100644
> > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > @@ -963,6 +963,9 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
> >  
> >  			ct->proto.tcp.last_flags =
> >  			ct->proto.tcp.last_wscale = 0;
> > +			/* Reset the max ack flag so in case the server replies
> > +			 * with RST/ACK it will be marked as an invalid rst */
> 
> "not be marked"?
> 
> > +			ct->proto.tcp.seen[dir].flags &= ~IP_CT_TCP_FLAG_MAXACK_SET;
> >  			tcp_options(skb, dataoff, th, &seen);
> >  			if (seen.flags & IP_CT_TCP_FLAG_WINDOW_SCALE) {
> 

-- 
Ali Abdallah | SUSE Linux L3 Engineer
GPG fingerprint: 51A0 F4A0 C8CF C98F 842E  A9A8 B945 56F8 1C85 D0D5

Re: [PATCH] conntrack_tcp: Reset the max ACK flag on SYN in ignore state

[Florian Westphal]

Ali Abdallah <ali.abdallah@suse.com> wrote:
> Hi,
> 
> Please find out the updated patch with the fixed comment.
> 
> PS: I'm just wondering if isn't better to just reset the MAXACK_SET on
> both directions once an RST is observed on the tracked connection, what
> do you think?

Mhh, can you share a patch?  Your patch clears it when a SYN is
observed, so I am not sure what you mean.

I think the patch is good; we only need to handle the case where we
let a SYN through, and might be out of state.

So, we only need to handle the reply dir, no?

Re: [PATCH] conntrack_tcp: Reset the max ACK flag on SYN in ignore state

[Ali Abdallah]

On 13.04.2021 15:45, Florian Westphal wrote:
> Mhh, can you share a patch?  Your patch clears it when a SYN is
> observed, so I am not sure what you mean.

We are also seeing some RST drops during live migration of a NFS
server (whose traffic goes through the filter before reaching the NFS
clients). Basically the NFS server will send RSTs during live migration,
and some of them are dropped, but we still don't understand the root
cause in this case. I will send another patch in case it turns out to be
an issue in in tcp conntrack.

> I think the patch is good; we only need to handle the case where we
> let a SYN through, and might be out of state.
> 
> So, we only need to handle the reply dir, no?

Yes, for the moment the proposed patch avoids the SYN -> RST -> drop
situation, so many thanks for taking it.

-- 
Ali Abdallah | SUSE Linux L3 Engineer
GPG fingerprint: 51A0 F4A0 C8CF C98F 842E  A9A8 B945 56F8 1C85 D0D5

Re: [PATCH] conntrack_tcp: Reset the max ACK flag on SYN in ignore state

[Florian Westphal]

Ali Abdallah <ali.abdallah@suse.com> wrote:

[..]

Can you resend your patch as a new submission, so patchwork can pick
it up properly?

The v2 was not, patchwork treated it as a comment to version 1.

Please also run your patch through scripts/checkpatch.pl before
doing so, the patch lacks at least a 'Signed-off-by' line.