* [PATCH] tcp: Fix subtle bug in fast re-transmit path
@ 2024-02-07 12:57 David Gibson
2024-02-11 23:16 ` Stefano Brivio
0 siblings, 1 reply; 2+ messages in thread
From: David Gibson @ 2024-02-07 12:57 UTC (permalink / raw)
To: Stefano Brivio, passt-dev; +Cc: jmaloy, David Gibson
When a duplicate ack from the tap side triggers a fast re-transmit, we set
both conn->seq_ack_from_tap and conn->seq_to_tap to the sequence number of
the duplicate ack. Setting seq_to_tap is correct: this is what triggers
the retransmit from this point onwards. Setting seq_ack_from_tap is
not correct, though.
In most cases setting seq_ack_from_tap will be redundant but harmless:
it will have already been updated to the same value by
tcp_update_seqack_from_tap() a few lines above. However that call can
be skipped if tcp_sock_consume() fails, which is rare but possible. In
that case this update will cause problems.
We use seq_ack_from_tap to track two logically distinct things: how much of
the stream has been acked by the guest, and how much of the stream from the
socket has been read and discarded (as opposed to MSG_PEEKed). We attempt
to keep those values the same, because we discard data exactly when it is
acked by the guest. However tcp_sock_consume() failing means we weren't
able to disard the acked data. To handle that case, we skip the usual
update of seq_ack_from_tap, effectively ignoring the ack assuming we'll get
one which supersedes it soon enough. Setting seq_ack_from_tap in the
fast retransmit path, however, means we now really will have the
read/discard point in the stream out of sync with seq_ack_from_tap.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
tcp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/tcp.c b/tcp.c
index 905d26f6..2ab443d5 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2365,7 +2365,6 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn,
flow_trace(conn,
"fast re-transmit, ACK: %u, previous sequence: %u",
max_ack_seq, conn->seq_to_tap);
- conn->seq_ack_from_tap = max_ack_seq;
conn->seq_to_tap = max_ack_seq;
tcp_data_from_sock(c, conn);
}
--
@@ -2365,7 +2365,6 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn,
flow_trace(conn,
"fast re-transmit, ACK: %u, previous sequence: %u",
max_ack_seq, conn->seq_to_tap);
- conn->seq_ack_from_tap = max_ack_seq;
conn->seq_to_tap = max_ack_seq;
tcp_data_from_sock(c, conn);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] tcp: Fix subtle bug in fast re-transmit path
2024-02-07 12:57 [PATCH] tcp: Fix subtle bug in fast re-transmit path David Gibson
@ 2024-02-11 23:16 ` Stefano Brivio
0 siblings, 0 replies; 2+ messages in thread
From: Stefano Brivio @ 2024-02-11 23:16 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev, jmaloy
On Wed, 7 Feb 2024 23:57:21 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> When a duplicate ack from the tap side triggers a fast re-transmit, we set
> both conn->seq_ack_from_tap and conn->seq_to_tap to the sequence number of
> the duplicate ack. Setting seq_to_tap is correct: this is what triggers
> the retransmit from this point onwards. Setting seq_ack_from_tap is
> not correct, though.
>
> In most cases setting seq_ack_from_tap will be redundant but harmless:
> it will have already been updated to the same value by
> tcp_update_seqack_from_tap() a few lines above. However that call can
> be skipped if tcp_sock_consume() fails, which is rare but possible. In
> that case this update will cause problems.
>
> We use seq_ack_from_tap to track two logically distinct things: how much of
> the stream has been acked by the guest, and how much of the stream from the
> socket has been read and discarded (as opposed to MSG_PEEKed). We attempt
> to keep those values the same, because we discard data exactly when it is
> acked by the guest. However tcp_sock_consume() failing means we weren't
> able to disard the acked data. To handle that case, we skip the usual
> update of seq_ack_from_tap, effectively ignoring the ack assuming we'll get
> one which supersedes it soon enough. Setting seq_ack_from_tap in the
> fast retransmit path, however, means we now really will have the
> read/discard point in the stream out of sync with seq_ack_from_tap.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Applied.
--
Stefano
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-02-11 23:17 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-07 12:57 [PATCH] tcp: Fix subtle bug in fast re-transmit path David Gibson
2024-02-11 23:16 ` Stefano Brivio
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).