sockets: refactor solisten() and pr_listen

[glebius]

Summary

For simple protocols the new KPI is as simple as it was in FreeBSD 4. The
protocol method just does protocol specific stuff and doesn't need to call
back into the socket layer. However, there is an option to acquire the
socket lock in the pr_listen and return with it locked, as well as set
SO_ACCEPTCONN early, using protocol locks for synchronization.

The historical design here is that socket layer calls into protocol and
protocol calls back into socket layer. This originates from the early
SMPng, see 0daccb9c94393. Main motivation for this added complexity was
of course TCP, where the socket state and the pcb state need to change
atomically. Once tcp_input() finds a tcpcb that is in TCPS_LISTEN, it
must point at SO_ACCEPTCONN socket. With new KPI this is achieved by
interlocking socket lock and pcb lock and using both to protect raising of
SO_ACCEPTCONN.

One other long standing bug was that consecutive listen(2) syscalls, that
just change the backlog would also enter pr_listen. This was mostly
harmless bug until 7cbb6b6e28db. Now handling of reconfiguring listen(2)s
is fully handled by solisten() and this bug no longer exists.

While here, merge tcp_usr_listen() and tcp6_usr_listen() into one. They
have more similarities than differencies.

[markj]

I like the result of the change, but am worried that it cannot work without deeper refactoring - see some of the inline comments.

sys/kern/uipc_socket.c

1435

1437

sys/netinet/tcp_usrreq.c

361

Suppose tcp_usr_listen() races with tcp_usr_connect(), and loses, so tcp_usr_connect() acquires the inpcb lock first. Then the socket is connected, and we proceed here - how do we detect that the socket is connected? The SS_ISCONNECTED check in solisten() needs to be synchronized by the inpcb lock, I believe.

sys/sys/socketvar.h

253

Why do you not check SO_INLISTEN in SOLISTENING() instead?

I think some existing uses of SOLISTENING() are racy after this change. Consider the checks in so_splice() - if the checks are done after SO_INLISTEN is set, we will continue with the operation. In that case it's ok, because the so_state check will fail, but that's not obvious.

Here is another example that is more serious: soo_stat() checks SOLISTENING(), and it will be false during a concurrent call to solisten(), so we could access the socket buffers as they're being destroyed.

[glebius]

sys/netinet/tcp_usrreq.c

361

You are right :( What if we do this: enter the pr_listen with SOCK_LOCK & sockbuf locks held and protocol is supposed to do try-lock operation on its internal locking? If try-lock fails, return EAGAIN. This just can never happen to a normal server application. Pretty much the same what you do now with socket I/O locks in solisten_proto_check().

sys/sys/socketvar.h

253

SOLISTENING() is positive predicate, that reports that socket can already be treated as a listening one, with all listening fields initialized. The SOMAYLISTEN() is a negative one, it expresses that socket buffer fields can no longer be used.

But this point will be moot, if we go with the above suggestion of not dropping any locks and use try-lock inside pr_listen.

[markj]

sys/netinet/tcp_usrreq.c

361

I think that could work, yes. At least for TCP. It feels a bit fragile: what if pr_listen needs to try-acquire a global lock that is sometimes contended? Then you could have spurious listen() failures.