You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/11/02 18:42:10 UTC
[GitHub] [incubator-nuttx] acassis opened a new pull request, #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
acassis opened a new pull request, #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510
## Summary
Avoid starting TCP sequence number and ack number from 0
Fixes the issue https://github.com/apache/incubator-nuttx/issues/7508
## Impact
Now it is possible to connect to servers that don't allow TCP seqno starting with value 0.
## Testing
esp32-ethernet-kit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020760159
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +332,20 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* If this is the very first connection, seqno and sndseq will be 0!
+ * Rational:
+ * We need to make tcp->seqno random because some servers will not accept
+ * it starting from zero. The way to do it is making the sndseq random
+ * because it will be "memcpyied" to tcp->seqno.
+ */
+
+ seqno = tcp_getsequence(tcp->seqno);
Review Comment:
This issue was difficult to track because when TCP_WRITE_BUFFER was enabled everything was working fine, just the random initialization was enough. The issue was happening because when TCP_WRITE_BUFFER is disabled the conn->rexmit_seq is copied to conn->sndseq in the tcp_timer.c file. Setting the rexmit with right value in the connect solved the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012244675
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
+ if (ret < 0)
+ {
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, GRND_RANDOM);
+ }
+
+ /* If getrandom() failed use sys ticks, use barely half of allowed value */
+
+ if (ret != sizeof(uint32_t))
Review Comment:
You request 3 bytes of random data, but compare return value with 4. Isn't this condition always evaluated to true?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#issuecomment-1301509740
@pkarashchenko I was wrong, after more tests I noticed that initializing conn->sndno at tcp_connect() was enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012238695
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
Review Comment:
Because I'm ++g_tcpsequence to avoid g_tcpsequence == 0. Wait good point, I don't need to do it, because the % 200000 already avoid using the max value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020528328
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +144,34 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t), 0);
Review Comment:
@xiaoxiang781216 do you propose not to call `getrandom` if `g_tcpsequence` is non-zero? If yes, then all the next values will be
```
g_tcpsequence = g_tcpsequence % 2000000000;
...
if (g_tcpsequence < 1000000000)
{
g_tcpsequence += 1000000000;
}
```
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +332,20 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* If this is the very first connection, seqno and sndseq will be 0!
+ * Rational:
+ * We need to make tcp->seqno random because some servers will not accept
+ * it starting from zero. The way to do it is making the sndseq random
+ * because it will be "memcpyied" to tcp->seqno.
+ */
+
+ seqno = tcp_getsequence(tcp->seqno);
Review Comment:
Yeah. This point is also not clear to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020808934
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +144,42 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* If g_tcpsequence is already initialized, just copy it */
+
+ if (g_tcpsequence != 0)
+ {
+ goto copy_seqno;
Review Comment:
Yes, good idea! Fixed!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012252306
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
+ if (ret < 0)
+ {
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, GRND_RANDOM);
+ }
+
+ /* If getrandom() failed use sys ticks, use barely half of allowed value */
+
+ if (ret != sizeof(uint32_t))
Review Comment:
Yes, it was wrong, already fixed, please check again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020568970
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +144,34 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t), 0);
Review Comment:
@xiaoxiang781216 initially my idea was to do exactly that because g_tcpsequence is a global variable and I thought it could be changed and mess with some ongoing transmission. ~~After analyzing I noticed it is not used in any other place. So, it even doesn't need to be a global variable. What you think? Should I move it to inside the initsequence function?~~ Actually it is used by tcp_nextsequence(), but for some ready it is not read back and tcp_initsequence() is the only place where it is read back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020696302
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +332,20 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* If this is the very first connection, seqno and sndseq will be 0!
+ * Rational:
+ * We need to make tcp->seqno random because some servers will not accept
+ * it starting from zero. The way to do it is making the sndseq random
+ * because it will be "memcpyied" to tcp->seqno.
+ */
+
+ seqno = tcp_getsequence(tcp->seqno);
Review Comment:
But it's better to call tcp_initsequence in the right place(connect/accept) instead hacking tcp_sendcommon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#issuecomment-1312010705
@pkarashchenko @xiaoxiang781216 fixed! Not it only makes seqno randominc at first connection
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020808918
##########
net/tcp/tcp_conn.c:
##########
@@ -1313,6 +1317,12 @@ int tcp_connect(FAR struct tcp_conn_s *conn, FAR const struct sockaddr *addr)
conn->tcpstateflags = TCP_SYN_SENT;
tcp_initsequence(conn->sndseq);
+ /* Save initial sndseq to rexmit_seq, otherwise it will zero */
Review Comment:
Fixed! Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020794345
##########
net/tcp/tcp_conn.c:
##########
@@ -1313,6 +1317,12 @@ int tcp_connect(FAR struct tcp_conn_s *conn, FAR const struct sockaddr *addr)
conn->tcpstateflags = TCP_SYN_SENT;
tcp_initsequence(conn->sndseq);
+ /* Save initial sndseq to rexmit_seq, otherwise it will zero */
Review Comment:
```suggestion
/* Save initial sndseq to rexmit_seq, otherwise it will be zero */
```
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +144,42 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* If g_tcpsequence is already initialized, just copy it */
+
+ if (g_tcpsequence != 0)
+ {
+ goto copy_seqno;
Review Comment:
nitpick: Maybe we can try avoid using `goto` and just
```
if (g_tcpsequence == 0)
{
// place all random logic here
}
tcp_setsequence(seqno, g_tcpsequence);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020568970
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +144,34 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t), 0);
Review Comment:
@xiaoxiang781216 initially my idea was to do exactly that because g_tcpsequence is a global variable and I thought it could be changed and mess with some ongoing transmission. ~~After analyzing I noticed it is not used in any other place. So, it even doesn't need to be a global variable. What you think? Should I move it to inside the initsequence function?~~ Actually it is used by tcp_nextsequence(), but for some reason it is not read back and tcp_initsequence() is the only place where it is read back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#issuecomment-1312482153
@xiaoxiang781216 @pkarashchenko please review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#issuecomment-1301548342
This is also related to the issue reported on #2839.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020789385
##########
net/tcp/tcp_conn.c:
##########
@@ -1039,6 +1039,10 @@ FAR struct tcp_conn_s *tcp_alloc_accept(FAR struct net_driver_s *dev,
conn->tcpstateflags = TCP_SYN_RCVD;
tcp_initsequence(conn->sndseq);
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
Review Comment:
Why to CONFIG_NET_SENDFILE? I'm testing here these scenarios:
1) no TCP_WRITE_BUFFERS
2) TCP_WRITE_BUFFERS but NO SENDFILE
3) TCP_WRITE_BUFFERS and SENDFILE
In all these cases it works fine without add defined(CONFIG_NET_SENDFILE)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012242222
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
Review Comment:
I need to spend more time reviewing this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012341146
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +331,32 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* The initial conn->rcvseq and conn->sndseq values are defined to 0 and
+ * they are copied to tcp->ackno and tcp->seqno respectively. According
+ * to RCF1948 the initial TCP sequence number should be a random value and
+ * some servers will not accept TCP seqno starting from value 0.
+ *
+ * This fix solves this issue, however after a seqno/ackno overflow this
+ * value will be 0 again and in this case this packet will be discarded.
+ * This is not big issue because it only will happen after transmitting
+ * more than 2 billions packages (see tcp_initsequence).
+ *
+ * FIXME: This check should be done only at start of connection, but I
+ * tried to put it at tcp_connect() but it didn't work.
+ */
+
+ tmpseq = tcp_getsequence(conn->rcvseq);
+ if (tmpseq == 0)
+ {
+ tcp_initsequence(conn->rcvseq);
+ }
Review Comment:
Why this is applied to `rcvseq` as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012251973
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
Review Comment:
It was a mistake, I was thinking it as UINT_MAX not as sizeof(uint32_t). Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020561183
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +332,20 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* If this is the very first connection, seqno and sndseq will be 0!
+ * Rational:
+ * We need to make tcp->seqno random because some servers will not accept
+ * it starting from zero. The way to do it is making the sndseq random
+ * because it will be "memcpyied" to tcp->seqno.
+ */
+
+ seqno = tcp_getsequence(tcp->seqno);
Review Comment:
Unfortunately just updating tcp_initsequence and calling it on tcp_connect() isn't enough because it is zeroed on other places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020789385
##########
net/tcp/tcp_conn.c:
##########
@@ -1039,6 +1039,10 @@ FAR struct tcp_conn_s *tcp_alloc_accept(FAR struct net_driver_s *dev,
conn->tcpstateflags = TCP_SYN_RCVD;
tcp_initsequence(conn->sndseq);
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
Review Comment:
Why to check CONFIG_NET_SENDFILE? I'm testing here these scenarios:
1) no TCP_WRITE_BUFFERS
2) TCP_WRITE_BUFFERS but NO SENDFILE
3) TCP_WRITE_BUFFERS and SENDFILE
In all these cases it works fine without add defined(CONFIG_NET_SENDFILE)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020782981
##########
net/tcp/tcp_conn.c:
##########
@@ -1313,6 +1317,12 @@ int tcp_connect(FAR struct tcp_conn_s *conn, FAR const struct sockaddr *addr)
conn->tcpstateflags = TCP_SYN_SENT;
tcp_initsequence(conn->sndseq);
+ /* Save initial sndseq to rexmit_seq, otherwise it will zero */
+
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
Review Comment:
```suggestion
#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS) || \
defined(CONFIG_NET_SENDFILE)
```
##########
net/tcp/tcp_conn.c:
##########
@@ -1039,6 +1039,10 @@ FAR struct tcp_conn_s *tcp_alloc_accept(FAR struct net_driver_s *dev,
conn->tcpstateflags = TCP_SYN_RCVD;
tcp_initsequence(conn->sndseq);
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
Review Comment:
```suggestion
#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS) || \
defined(CONFIG_NET_SENDFILE)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020760159
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +332,20 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* If this is the very first connection, seqno and sndseq will be 0!
+ * Rational:
+ * We need to make tcp->seqno random because some servers will not accept
+ * it starting from zero. The way to do it is making the sndseq random
+ * because it will be "memcpyied" to tcp->seqno.
+ */
+
+ seqno = tcp_getsequence(tcp->seqno);
Review Comment:
This issue was difficult to track because with TCP_WRITE_BUFFER was enabled everything was working fine, just the random initialization was enough. The issue was happening because when TCP_WRITE_BUFFER is disabled the conn->rexmit_seq is copied to conn->sndseq in the tcp_timer.c file. Setting the rexmit with write value in the connect solved the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020790895
##########
net/tcp/tcp_conn.c:
##########
@@ -1313,6 +1317,12 @@ int tcp_connect(FAR struct tcp_conn_s *conn, FAR const struct sockaddr *addr)
conn->tcpstateflags = TCP_SYN_SENT;
tcp_initsequence(conn->sndseq);
+ /* Save initial sndseq to rexmit_seq, otherwise it will zero */
+
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
Review Comment:
Ditto!
Actually the rexmit_seq is modified in sendfile, but when TCP_WRITE_BUFFERS is enabled its value is not copied to sndseq.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#issuecomment-1301108513
Note: I tried to put this test inside tcp_connect() function to avoid doing the test for each transmitted packet, but unfortunately it doesn't work because these variables are overwritten later on. So, the safer place to put this check is on tcp_sendcommon() just before copying it to tcp->ackno and tcp->seqno.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020568970
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +144,34 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t), 0);
Review Comment:
@xiaoxiang781216 initially my idea was to do exactly that because g_tcpsequence is a global variable and I thought it could be changed and mess with some ongoing transmission. After analyzing I noticed it is not used in any other place. So, it even doesn't need to be a global variable. What you think? Should I move it to inside the initsequence function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012229914
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
Review Comment:
Why -1 here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012353573
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +331,32 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* The initial conn->rcvseq and conn->sndseq values are defined to 0 and
+ * they are copied to tcp->ackno and tcp->seqno respectively. According
+ * to RCF1948 the initial TCP sequence number should be a random value and
+ * some servers will not accept TCP seqno starting from value 0.
+ *
+ * This fix solves this issue, however after a seqno/ackno overflow this
+ * value will be 0 again and in this case this packet will be discarded.
+ * This is not big issue because it only will happen after transmitting
+ * more than 2 billions packages (see tcp_initsequence).
+ *
+ * FIXME: This check should be done only at start of connection, but I
+ * tried to put it at tcp_connect() but it didn't work.
+ */
+
+ tmpseq = tcp_getsequence(conn->rcvseq);
+ if (tmpseq == 0)
+ {
+ tcp_initsequence(conn->rcvseq);
+ }
Review Comment:
to make ACK number random as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012241509
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
Review Comment:
But current behavior will be different for BE and LE archs. You request to fill 3 bytes of uint32_t value with random data, but it will be 3 bytes at the beginning or at the end depending if the system is big or little endian.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number and ack number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1012238695
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +143,31 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t) - 1, 0);
Review Comment:
Because I'm ++g_tcpsequence to avoid g_tcpsequence == 0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020486389
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +332,20 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* If this is the very first connection, seqno and sndseq will be 0!
+ * Rational:
+ * We need to make tcp->seqno random because some servers will not accept
+ * it starting from zero. The way to do it is making the sndseq random
+ * because it will be "memcpyied" to tcp->seqno.
+ */
+
+ seqno = tcp_getsequence(tcp->seqno);
Review Comment:
why need change here? we can't update only tcp_initsequence instead
##########
net/tcp/tcp_seqno.c:
##########
@@ -142,6 +144,34 @@ uint32_t tcp_addsequence(FAR uint8_t *seqno, uint16_t len)
void tcp_initsequence(FAR uint8_t *seqno)
{
+ int ret;
+
+ /* Get a random TCP sequence number */
+
+ ret = getrandom(&g_tcpsequence, sizeof(uint32_t), 0);
Review Comment:
let's check g_tcpsequence isn't zero before getrandom
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a diff in pull request #7510: net/tcp: Avoid starting TCP sequence number from 0
Posted by GitBox <gi...@apache.org>.
acassis commented on code in PR #7510:
URL: https://github.com/apache/incubator-nuttx/pull/7510#discussion_r1020760159
##########
net/tcp/tcp_send.c:
##########
@@ -329,6 +332,20 @@ static void tcp_sendcommon(FAR struct net_driver_s *dev,
}
#endif /* CONFIG_NET_IPv4 */
+ /* If this is the very first connection, seqno and sndseq will be 0!
+ * Rational:
+ * We need to make tcp->seqno random because some servers will not accept
+ * it starting from zero. The way to do it is making the sndseq random
+ * because it will be "memcpyied" to tcp->seqno.
+ */
+
+ seqno = tcp_getsequence(tcp->seqno);
Review Comment:
This issue was difficult to track because when TCP_WRITE_BUFFER was enabled everything was working fine, just the random initialization was enough. The issue was happening because when TCP_WRITE_BUFFER is disabled the conn->rexmit_seq is copied to conn->sndseq in the tcp_timer.c file. Setting the rexmit with write value in the connect solved the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org