Re: [PATCH] Avoid potentially erroneos RST drop.

[Florian Westphal <fw@strlen.de>]

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. So we reset the flag
> in this case.
> 
> Unfortunately that might not be enough, an out of order ACK in origin
> might reset it back, and we might end up again dropping a valid RST when
> the server responds with RST SEQ=0.
> 
> The patch disables also the RST check when we are not in established
> state and we receive an RST with SEQ=0 that is most likely a response to
> a SYN we had let it go through.

Ali, sorry for coming back to this again and again.

What do you think of this change?

Its an incremental change on top of your patch.

The only real change is that this will skip window check if
conntrack thinks connection is closing already.

In addition, tcp window check is skipped in that case.

This is supposed to expedite conntrack eviction in case of tuple reuse
by some nat/pat middlebox, or a peer that has lower timeouts than
conntrack before a port is re-used.

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -834,6 +834,22 @@ static noinline bool tcp_new(struct nf_conn *ct, const struct sk_buff *skb,
 	return true;
 }
 
+static bool tcp_can_early_drop(const struct nf_conn *ct)
+{
+	switch (ct->proto.tcp.state) {
+	case TCP_CONNTRACK_FIN_WAIT:
+	case TCP_CONNTRACK_LAST_ACK:
+	case TCP_CONNTRACK_TIME_WAIT:
+	case TCP_CONNTRACK_CLOSE:
+	case TCP_CONNTRACK_CLOSE_WAIT:
+		return true;
+	default:
+		break;
+	}
+
+	return false;
+}
+
 /* Returns verdict for packet, or -1 for invalid. */
 int nf_conntrack_tcp_packet(struct nf_conn *ct,
 			    struct sk_buff *skb,
@@ -1053,8 +1069,16 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 			/* If we are not in established state, and an RST is
 			 * observed with SEQ=0, this is most likely an answer
 			 * to a SYN we had let go through above.
+			 *
+			 * Also expedite conntrack destruction: If we were already
+			 * closing, peer or NAT/PAT might already have reused tuple.
 			 */
-			if (seq == 0 && !nf_conntrack_tcp_established(ct))
+			if (!nf_conntrack_tcp_established(ct)) {
+				if (seq == 0 || tcp_can_early_drop(ct))
+					goto in_window;
+			}
+
+			if (seq == ct->proto.tcp.seen[!dir].td_maxack)
 				break;
 
 			if (before(seq, ct->proto.tcp.seen[!dir].td_maxack)) {
@@ -1066,10 +1090,6 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 				return -NF_ACCEPT;
 			}
 
-			if (!nf_conntrack_tcp_established(ct) ||
-			    seq == ct->proto.tcp.seen[!dir].td_maxack)
-				break;
-
 			/* Check if rst is part of train, such as
 			 *   foo:80 > bar:4379: P, 235946583:235946602(19) ack 42
 			 *   foo:80 > bar:4379: R, 235946602:235946602(0)  ack 42
@@ -1181,22 +1201,6 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 	return NF_ACCEPT;
 }
 
-static bool tcp_can_early_drop(const struct nf_conn *ct)
-{
-	switch (ct->proto.tcp.state) {
-	case TCP_CONNTRACK_FIN_WAIT:
-	case TCP_CONNTRACK_LAST_ACK:
-	case TCP_CONNTRACK_TIME_WAIT:
-	case TCP_CONNTRACK_CLOSE:
-	case TCP_CONNTRACK_CLOSE_WAIT:
-		return true;
-	default:
-		break;
-	}
-
-	return false;
-}
-
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK)
 
 #include <linux/netfilter/nfnetlink.h>