socket: Move sockbuf locks into the owning socket

[markj]

Summary

There is a long-standing race with listen(2) in that it does not
properly interlock with concurrent I/O on the socket. Becuase listen(2)
destroys the socket buffers, such a race can trigger panics. In
practice this does not arise since applications have no reason to
trigger the race. However, it needs to be fixed.

Part of the problem is that there is no good way to reliably check
whether a socket is a listening socket, except by locking the socket and
checking SOLISTENING(so). However, the socket lock is generally not
used during I/O, and it is undesirable to incur the overhead of
acquiring it just to check for a condition that should never occur,
modulo application bugs or malicious behaviour.

listen(2) calls are relatively rare. Thus, the strategy is to have it
acquire all socket buffer locks when transitioning to a listening
socket. To do this safely, these locks must be stable, and not
destroyed during listen(2) as they are today. So, move them out of the
sockbuf and into the owning socket. For the sockbuf mutexes, keep a
pointer to the mutex in the sockbuf itself, for now.

The change manages to avoid bloating the size of struct socket. The
slab efficiency is the same as before: 4 sockets per page on LP64
systems.

[tuexen]

Please take into account what gallatin@ says regarding performance impacts.

[markj]

sys/sys/sockbuf.h

83

Rather than keeping this pointer, I think it'd be better to introduce SOCK_RECVBUF_LOCK() and SOCK_SENDBUF_LOCK() macros which take a socket rather than a socket buffer. They would replace SOCKBUF_LOCK(). I think that most if not all consumers can be trivially adapted to that scheme.

[markj]

sys/sys/sockbuf.h

83

... this is a bit tricky for some code in uipc_sockbuf.c which operates solely on socket buffers but wants to assert that the sockbuf mutex is owned. So maybe we should keep this pointer and use it only there. Most external consumers can easily switch to the new macros.

To be clear, there are two problems with keeping SOCKBUF_MTX(): it is slightly more expensive because of additional indirection, and the sb_mtx pointer is cleared by solisten_proto(), so this pattern is not safe:

SOCKBUF_LOCK(&so->so_snd);
if (SOLISTENING(so)) {
    return (EINVAL);
}
...

[gallatin]

sys/sys/sockbuf.h

83

What is the justification for these locking macros in general? In the beginning, I seem to recall they eased the transition from SPL to real locks.

But these days, they just seem to hide valuable information and add mental overhead for the reader. At least I'm constantly looking up the macro definitions to see if its an SX, sleep or spin mutex, RW lock, etc. Maybe other people have larger mental caches than I do.

I'd much prefer we just did a mtx_lock so that what was happening is clear to the reader.

[gallatin]

In D31658#714086, @tuexen wrote:

Please take into account what gallatin@ says regarding performance impacts.

I'll be happy to try this when it gets close to a final version. I'm moderately concerned about the patch series, since it moves things around that are in the hot path. But we won't know if that matters until its tested.

[markj]

In D31658#714125, @gallatin wrote:

In D31658#714086, @tuexen wrote:

Please take into account what gallatin@ says regarding performance impacts.

I'll be happy to try this when it gets close to a final version. I'm moderately concerned about the patch series, since it moves things around that are in the hot path. But we won't know if that matters until its tested.

Thanks, I can send you a single standalone patch in the next day or two. I do have some follow-up changes planned, but they mostly affect AIO.

sys/sys/sockbuf.h

83

The macros make it easier to swap out implementations, sometimes. Consider that with plain mtx_* usage for the sockbuf lock, I would have to update hundreds of lines in order to move the mutex into the socket. With the macros, all I had to do was modify SOCKBUT_MTX().

I generally don't like using macros for locking that's internal to a single file, but when it's used in different subsystems, as in this case, the macros make life easier.

[gallatin]

I agree that removing the indirection with a new SOCK_{SEND|RECVBUF}_LOCK() is a good idea.

[markj]

In D31658#718201, @gallatin wrote:

I agree that removing the indirection with a new SOCK_{SEND|RECVBUF}_LOCK() is a good idea.

Thanks for taking a look. I will do that as part of a follow-up series of patches, as it creates a lot of churn.