* [PATCH v3 2/5] bbremote: Fix RATP handshake, errata #7321 for RFC916
2023-02-07 16:20 [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket Jules Maselbas
@ 2023-02-07 16:20 ` Jules Maselbas
2023-02-07 16:20 ` [PATCH v3 3/5] ratp: Fix retransmission for header-only packets Jules Maselbas
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jules Maselbas @ 2023-02-07 16:20 UTC (permalink / raw)
To: barebox; +Cc: Jules Maselbas
Side A Side B
1. CLOSED LISTEN
2. [OPEN request]
SYN_SENT --> <SN=0><CTL=SYN> --> SYN_RECEIVED
3. ESTABLISHED <-- <SN=0><AN=1><CTL=SYN,ACK> <--
4. --> <SN=1><AN=0><CTL=ACK> ...
5. ... <SN=0><AN=1><CTL=SYN,ACK> <-- (retransmit)
6. (packet sent by A at 4. finally arrives to B)
... --> ESTABLISHED
7. (packet resent by B at 5. finally arrives to A)
CLOSED (C2) <-- ...
8. --> <SN=1><AN=1><CTL=RST> --> (connection reset)
The Figure above illustrate the current issue RATP can face during the
three-way handshake, and how behavior C2 can cause a connection to be
closed immediately after being established.
In the Figure above, side A try to establish a connection with side B,
which is in the LISTEN state. Commented line:
1. side A is in the CLOSED state and side B is in the LISTEN state;
2. side A open a new connection and send a SYN packet and is received by
side B which enters the SYN_RECEIVED state and send back a SYN-ACK;
3. side A receive the SYN-ACK packet from B;
4. side A respond with an ACK packet and move to the ESTABLISHED state.
Meanwhile;
5. side B hasn't received yet the ACK from side A and decide to
retransmit the SYN-ACK packet;
6. side B finally receive the ACK from side A and move to the
ESTABLISHED state;
7. side A finally receive the duplicated SYN-ACK packet from side B,
execute behavior C2: the received packet doesn't have the expected
SN and has the SYN flag set, thus respond by sending a legal reset.
8. side B receive the reset and close the connection.
One solution could be to tweak the initial RTO value of side B in order
to prevent sending a duplicated SYN-ACK packet, however the initial RTT
value is likely inaccurate during the handshake. This solution seems a
bit brittle.
The second solution would be to consider that a host has crashed only if
the packet received has the SYN flag set but not the ACK flag. The
rational is that the first step during handshake is to send a packet
only containing the SYN flag, however a packet containing both ACK and
SYN flags must have come after the initial handshake exchange and can
be considered as a duplicated and be dropped.
I proposed the following errata to RFC916 [1]:
[Page 29]
- If SYN was set we assume that the other end crashed and has
- attempted to open a new connection. We respond by sending a
- legal reset:
+ If the SYN flag was set but the ACK was not set then we assume
+ that the other end crashed and has attempted to open a new connection.
+ We respond by sending a legal reset:
[Page 30]
- If neither RST, FIN, nor SYN flags were set it is assumed that
- this packet is a duplicate of one already received. Send an
- ACK back:
+ If neither RST nor FIN flags were set, or if SYN and ACK flags
+ were set, it is assumed that this packet is a duplicate of one
+ already received. Send an ACK back:
[1] https://www.rfc-editor.org/errata/eid7321
Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
---
scripts/remote/ratp.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/remote/ratp.py b/scripts/remote/ratp.py
index 25ca442d15..956c788234 100644
--- a/scripts/remote/ratp.py
+++ b/scripts/remote/ratp.py
@@ -345,7 +345,7 @@ class RatpConnection(object):
if r.c_rst or r.c_fin:
return False
- if r.c_syn:
+ if r.c_syn and not r.c_ack:
s = RatpPacket(flags='RA')
s.c_sn = r.c_an
s.c_an = (r.c_sn + 1) % 2
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3 3/5] ratp: Fix retransmission for header-only packets
2023-02-07 16:20 [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket Jules Maselbas
2023-02-07 16:20 ` [PATCH v3 2/5] bbremote: Fix RATP handshake, errata #7321 for RFC916 Jules Maselbas
@ 2023-02-07 16:20 ` Jules Maselbas
2023-02-07 16:20 ` [PATCH v3 4/5] ratp: Increase the establish timeout to 1sec Jules Maselbas
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jules Maselbas @ 2023-02-07 16:20 UTC (permalink / raw)
To: barebox; +Cc: Jules Maselbas
Using sendmsg_current to detect if a packet needs to be retransmitted is
brittle as only packets containing data will ever be considered, packets
only containing a header (without data) were never being retransmitted.
The sendbuf_len is used to determine if a packets is being send or not,
a non-zero length means that a packets is still being in the "try-send"
state, the length is not set to zero when a valid SN is received.
Retransmission issue can be easily reproduced with a physical UART (not
cdc_acm over USB), by running the following command:
while ratp-barebox-cli -b 115200 -t /dev/ttyACMx -p; do :; done
Alternatively, it is possible to modify lib/ratp.c to force packets to
be sent by the retransmission logic with the following change:
@ int ratp_send_pkt(struct ratp_internal *ri, void *pkt, int length)
ri->sendbuf_len = length;
- ri->retransmission_timer_start = get_time_ns();
+ ri->retransmission_timer_start = get_time_ns() - ri->rto * MSECOND/2;
ri->retransmission_count = 0;
}
- return ri->ratp->send(ri->ratp, pkt, length);
+ return 0;
Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
---
lib/ratp.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/lib/ratp.c b/lib/ratp.c
index ce30223bac..d5205a4e93 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -738,7 +738,7 @@ static int ratp_behaviour_c2(struct ratp_internal *ri, void *pkt)
control = RATP_CONTROL_RST | RATP_CONTROL_ACK |
ratp_set_sn(ratp_an(hdr)) | ratp_set_next_an(ratp_sn(hdr));
ratp_send_hdr(ri, control);
-
+ ri->sendbuf_len = 0;
ratp_state_change(ri, RATP_STATE_CLOSED);
return 1;
}
@@ -784,7 +784,7 @@ static int ratp_behaviour_d1(struct ratp_internal *ri, void *pkt)
ri->status = -ECONNREFUSED;
pr_debug("Error: connection refused\n");
-
+ ri->sendbuf_len = 0;
ratp_state_change(ri, RATP_STATE_CLOSED);
return 1;
@@ -812,6 +812,8 @@ static int ratp_behaviour_d2(struct ratp_internal *ri, void *pkt)
ri->status = -ECONNRESET;
pr_debug("connection reset\n");
+ ri->sendbuf_len = 0;
+ ratp_state_change(ri, RATP_STATE_CLOSED);
return 0;
}
@@ -879,7 +881,7 @@ static int ratp_behaviour_e(struct ratp_internal *ri, void *pkt)
ratp_send_hdr(ri, control);
pr_debug("connection reset\n");
-
+ ri->sendbuf_len = 0;
ratp_state_change(ri, RATP_STATE_CLOSED);
return 1;
@@ -924,8 +926,10 @@ static int ratp_behaviour_f1(struct ratp_internal *ri, void *pkt)
if (!(hdr->control & RATP_CONTROL_ACK))
return 1;
- if (ratp_an_expected(ri, hdr))
+ if (ratp_an_expected(ri, hdr)) {
+ ri->sendbuf_len = 0; /* packet succesfully received */
return 0;
+ }
control = RATP_CONTROL_RST | ratp_set_sn(ratp_an(hdr));
ratp_send_hdr(ri, control);
@@ -971,6 +975,7 @@ static int ratp_behaviour_f2(struct ratp_internal *ri, void *pkt)
if (ri->sendmsg_current)
ratp_msg_done(ri, ri->sendmsg_current, 0);
ri->sendmsg_current = NULL;
+ ri->sendbuf_len = 0; /* packet succesfully received */
return 0;
} else {
pr_vdebug("%s: an not expected\n", __func__);
@@ -1175,6 +1180,7 @@ static int ratp_behaviour_h3(struct ratp_internal *ri, void *pkt)
ratp_send_hdr(ri, control);
ri->status = -ECONNRESET;
pr_debug("Error: Connection reset\n");
+ ri->sendbuf_len = 0;
ratp_state_change(ri, RATP_STATE_CLOSED);
return 1;
}
@@ -1217,8 +1223,10 @@ static int ratp_behaviour_h4(struct ratp_internal *ri, void *pkt)
pr_debug("%s\n", __func__);
- if (ratp_an_expected(ri, hdr))
+ if (ratp_an_expected(ri, hdr)) {
+ ri->sendbuf_len = 0; /* packet succesfully received */
ratp_state_change(ri, RATP_STATE_CLOSED);
+ }
return 1;
}
@@ -1244,6 +1252,7 @@ static int ratp_behaviour_h5(struct ratp_internal *ri, void *pkt)
pr_debug("%s\n", __func__);
if (ratp_an_expected(ri, hdr)) {
+ ri->sendbuf_len = 0; /* packet succesfully received */
ratp_state_change(ri, RATP_STATE_TIME_WAIT);
ratp_start_time_wait_timer(ri);
}
@@ -1580,9 +1589,8 @@ int ratp_poll(struct ratp *ratp)
}
}
- if (ri->sendmsg_current && is_timeout(ri->retransmission_timer_start,
+ if (ri->sendbuf_len && is_timeout(ri->retransmission_timer_start,
ri->rto * MSECOND)) {
-
ri->retransmission_count++;
if (ri->retransmission_count == ri->max_retransmission) {
ri->status = ret = -ETIMEDOUT;
@@ -1601,7 +1609,7 @@ int ratp_poll(struct ratp *ratp)
goto out;
}
- if (!ri->sendmsg_current && !list_empty(&ri->sendmsg))
+ if (ri->sendbuf_len == 0 && !list_empty(&ri->sendmsg))
ratp_send_next_data(ri);
ret = 0;
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread