mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket
@ 2023-02-07 16:20 Jules Maselbas
  2023-02-07 16:20 ` [PATCH v3 2/5] bbremote: Fix RATP handshake, errata #7321 for RFC916 Jules Maselbas
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Jules Maselbas @ 2023-02-07 16:20 UTC (permalink / raw)
  To: barebox; +Cc: Jules Maselbas

The payload variable is expected to typeof bytes, however the default
value in BBPacket contructor is of type str, a simple way to declare
a byte literal in Python is to prefix the string literal with `b`.

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
---
 scripts/remote/messages.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/remote/messages.py b/scripts/remote/messages.py
index 76cccad393..de15e72ed5 100644
--- a/scripts/remote/messages.py
+++ b/scripts/remote/messages.py
@@ -35,12 +35,13 @@ class BBType(object):
 
 
 class BBPacket(object):
-    def __init__(self, p_type=0, p_flags=0, payload="", raw=None):
+    def __init__(self, p_type=0, p_flags=0, payload=b"", raw=None):
         self.p_type = p_type
         self.p_flags = p_flags
         if raw is not None:
             self.unpack(raw)
         else:
+            assert isinstance(payload, bytes)
             self.payload = payload
 
     def __repr__(self):
-- 
2.17.1




^ permalink raw reply	[flat|nested] 6+ messages in thread

* [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

* [PATCH v3 4/5] ratp: Increase the establish timeout to 1sec
  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 ` [PATCH v3 3/5] ratp: Fix retransmission for header-only packets Jules Maselbas
@ 2023-02-07 16:20 ` Jules Maselbas
  2023-02-07 16:20 ` [PATCH v3 5/5] ratp: Increase the initial RTO to 200ms Jules Maselbas
  2023-02-13  9:11 ` [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket Sascha Hauer
  4 siblings, 0 replies; 6+ messages in thread
From: Jules Maselbas @ 2023-02-07 16:20 UTC (permalink / raw)
  To: barebox; +Cc: Jules Maselbas

Current timeout was only 100ms which is also the current initial value
for the RTO (Retransmission Time Out), this is too short as it doesn't
give barebox a chance to resent a packet during the handshake.

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
---
 common/ratp/ratp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
index 424c9406d2..fddb286e01 100644
--- a/common/ratp/ratp.c
+++ b/common/ratp/ratp.c
@@ -486,7 +486,7 @@ int barebox_ratp(struct console_device *cdev)
 	ctx->cdev = cdev;
 	ctx->have_synch = 1;
 
-	ret = ratp_establish(&ctx->ratp, false, 100);
+	ret = ratp_establish(&ctx->ratp, false, 1000);
 	if (ret < 0)
 		goto out;
 
-- 
2.17.1




^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 5/5] ratp: Increase the initial RTO to 200ms
  2023-02-07 16:20 [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket Jules Maselbas
                   ` (2 preceding siblings ...)
  2023-02-07 16:20 ` [PATCH v3 4/5] ratp: Increase the establish timeout to 1sec Jules Maselbas
@ 2023-02-07 16:20 ` Jules Maselbas
  2023-02-13  9:11 ` [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket Sascha Hauer
  4 siblings, 0 replies; 6+ messages in thread
From: Jules Maselbas @ 2023-02-07 16:20 UTC (permalink / raw)
  To: barebox; +Cc: Jules Maselbas

The initial value for RTO is 100ms which might be a bit low. From the
RFC916 the RTO is expected to have a lower and upper bound but values
are not specified.  The RFC916 also define the calculation of the RTO
to be somewhere between 1.3 to 2.0 times the SRTT (which is currently
defined to 100ms).  Thus I propose to set the initial value of RTO to
200ms, to be 2.0 times greater than the initial SRTT.

Moreover, the current runtime calculation for RTO is done in the
function ratp_msg_done and has lower bound of 200ms:
    ri->srtt = (alpha * ri->srtt + (10 - alpha) * rtt) / 10;
    ri->rto = max(200, beta * ri->srtt / 10);

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
---
 lib/ratp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ratp.c b/lib/ratp.c
index d5205a4e93..c597e96784 100644
--- a/lib/ratp.c
+++ b/lib/ratp.c
@@ -1648,7 +1648,7 @@ int ratp_establish(struct ratp *ratp, bool active, int timeout_ms)
 	INIT_LIST_HEAD(&ri->sendmsg);
 	ri->max_retransmission = 100;
 	ri->srtt = 100;
-	ri->rto = 100;
+	ri->rto = 200;
 	ri->active = active;
 
 	ri->in_ratp++;
-- 
2.17.1




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket
  2023-02-07 16:20 [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket Jules Maselbas
                   ` (3 preceding siblings ...)
  2023-02-07 16:20 ` [PATCH v3 5/5] ratp: Increase the initial RTO to 200ms Jules Maselbas
@ 2023-02-13  9:11 ` Sascha Hauer
  4 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2023-02-13  9:11 UTC (permalink / raw)
  To: Jules Maselbas; +Cc: barebox

On Tue, Feb 07, 2023 at 05:20:51PM +0100, Jules Maselbas wrote:
> The payload variable is expected to typeof bytes, however the default
> value in BBPacket contructor is of type str, a simple way to declare
> a byte literal in Python is to prefix the string literal with `b`.
> 
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>  scripts/remote/messages.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/remote/messages.py b/scripts/remote/messages.py
> index 76cccad393..de15e72ed5 100644
> --- a/scripts/remote/messages.py
> +++ b/scripts/remote/messages.py
> @@ -35,12 +35,13 @@ class BBType(object):

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-13  9:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH v3 3/5] ratp: Fix retransmission for header-only packets Jules Maselbas
2023-02-07 16:20 ` [PATCH v3 4/5] ratp: Increase the establish timeout to 1sec Jules Maselbas
2023-02-07 16:20 ` [PATCH v3 5/5] ratp: Increase the initial RTO to 200ms Jules Maselbas
2023-02-13  9:11 ` [PATCH v3 1/5] bbremote: Fix default payload value in BBPacket Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox