Thread (13 messages) flat view 13 messages, 2 authors, 2021-07-07
STALE1857d

Revision v1 of 112 in this series.

Revisions (112)
  1. v1 [diff vs current]
  2. v1 [diff vs current]
  3. v1 [diff vs current]
  4. v1 [diff vs current]
  5. v1 [diff vs current]
  6. v1 [diff vs current]
  7. v1 [diff vs current]
  8. v1 [diff vs current]
  9. v1 [diff vs current]
  10. v1 [diff vs current]
  11. v1 [diff vs current]
  12. v1 [diff vs current]
  13. v1 [diff vs current]
  14. v1 [diff vs current]
  15. v1 [diff vs current]
  16. v1 [diff vs current]
  17. v1 [diff vs current]
  18. v1 [diff vs current]
  19. v1 [diff vs current]
  20. v1 [diff vs current]
  21. v1 [diff vs current]
  22. v1 [diff vs current]
  23. v1 [diff vs current]
  24. v1 [diff vs current]
  25. v1 [diff vs current]
  26. v1 [diff vs current]
  27. v1 [diff vs current]
  28. v1 [diff vs current]
  29. v1 [diff vs current]
  30. v1 [diff vs current]
  31. v1 [diff vs current]
  32. v1 [diff vs current]
  33. v1 [diff vs current]
  34. v1 [diff vs current]
  35. v1 [diff vs current]
  36. v1 [diff vs current]
  37. v1 [diff vs current]
  38. v1 [diff vs current]
  39. v1 [diff vs current]
  40. v1 [diff vs current]
  41. v1 [diff vs current]
  42. v1 [diff vs current]
  43. v1 [diff vs current]
  44. v1 [diff vs current]
  45. v1 [diff vs current]
  46. v1 [diff vs current]
  47. v1 [diff vs current]
  48. v1 [diff vs current]
  49. v1 [diff vs current]
  50. v1 [diff vs current]
  51. v1 [diff vs current]
  52. v1 [diff vs current]
  53. v1 [diff vs current]
  54. v1 [diff vs current]
  55. v1 [diff vs current]
  56. v1 [diff vs current]
  57. v1 [diff vs current]
  58. v1 [diff vs current]
  59. v1 [diff vs current]
  60. v1 [diff vs current]
  61. v1 [diff vs current]
  62. v1 [diff vs current]
  63. v1 [diff vs current]
  64. v1 [diff vs current]
  65. v1 [diff vs current]
  66. v1 [diff vs current]
  67. v1 current
  68. v1 [diff vs current]
  69. v1 [diff vs current]
  70. v2 [diff vs current]
  71. v1 [diff vs current]
  72. v1 [diff vs current]
  73. v1 [diff vs current]
  74. v1 [diff vs current]
  75. v1 [diff vs current]
  76. v1 [diff vs current]
  77. v1 [diff vs current]
  78. v1 [diff vs current]
  79. v1 [diff vs current]
  80. v1 [diff vs current]
  81. v1 [diff vs current]
  82. v1 [diff vs current]
  83. v1 [diff vs current]
  84. v1 [diff vs current]
  85. v1 [diff vs current]
  86. v1 [diff vs current]
  87. v1 [diff vs current]
  88. v2 [diff vs current]
  89. v1 [diff vs current]
  90. v1 [diff vs current]
  91. v1 [diff vs current]
  92. v2 [diff vs current]
  93. v1 [diff vs current]
  94. v1 [diff vs current]
  95. v1 [diff vs current]
  96. v2 [diff vs current]
  97. v3 [diff vs current]
  98. v1 [diff vs current]
  99. v1 [diff vs current]
  100. v2 [diff vs current]
  101. v1 [diff vs current]
  102. v1 [diff vs current]
  103. v1 [diff vs current]
  104. v2 [diff vs current]
  105. v1 [diff vs current]
  106. v2 [diff vs current]
  107. v1 [diff vs current]
  108. v1 [diff vs current]
  109. v1 [diff vs current]
  110. v1 [diff vs current]
  111. v2 [diff vs current]
  112. v1 [diff vs current]

[PATCH net 06/11] netfilter: conntrack: improve RST handling when tuple is re-used

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: 2021-07-07 16:19:07
Also in: netfilter-devel
Subsystem: netfilter, networking [general], the rest · Maintainers: Pablo Neira Ayuso, Florian Westphal, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds 

From: Ali Abdallah <redacted>

If we receive a SYN packet in original direction on an existing
connection tracking entry, we let this SYN through because conntrack
might be out-of-sync.

Conntrack gets back in sync when server responds with SYN/ACK and state
gets updated accordingly.

However, if server replies with RST, this packet might be marked as
INVALID because td_maxack value reflects the *old* conntrack state
and not the state of the originator of the RST.

Avoid td_maxack-based checks if previous packet was a SYN.

Unfortunately that is not be enough: an out of order ACK in original
direction updates last_index, so we still end up marking valid RST.

Thus disable the sequence check when we are not in established state and
the received RST has a sequence of 0.

Because marking RSTs as invalid usually leads to unwanted timeouts,
also skip RST sequence checks if a conntrack entry is already closing.

Such entries can already be evicted via GC in case the table is full.

Co-developed-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Ali Abdallah <redacted>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_proto_tcp.c | 53 +++++++++++++++++---------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index eb4de92077a8..b8ff67671e93 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -823,6 +823,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,
@@ -1030,9 +1046,28 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
 		if (index != TCP_RST_SET)
 			break;
 
-		if (ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET) {
+		/* If we are closing, tuple might have been re-used already.
+		 * last_index, last_ack, and all other ct fields used for
+		 * sequence/window validation are outdated in that case.
+		 *
+		 * As the conntrack can already be expired by GC under pressure,
+		 * just skip validation checks.
+		 */
+		if (tcp_can_early_drop(ct))
+			goto in_window;
+
+		/* td_maxack might be outdated if we let a SYN through earlier */
+		if ((ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET) &&
+		    ct->proto.tcp.last_index != TCP_SYN_SET) {
 			u32 seq = ntohl(th->seq);
 
+			/* If we are not in established state and SEQ=0 this is most
+			 * likely an answer to a SYN we let go through above (last_index
+			 * can be updated due to out-of-order ACKs).
+			 */
+			if (seq == 0 && !nf_conntrack_tcp_established(ct))
+				break;
+
 			if (before(seq, ct->proto.tcp.seen[!dir].td_maxack)) {
 				/* Invalid RST  */
 				spin_unlock_bh(&ct->lock);
@@ -1165,22 +1200,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>
-- 
2.20.1
Other recent patches touching these files
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help