tcp: Unconditionally force ACK for all !SYN, !RST packets

Currently we set ACK on flags packets only when the acknowledged byte pointer has advanced, or we hadn't previously set a window. This means in particular that we can send a window update with no ACK flag, which doesn't appear to be correct. RFC 9293 requires a receiver to ignore such a packet [0], and indeed it appears that every non-SYN, non-RST packet should have the ACK flag. The reason for the existing logic, rather than always forcing an ACK seems to be to avoid having the packet mistaken as a duplicate ACK which might trigger a fast retransmit. However, earlier tests in the function mean we won't reach here if we don't have either an advance in the ack pointer - which will already set the ACK flag, or a window update - which shouldn't trigger a fast retransmit. [0] https://www.ietf.org/rfc/rfc9293.html#section-3.10.7.4-2.5.2.1 Link: https://github.com/containers/podman/issues/22146 Link: https://bugs.passt.top/show_bug.cgi?id=84 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
tcp: Never automatically add the ACK flag to RST packets
2024-12-22 05:35:23 +00:00 · 2024-03-26 09:52:04 +01:00 · 2024-03-26 09:51:58 +01:00 · 2024-03-26 09:51:55 +01:00 · 2024-03-26 09:51:41 +01:00
1 changed files with 4 additions and 9 deletions
--- a/tcp.c
+++ b/tcp.c
@ -1593,8 +1593,6 @@ static void tcp_update_seqack_from_tap(const struct ctx *c,
 */
 static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 {
-	uint32_t prev_ack_to_tap = conn->seq_ack_to_tap;
-	uint32_t prev_wnd_to_tap = conn->wnd_to_tap;
 	struct tcp4_l2_flags_buf_t *b4 = NULL;
 	struct tcp6_l2_flags_buf_t *b6 = NULL;
 	struct tcp_info tinfo = { 0 };
@ -1674,16 +1672,13 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
 		*data++ = OPT_WS;
 		*data++ = OPT_WS_LEN;
 		*data++ = conn->ws_to_tap;
-
-		th->ack = !!(flags & ACK);
-	} else {
-		th->ack = !!(flags & (ACK | DUP_ACK)) ||
-			  conn->seq_ack_to_tap != prev_ack_to_tap ||
-			  !prev_wnd_to_tap;
+	} else if (!(flags & RST)) {
+		flags |= ACK;
 	}

 	th->doff = (sizeof(*th) + optlen) / 4;

+	th->ack = !!(flags & ACK);
 	th->rst = !!(flags & RST);
 	th->syn = !!(flags & SYN);
 	th->fin = !!(flags & FIN);
@ -2503,7 +2498,7 @@ out:
 		 */
 		if (conn->seq_dup_ack_approx != (conn->seq_from_tap & 0xff)) {
 			conn->seq_dup_ack_approx = conn->seq_from_tap & 0xff;
-			tcp_send_flag(c, conn, DUP_ACK);
+			tcp_send_flag(c, conn, ACK | DUP_ACK);
 		}
 		return p->count - idx;
 	}
Author	SHA1	Message	Date
David Gibson	4988e2b406	tcp: Unconditionally force ACK for all !SYN, !RST packets Currently we set ACK on flags packets only when the acknowledged byte pointer has advanced, or we hadn't previously set a window. This means in particular that we can send a window update with no ACK flag, which doesn't appear to be correct. RFC 9293 requires a receiver to ignore such a packet [0], and indeed it appears that every non-SYN, non-RST packet should have the ACK flag. The reason for the existing logic, rather than always forcing an ACK seems to be to avoid having the packet mistaken as a duplicate ACK which might trigger a fast retransmit. However, earlier tests in the function mean we won't reach here if we don't have either an advance in the ack pointer - which will already set the ACK flag, or a window update - which shouldn't trigger a fast retransmit. [0] https://www.ietf.org/rfc/rfc9293.html#section-3.10.7.4-2.5.2.1 Link: https://github.com/containers/podman/issues/22146 Link: https://bugs.passt.top/show_bug.cgi?id=84 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>	2024-03-26 09:52:04 +01:00
David Gibson	5894a245b9	tcp: Never automatically add the ACK flag to RST packets tcp_send_flag() will sometimes force on the ACK flag for all !SYN packets. This doesn't make sense for RST packets, where plain RST and RST+ACK have somewhat different meanings. AIUI, RST+ACK indicates an abrupt end to a connection, but acknowledges data already sent. Plain RST indicates an abort, when one end receives a packet that doesn't seem to make sense in the context of what it knows about the connection. All of the cases where we send RSTs are the second, so we don't want an ACK flag, but we currently could add one anyway. Change that, so we won't add an ACK to an RST unless the caller explicitly requests it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>	2024-03-26 09:51:58 +01:00
David Gibson	16c2d8da0d	tcp: Rearrange logic for setting ACK flag in tcp_send_flag() We have different paths for controlling the ACK flag for the SYN and !SYN paths. This amounts to sometimes forcing on the ACK flag in the !SYN path regardless of options. We can rearrange things to explicitly be that which will make things neater for some future changes. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>	2024-03-26 09:51:55 +01:00
David Gibson	99355e25b9	tcp: Split handling of DUP_ACK from ACK The DUP_ACK flag to tcp_send_flag() has two effects: first it forces the setting of the ACK flag in the packet, even if we otherwise wouldn't. Secondly, it causes a duplicate of the flags packet to be sent immediately after the first. Setting the ACK flag to tcp_send_flag() also has the first effect, so instead of having DUP_ACK also do that, pass both flags when we need both operations. This slightly simplifies the logic of tcp_send_flag() in a way that makes some future changes easier. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>	2024-03-26 09:51:41 +01:00