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

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 <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) {

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 |=

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?

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 <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.