ktls: Fix interlocking between ktls_enable_rx() and listen(2)

[markj]

Summary

The TCP_TXTLS_ENABLE and TCP_RXTLS_ENABLE socket option handlers check
whether the socket is listening socket and fail if so, but this check is
racy. Since we have to lock the socket buffer later anyway, defer the
check to that point.

ktls_enable_tx() locks the send buffer's I/O lock, which will fail if
the socket is a listening socket, so no explicit checks are needed. In
ktls_enable_rx(), which does not acquire the I/O lock for some reason,
use an explicit SOLISTENING() check after locking the recv socket
buffer.

Otherwise, a concurrent solisten_proto() call can trigger crashes and
memory leaks by wiping out socket buffers as ktls_enable_*() is
modifying them.

Also make sure that a KTLS-enabled socket can't be converted to a
listening socket, and use SOCK_(SEND|RECV)BUF_LOCK macros instead of the
old ones.

Reported by: syzkaller

[gallatin]

It might be nice to document why the SOLISTENTING() check is not needed in the tx case..

[markj]

In D38504#876841, @gallatin wrote:

It might be nice to document why the SOLISTENTING() check is not needed in the tx case..

Why exactly do we acquire the SEND_IO lock in enable_tx but we don't acquire the RECV_IO lock in enable_rx? It's a bit hard to write a useful comment without that info.

[gallatin]

In D38504#877161, @markj wrote:

In D38504#876841, @gallatin wrote:

It might be nice to document why the SOLISTENTING() check is not needed in the tx case..

Why exactly do we acquire the SEND_IO lock in enable_tx but we don't acquire the RECV_IO lock in enable_rx? It's a bit hard to write a useful comment without that info.

Maybe not acquiring the RECV IO lock is a bug? I don't (yet) use the rx code, so I've not paid close attention to it. Maybe @jhb knows?

[jhb]

In D38504#880532, @gallatin wrote:

In D38504#877161, @markj wrote:

In D38504#876841, @gallatin wrote:

It might be nice to document why the SOLISTENTING() check is not needed in the tx case..

Why exactly do we acquire the SEND_IO lock in enable_tx but we don't acquire the RECV_IO lock in enable_rx? It's a bit hard to write a useful comment without that info.

Maybe not acquiring the RECV IO lock is a bug? I don't (yet) use the rx code, so I've not paid close attention to it. Maybe @jhb knows?

We acquire the SEND_IO lock to synchronize with concurrent calls to write() or sendfile() on the socket from other threads.

On the receive side, the socket buffer is sufficient I believe as the arriving data comes from interrupt threads queueing data via sbappend* which is done under the socket buffer lock. For threads calling read() concurrently, I think losing the race is harmless as they can only drain previously-received data? However, perhaps a thread can do a concurrent blocking read() which would sleep in soreceive_stream() and won't take the KTLS detour when it is later awakened, so perhaps we do need the I/O lock for the rx case as well.

sys/kern/uipc_socket.c

1008

[jhb]

In D38504#884206, @jhb wrote:

In D38504#880532, @gallatin wrote:

In D38504#877161, @markj wrote:

In D38504#876841, @gallatin wrote:

It might be nice to document why the SOLISTENTING() check is not needed in the tx case..

Why exactly do we acquire the SEND_IO lock in enable_tx but we don't acquire the RECV_IO lock in enable_rx? It's a bit hard to write a useful comment without that info.

Maybe not acquiring the RECV IO lock is a bug? I don't (yet) use the rx code, so I've not paid close attention to it. Maybe @jhb knows?

We acquire the SEND_IO lock to synchronize with concurrent calls to write() or sendfile() on the socket from other threads.

On the receive side, the socket buffer is sufficient I believe as the arriving data comes from interrupt threads queueing data via sbappend* which is done under the socket buffer lock. For threads calling read() concurrently, I think losing the race is harmless as they can only drain previously-received data? However, perhaps a thread can do a concurrent blocking read() which would sleep in soreceive_stream() and won't take the KTLS detour when it is later awakened, so perhaps we do need the I/O lock for the rx case as well.

So the I/O lock would not close that race. Instead, we need to move the existing check for KTLS in soreceive_stream below the restart label I think.

[glebius]

sys/kern/uipc_socket.c

989–1008

[glebius]

sys/kern/uipc_socket.c

989–1008

IMHO, throwing all socket states that don't allow it to turn into a listening under one if (!SOLISTENING(so)) reduces paste and makes it easier to read.

[markj]

In D38504#884208, @jhb wrote:

In D38504#884206, @jhb wrote:

In D38504#880532, @gallatin wrote:

In D38504#877161, @markj wrote:

In D38504#876841, @gallatin wrote:

It might be nice to document why the SOLISTENTING() check is not needed in the tx case..

Why exactly do we acquire the SEND_IO lock in enable_tx but we don't acquire the RECV_IO lock in enable_rx? It's a bit hard to write a useful comment without that info.

Maybe not acquiring the RECV IO lock is a bug? I don't (yet) use the rx code, so I've not paid close attention to it. Maybe @jhb knows?

We acquire the SEND_IO lock to synchronize with concurrent calls to write() or sendfile() on the socket from other threads.

On the receive side, the socket buffer is sufficient I believe as the arriving data comes from interrupt threads queueing data via sbappend* which is done under the socket buffer lock. For threads calling read() concurrently, I think losing the race is harmless as they can only drain previously-received data? However, perhaps a thread can do a concurrent blocking read() which would sleep in soreceive_stream() and won't take the KTLS detour when it is later awakened, so perhaps we do need the I/O lock for the rx case as well.

So the I/O lock would not close that race. Instead, we need to move the existing check for KTLS in soreceive_stream below the restart label I think.

Don't we want to do both? soreceive_stream() drops the sockbuf mutex in a couple of places, and I'd think that we want to serialize against that as well.

[markj]

• Add a comment in ktls_enable_tx() explaining why we don't check for a listening socket.
• Fix the ktls check in solisten_proto() and reduce code duplication.
• Add a regression test.

[jhb]

In D38504#886490, @markj wrote:

In D38504#884208, @jhb wrote:

In D38504#884206, @jhb wrote:

In D38504#880532, @gallatin wrote:

In D38504#877161, @markj wrote:

In D38504#876841, @gallatin wrote:

It might be nice to document why the SOLISTENTING() check is not needed in the tx case..

Why exactly do we acquire the SEND_IO lock in enable_tx but we don't acquire the RECV_IO lock in enable_rx? It's a bit hard to write a useful comment without that info.

Maybe not acquiring the RECV IO lock is a bug? I don't (yet) use the rx code, so I've not paid close attention to it. Maybe @jhb knows?

We acquire the SEND_IO lock to synchronize with concurrent calls to write() or sendfile() on the socket from other threads.

On the receive side, the socket buffer is sufficient I believe as the arriving data comes from interrupt threads queueing data via sbappend* which is done under the socket buffer lock. For threads calling read() concurrently, I think losing the race is harmless as they can only drain previously-received data? However, perhaps a thread can do a concurrent blocking read() which would sleep in soreceive_stream() and won't take the KTLS detour when it is later awakened, so perhaps we do need the I/O lock for the rx case as well.

So the I/O lock would not close that race. Instead, we need to move the existing check for KTLS in soreceive_stream below the restart label I think.

Don't we want to do both? soreceive_stream() drops the sockbuf mutex in a couple of places, and I'd think that we want to serialize against that as well.

Hummm, perhaps, and it is probably safest to do so.

[glebius]

sys/kern/uipc_socket.c

989–1008

To my taste this looks better than previous version, but fully collapsing common code solisten_proto_abort() and return would be even better, IMHO. Leaving only two predicates under KERN_TLS.

[markj]

sys/kern/uipc_socket.c

989–1008

I don't like mixing preprocessor directives and C expressions, which is why I didn't do it that way.

I'll upload a version which introduces a local variable instead, so there's no code duplication.

[markj]

Eliminate some code duplication.