See my comments below. On Sunday, April 29, 2018 at 6:41:01 PM UTC-4, Nadav Har'El wrote: > > > On Thu, Apr 26, 2018 at 8:43 AM, Waldemar Kozaczuk <jwkoz...@gmail.com > <javascript:>> wrote: > >> This patch fixes an intermittent issue happening in TCP stack code >> during session 3-way handshake protocol due to inconsistent state >> of transmission control block (TCB) data structure. Specifically >> right after server sends SYN+ACK to client the function syncache_socket() >> would prematurely invoke soisconnected() to move child socket from >> so_incomp >> queue to so_comp queue and release ACCEPT lock. This results in premature >> accepting of new connection that leads to sending data using inconsistent >> variables in TCB structure (snd_nxt, snd_una). Finally it is manifested >> by client missing to receive 1st octet when server thinks it has sent all >> data (see failing tst-tcp-nbwrite test). >> >> This bug was inadvertently introduced when backporting a patch from >> FreeBSD >> - >> https://github.com/cloudius-systems/osv/commit/330d85c2e858fd0f32f7377ce40ea21cd8a87041#diff-689dfa92b697b6e1327d6c0bad5fad26. >> >> Two years later part of the patch was reversed >> in FreeBSD codebase - >> https://github.com/freebsd/freebsd/commit/0b9cdc127b85d70aaf5f72c14f72e6f7bc43647f#diff-f3fd9f911dd76ffd9fbd0c8bfb458a61. >> >> For more details please see >> - https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=183659 and >> https://reviews.freebsd.org/D8072. >> >> So in essence this patch removes invocation of soisconnected() along >> with SOCK_LOCK from syncache_socket() and adds SOCK_LOCK right >> before soisconnected() invocation in tcp_do_segment(). >> >> Unfortunately it is not clear why soisconnected() was added to >> syncache_socket() >> in first place in original freebsd patch backported in 2014 to OSv >> neither why >> it was no longer necessary per patch in 2016. The former patch also >> removed >> locking logic around V_tcbinfo in tcp_usr_accept() to alleviate lock >> contention >> but it is not clear what the consequences of removing soisconnected() from >> syncache_socket() might be. However based on over 1000 (1K) successful >> runs >> of tst-tcp-nbwrite test and many executions of concurrent tests of >> rust-httpserver >> using apache benchmark tool with 100 concurrent clients, TCP stack seems >> to be >> functioning fine and does seem to perform in similar way >> (around 10-15K request per second with 4 cpus setup). >> > >> Fixes #859 >> > > That's really great, thanks! I'll be happy for this bug to go away :-) > > I can't say I fully understand the details here, or whether this is the > right fix, so I have a couple of comments, I don't know how stupid they are: > > If I look at tcp_input.cc, it calls syncache_expand() (which calls > syncache_socket()) which ends with state TCPS_SYN_RECEIVED, > so certainly I agree it was wrong to call soisconnected() from it. The > question is what calls soisconnected() later. I'm guessing that > when tcp_input.cc calls tcp_do_segment() after syncache_expand(), that > call will switch us to TCPS_ESTABLISHED, and also call > soisconnect(). > > The change to add SOCK_LOCK(so) inside tcp_do_segment in that specific > place is a bit stranger for me. Must it be there? Could it be right after > the call to syncache_expand() perhaps? So the behavior will be similar to > what we had before (previously, when syncache_expand() returned it means > sycache_socket() returned so the socket would have been locked). > Maybe you're right, though, I don't know - did you base the new position > of SOCK_LOCK(so) on some of the BSD patches? > > > Here is my little 'naive' explanation (naive because I also do not fully appreciate impact of this change nor all the logic and code). First I tried to simply remove soisconnected() and SOCK_LOCK(so) (2 lines) from syncache_socket() but then I would be consistently getting asserts errors complaining that some resource (I think socket object) was not locked.
Then I read comments made by Pekka or Asias in the original patch from 2014 - https://github.com/cloudius-systems/osv/commit/330d85c2e858fd0f32f7377ce40ea21cd8a87041#diff-689dfa92b697b6e1327d6c0bad5fad26: "A modification is made compared to original patch. In OSv, we call SOCK_LOCK() before soisconnected(). This is becasue in 79005e3 <https://github.com/cloudius-systems/osv/commit/79005e321f2f578e0b254d4ecd1c15e8485ac4b5> (net: remove socket lock, use protocol layer connection lock instead) an extra SOCK_UNLOCK() was added to soisconnected()." This made me think that I simply need to add SOCK_LOCK before soisconnected. Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com <javascript:>> >> --- >> bsd/sys/netinet/tcp_input.cc | 1 + >> bsd/sys/netinet/tcp_syncache.cc | 3 --- >> 2 files changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/bsd/sys/netinet/tcp_input.cc b/bsd/sys/netinet/tcp_input.cc >> index 3726ec4..c710082 100644 >> --- a/bsd/sys/netinet/tcp_input.cc >> +++ b/bsd/sys/netinet/tcp_input.cc >> @@ -1914,6 +1914,7 @@ tcp_do_segment(struct mbuf *m, struct tcphdr *th, >> struct socket *so, >> case TCPS_SYN_RECEIVED: >> >> TCPSTAT_INC(tcps_connects); >> + SOCK_LOCK(so); >> soisconnected(so); >> /* Do window scaling? */ >> if ((tp->t_flags & (TF_RCVD_SCALE|TF_REQ_SCALE)) == >> diff --git a/bsd/sys/netinet/tcp_syncache.cc >> b/bsd/sys/netinet/tcp_syncache.cc >> index c9e654c..b1fb17c 100644 >> --- a/bsd/sys/netinet/tcp_syncache.cc >> +++ b/bsd/sys/netinet/tcp_syncache.cc >> @@ -810,9 +810,6 @@ syncache_socket(struct syncache *sc, struct socket >> *lso, struct mbuf *m) >> >> INP_UNLOCK(inp); >> >> - SOCK_LOCK(so); >> - soisconnected(so); >> - >> TCPSTAT_INC(tcps_accepts); >> return (so); >> >> -- >> 2.7.4 >> >> -- >> You received this message because you are subscribed to the Google Groups >> "OSv Development" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to osv-dev+u...@googlegroups.com <javascript:>. >> For more options, visit https://groups.google.com/d/optout. >> > > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
