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/01/02 16:02:26 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #5138: net/tcp(unbuffered): advance sndseq by +1 because SYN and FIN occupy one sequence number
a-lunev opened a new pull request #5138:
URL: https://github.com/apache/incubator-nuttx/pull/5138
## Summary
Advance sndseq by +1 because SYN and FIN occupy one sequence number (RFC 793).
This PR refers to my previous PR #5102.
## Impact
TCP
## Testing
Install and configure vsftpd on Linux host.
Build NuttX:
```
$ ./tools/configure.sh -l sim:tcpblaster
$ make menuconfig (disable CONFIG_NET_TCP_WRITE_BUFFERS)
$ make
```
Enable TUN/TAP on Linux host:
```
$ sudo setcap cap_net_admin+ep ./nuttx
$ sudo ./tools/simhostroute.sh wlan0 on
```
Run NuttX on Linux host:
```
$ ./nuttx
NuttShell (NSH) NuttX-10.2.0
nsh> ifconfig eth0 10.0.1.2
nsh> ifup eth0
ifup eth0...OK
```
Start Wireshark (or tcpdump) and capture appeared tap0 interface.
Run in NuttX:
```
nsh> dd if=/dev/zero of=/tmp/test.bin count=1000
nsh> ftpc -4 -n LINUX_HOST_IP_ADDRESS
NuttX FTP Client:
login anonymous
put /tmp/test.bin pub/test.bin
quit
```
Observe TCP dump.
Shutdown NuttX:
`nsh> poweroff`
Disable TUN/TAP on Linux host:
`$ sudo ./tools/simhostroute.sh wlan0 off`
--
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] masayuki2009 merged pull request #5138: net/tcp(unbuffered): advance sndseq by +1 because SYN and FIN occupy one sequence number
Posted by GitBox <gi...@apache.org>.
masayuki2009 merged pull request #5138:
URL: https://github.com/apache/incubator-nuttx/pull/5138
--
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] a-lunev commented on a change in pull request #5138: net/tcp(unbuffered): advance sndseq by +1 because SYN and FIN occupy one sequence number
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5138:
URL: https://github.com/apache/incubator-nuttx/pull/5138#discussion_r777877997
##########
File path: net/tcp/tcp_input.c
##########
@@ -870,6 +870,11 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
if ((tcp->flags & TCP_CTL) == TCP_SYN)
{
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
+ tcp_setsequence(conn->sndseq, conn->rexmit_seq);
+#else
+ /* REVISIT for the buffered mode */
Review comment:
PRs #5102 and #5138 changed only the unbuffered mode. The buffered mode was not modified (isolated by conditional compilation in those patches).
Considering the unbuffered mode, previously sndseq was updated / overwritten in multiple places. In PR #5102 I removed excessive sndseq overwrites, including sndseq overwrite in tcp_input.c. However, that exposed an issue (sndseq did not advance by +1 on sending SYN packet). In PR #5138 I added advancing sndseq by +1 on sending SYN and FIN packets as it should be according to RFC 793 (previously the issue was hidden / masked because sndseq was overwritten by acknowledge numbers from received packets of the remote party. As I can see, that algorithm came from uIP).
I added "REVISIT for the buffered mode" comments because the same correction may be reasonable for the buffered mode. I suppose there is the same situation as for the unbuffered mode (I have not yet analyzed the buffered mode code thoroughly).
However, in the buffered mode I see other issues. When I tested the buffered mode some time ago I noticed that TCP window size advertised by NuttX stack jumped up and down very strange.
One more issue with the buffered mode I found recently based on my netcat testing with enabled NETUTILS_NETCAT_SENDFILE option (I implemented the option here: https://github.com/apache/incubator-nuttx-apps/pull/952). After sending a file via netcat (over tcp_sendfile) the TCP connection is not finalized properly. Preliminary I think it may be similar issue with incorrect unackseq calculation that was with the unbuffered mode (#4656). I will investigate the issue.
Anyway, I am going to prepare several new patches concerning tcp_sendfile similar to what I did for tcp_send_unbuffered. As I can see, tcp_sendfile implementation is very similar to tcp_send_unbuffered.
--
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 change in pull request #5138: net/tcp(unbuffered): advance sndseq by +1 because SYN and FIN occupy one sequence number
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5138:
URL: https://github.com/apache/incubator-nuttx/pull/5138#discussion_r778182272
##########
File path: net/tcp/tcp_input.c
##########
@@ -870,6 +870,11 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
if ((tcp->flags & TCP_CTL) == TCP_SYN)
{
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
+ tcp_setsequence(conn->sndseq, conn->rexmit_seq);
+#else
+ /* REVISIT for the buffered mode */
Review comment:
Sure, thanks for the detail explanation, let's wait your new finding.
--
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] masayuki2009 commented on pull request #5138: net/tcp(unbuffered): advance sndseq by +1 because SYN and FIN occupy one sequence number
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5138:
URL: https://github.com/apache/incubator-nuttx/pull/5138#issuecomment-1003848173
@a-lunev
I confirmed spresense:rndis (both CONFIG_NET_TCP_WRITE_BUFFERS n and y) work.
Thanks for the quick fix.
Let me merge this PR.
--
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 change in pull request #5138: net/tcp(unbuffered): advance sndseq by +1 because SYN and FIN occupy one sequence number
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5138:
URL: https://github.com/apache/incubator-nuttx/pull/5138#discussion_r777287890
##########
File path: net/tcp/tcp_input.c
##########
@@ -870,6 +870,11 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
if ((tcp->flags & TCP_CTL) == TCP_SYN)
{
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
+ tcp_setsequence(conn->sndseq, conn->rexmit_seq);
+#else
+ /* REVISIT for the buffered mode */
Review comment:
@a-lunev could you verify whether the buffer mode also broken now?
--
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] a-lunev commented on a change in pull request #5138: net/tcp(unbuffered): advance sndseq by +1 because SYN and FIN occupy one sequence number
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5138:
URL: https://github.com/apache/incubator-nuttx/pull/5138#discussion_r785393232
##########
File path: net/tcp/tcp_input.c
##########
@@ -870,6 +870,11 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
if ((tcp->flags & TCP_CTL) == TCP_SYN)
{
+#if !defined(CONFIG_NET_TCP_WRITE_BUFFERS)
+ tcp_setsequence(conn->sndseq, conn->rexmit_seq);
+#else
+ /* REVISIT for the buffered mode */
Review comment:
> One more issue with the buffered mode I found recently based on my netcat testing with enabled NETUTILS_NETCAT_SENDFILE option (I implemented the option here: [apache/incubator-nuttx-apps#952](https://github.com/apache/incubator-nuttx-apps/pull/952)). After sending a file via netcat (over tcp_sendfile) the TCP connection is not finalized properly. Preliminary I think it may be similar issue with incorrect unackseq calculation that was with the unbuffered mode (#4656). I will investigate the issue.
@xiaoxiang781216 As it turned out, NET_TCP_WRITE_BUFFERS and NET_SENDFILE options were totally inconsistent with each other. One of the visible issue was that after sending a file via netcat (over tcp_sendfile) the TCP connection was not finalized. It looks like the combination of both enabled NET_TCP_WRITE_BUFFERS and NET_SENDFILE options have not yet been supported in NuttX. PR #5239 supports 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