Move channel mutex out of QUIC_CHANNEL for init/teardown flexibility

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20348)

5 files changed, 70 insertions, 46 deletions

diff --git a/include/internal/quic_channel.h b/include/internal/quic_channel.h
index 93cdfd6919..cd51fe30a4 100644
--- a/include/internal/quic_channel.h
+++ b/include/internal/quic_channel.h
@@ -54,9 +54,11 @@
  * To support thread assisted mode, QUIC_CHANNEL can be used by multiple
  * threads. **It is the caller's responsibility to ensure that the QUIC_CHANNEL
  * is only accessed (whether via its methods or via direct access to its state)
- * while the QUIC_CHANNEL mutex is held**, except for methods explicitly marked
- * as not requiring prior locking. See ossl_quic_channel_get_mutex() for more
- * information. This is an unchecked precondition.
+ * while the channel mutex is held**, except for methods explicitly marked as
+ * not requiring prior locking. This is an unchecked precondition.
+ *
+ * The instantiator of the channel is responsible for providing a suitable
+ * mutex which then serves as the channel mutex; see QUIC_CHANNEL_ARGS.
  */
 
 #  define QUIC_NEEDS_LOCK
@@ -70,10 +72,18 @@
 #  define QUIC_CHANNEL_STATE_TERMINATED                  4
 
 typedef struct quic_channel_args_st {
-    OSSL_LIB_CTX *libctx;
-    const char *propq;
-    int is_server;
-    SSL *tls;
+    OSSL_LIB_CTX    *libctx;
+    const char      *propq;
+    int             is_server;
+    SSL             *tls;
+
+    /*
+     * This must be a mutex the lifetime of which will exceed that of the
+     * channel. The instantiator of the channel is responsible for providing a
+     * mutex as this makes it easier to handle instantiation and teardown of
+     * channels in situations potentially requiring locking.
+     */
+    CRYPTO_RWLOCK   *mutex;
 } QUIC_CHANNEL_ARGS;
 
 typedef struct quic_channel_st QUIC_CHANNEL;
@@ -213,31 +223,20 @@ QUIC_DEMUX *ossl_quic_channel_get0_demux(QUIC_CHANNEL *ch);
 SSL *ossl_quic_channel_get0_ssl(QUIC_CHANNEL *ch);
 
 /*
- * Retreves the channel mutex, which can be used to synchronise access to
- * channel functions and internal data. In order to allow locks to be acquired
- * and released with the correct granularity, it is the caller's responsibility
- * to ensure this lock is held for write while calling any QUIC_CHANNEL method.
+ * Retrieves a pointer to the channel mutex which was provided at the time the
+ * channel was instantiated. In order to allow locks to be acquired and released
+ * with the correct granularity, it is the caller's responsibility to ensure
+ * this lock is held for write while calling any QUIC_CHANNEL method, except for
+ * methods explicitly designed otherwise.
  *
  * This method is thread safe and does not require prior locking. It can also be
- * called while the lock is already held.
+ * called while the lock is already held. Note that this is simply a convenience
+ * function to access the mutex which was passed to the channel at instantiation
+ * time; it does not belong to the channel but rather is presumed to belong to
+ * the owner of the channel.
  */
 CRYPTO_RWLOCK *ossl_quic_channel_get_mutex(QUIC_CHANNEL *ch);
 
-/*
- * Locks the channel mutex. It is roughly analagous to locking the mutex
- * returned by ossl_quic_channel_get_mutex() but might be able to avoid locking
- * where thread assisted mode is not being used, thus it is recommended that
- * these methods are used uniformly rather than locking the channel mutex
- * directly.
- *
- * This method is (obviously) thread safe and does not require prior locking. It
- * must not be called while the lock is already held.
- */
-int ossl_quic_channel_lock(QUIC_CHANNEL *ch);
-
-/* Unlocks the channel mutex. */
-void ossl_quic_channel_unlock(QUIC_CHANNEL *ch);
-
 # endif
 
 #endif
diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
index efe8ca6def..07c680511c 100644
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -115,10 +115,6 @@ static int ch_init(QUIC_CHANNEL *ch)
     qtx_args.mdpl = QUIC_MIN_INITIAL_DGRAM_LEN;
     ch->rx_max_udp_payload_size = qtx_args.mdpl;
 
-    ch->mutex = CRYPTO_THREAD_lock_new();
-    if (ch->mutex == NULL)
-        goto err;
-
     ch->qtx = ossl_qtx_new(&qtx_args);
     if (ch->qtx == NULL)
         goto err;
@@ -322,7 +318,6 @@ static void ch_cleanup(QUIC_CHANNEL *ch)
     ossl_qrx_free(ch->qrx);
     ossl_quic_demux_free(ch->demux);
     OPENSSL_free(ch->local_transport_params);
-    CRYPTO_THREAD_lock_free(ch->mutex);
 }
 
 QUIC_CHANNEL *ossl_quic_channel_new(const QUIC_CHANNEL_ARGS *args)
@@ -336,6 +331,7 @@ QUIC_CHANNEL *ossl_quic_channel_new(const QUIC_CHANNEL_ARGS *args)
     ch->propq       = args->propq;
     ch->is_server   = args->is_server;
     ch->tls         = args->tls;
+    ch->mutex       = args->mutex;
 
     if (!ch_init(ch)) {
         OPENSSL_free(ch);
@@ -453,16 +449,6 @@ CRYPTO_MUTEX *ossl_quic_channel_get_mutex(QUIC_CHANNEL *ch)
     return ch->mutex;
 }
 
-int ossl_quic_channel_lock(QUIC_CHANNEL *ch)
-{
-    return ossl_crypto_mutex_lock(ch->mutex);
-}
-
-void ossl_quic_channel_unlock(QUIC_CHANNEL *ch)
-{
-    ossl_crypto_mutex_unlock(ch->mutex);
-}
-
 /*
  * QUIC Channel: Callbacks from Miscellaneous Subsidiary Components
  * ================================================================
diff --git a/ssl/quic/quic_channel_local.h b/ssl/quic/quic_channel_local.h
index 2f63119cef..e1e60bf2b4 100644
--- a/ssl/quic/quic_channel_local.h
+++ b/ssl/quic/quic_channel_local.h
@@ -27,7 +27,9 @@ struct quic_channel_st {
 
     /*
      * Master synchronisation mutex used for thread assisted mode
-     * synchronisation.
+     * synchronisation. We don't own this; the instantiator of the channel
+     * passes it to us and is responsible for freeing it after channel
+     * destruction.
      */
     CRYPTO_RWLOCK                   *mutex;
 
diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index 73501cd286..c17b5354a2 100644
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -42,7 +42,7 @@ static int block_until_pred(QUIC_CONNECTION *qc,
 
     rtor = ossl_quic_channel_get_reactor(qc->ch);
     return ossl_quic_reactor_block_until_pred(rtor, pred, pred_arg, flags,
-                                              ossl_quic_channel_get_mutex(qc->ch));
+                                              qc->mutex);
 }
 
 /*
@@ -105,6 +105,25 @@ static ossl_inline int expect_quic_conn(const QUIC_CONNECTION *qc)
 }
 
 /*
+ * Ensures that the channel mutex is held for a method which touches channel
+ * state.
+ *
+ * Precondition: Channel mutex is not held (unchecked)
+ */
+static int quic_lock(QUIC_CONNECTION *qc)
+{
+    return CRYPTO_THREAD_write_lock(qc->mutex);
+}
+
+/* Precondition: Channel mutex is held (unchecked) */
+QUIC_NEEDS_LOCK
+static void quic_unlock(QUIC_CONNECTION *qc)
+{
+    CRYPTO_THREAD_unlock(qc->mutex);
+}
+
+
+/*
  * QUIC Front-End I/O API: Initialization
  * ======================================
  *
@@ -139,6 +158,9 @@ SSL *ossl_quic_new(SSL_CTX *ctx)
     if (qc->tls == NULL || (sc = SSL_CONNECTION_FROM_SSL(qc->tls)) == NULL)
          goto err;
 
+    if ((qc->mutex = CRYPTO_THREAD_lock_new()) == NULL)
+        goto err;
+
     /* Channel is not created yet. */
     qc->ssl_mode   = qc->ssl.ctx->mode;
     qc->last_error = SSL_ERROR_NONE;
@@ -147,6 +169,7 @@ SSL *ossl_quic_new(SSL_CTX *ctx)
     return ssl_base;
 
 err:
+    SSL_free(qc->tls);
     OPENSSL_free(qc);
     return NULL;
 }
@@ -169,6 +192,7 @@ void ossl_quic_free(SSL *s)
     /* Note: SSL_free calls OPENSSL_free(qc) for us */
 
     SSL_free(qc->tls);
+    CRYPTO_THREAD_lock_free(qc->mutex);
 }
 
 /* SSL method init */
@@ -601,7 +625,7 @@ static int configure_channel(QUIC_CONNECTION *qc)
     return 1;
 }
 
-QUIC_TODO_LOCK
+QUIC_NEEDS_LOCK
 static int ensure_channel(QUIC_CONNECTION *qc)
 {
     QUIC_CHANNEL_ARGS args = {0};
@@ -626,7 +650,7 @@ static int ensure_channel(QUIC_CONNECTION *qc)
  * via calls made to us from the application prior to starting a handshake
  * attempt.
  */
-QUIC_TODO_LOCK
+QUIC_NEEDS_LOCK
 static int ensure_channel_and_start(QUIC_CONNECTION *qc)
 {
     if (!ensure_channel(qc))
@@ -793,6 +817,7 @@ int ossl_quic_get_error(const QUIC_CONNECTION *qc, int i)
  *   - SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER.
  *
  */
+QUIC_NEEDS_LOCK
 static void quic_post_write(QUIC_CONNECTION *qc, int did_append, int do_tick)
 {
     /*
@@ -820,6 +845,7 @@ struct quic_write_again_args {
     size_t              total_written;
 };
 
+QUIC_NEEDS_LOCK
 static int quic_write_again(void *arg)
 {
     struct quic_write_again_args *args = arg;
@@ -847,6 +873,7 @@ static int quic_write_again(void *arg)
     return 0;
 }
 
+QUIC_NEEDS_LOCK
 static int quic_write_blocking(QUIC_CONNECTION *qc, const void *buf, size_t len,
                                size_t *written)
 {
@@ -915,6 +942,7 @@ static void aon_write_finish(QUIC_CONNECTION *qc)
     qc->aon_buf_len             = 0;
 }
 
+QUIC_NEEDS_LOCK
 static int quic_write_nonblocking_aon(QUIC_CONNECTION *qc, const void *buf,
                                       size_t len, size_t *written)
 {
@@ -1001,6 +1029,7 @@ static int quic_write_nonblocking_aon(QUIC_CONNECTION *qc, const void *buf,
     return QUIC_RAISE_NORMAL_ERROR(qc, SSL_ERROR_WANT_WRITE);
 }
 
+QUIC_NEEDS_LOCK
 static int quic_write_nonblocking_epw(QUIC_CONNECTION *qc, const void *buf, size_t len,
                                       size_t *written)
 {
@@ -1060,6 +1089,7 @@ struct quic_read_again_args {
     int             peek;
 };
 
+QUIC_NEEDS_LOCK
 static int quic_read_actual(QUIC_CONNECTION *qc,
                             QUIC_STREAM *stream,
                             void *buf, size_t buf_len,
@@ -1114,6 +1144,7 @@ static int quic_read_actual(QUIC_CONNECTION *qc,
     return 1;
 }
 
+QUIC_NEEDS_LOCK
 static int quic_read_again(void *arg)
 {
     struct quic_read_again_args *args = arg;
diff --git a/ssl/quic/quic_local.h b/ssl/quic/quic_local.h
index f4397447dd..300332307c 100644
--- a/ssl/quic/quic_local.h
+++ b/ssl/quic/quic_local.h
@@ -50,6 +50,12 @@ struct quic_conn_st {
      */
     QUIC_CHANNEL                    *ch;
 
+    /*
+     * The mutex used to synchronise access to the QUIC_CHANNEL. We own this but
+     * provide it to the channel.
+     */
+    CRYPTO_RWLOCK                   *mutex;
+
     /* Our single bidirectional application data stream. */
     QUIC_STREAM                     *stream0;