Fixes call home deadlock

CMakeLists.txt

@@ -58,15 +58,15 @@ set(CMAKE_MACOSX_RPATH TRUE)
 # micro version is changed with a set of small changes or bugfixes anywhere in the project.
 set(LIBNETCONF2_MAJOR_VERSION 4)
 set(LIBNETCONF2_MINOR_VERSION 2)
-set(LIBNETCONF2_MICRO_VERSION 9)
+set(LIBNETCONF2_MICRO_VERSION 10)
 set(LIBNETCONF2_VERSION ${LIBNETCONF2_MAJOR_VERSION}.${LIBNETCONF2_MINOR_VERSION}.${LIBNETCONF2_MICRO_VERSION})
 
 # Version of the library
 # Major version is changed with every backward non-compatible API/ABI change in the library, minor version changes
 # with backward compatible change and micro version is connected with any internal change of the library.
 set(LIBNETCONF2_MAJOR_SOVERSION 5)
 set(LIBNETCONF2_MINOR_SOVERSION 2)
-set(LIBNETCONF2_MICRO_SOVERSION 9)
+set(LIBNETCONF2_MICRO_SOVERSION 10)
 set(LIBNETCONF2_SOVERSION_FULL ${LIBNETCONF2_MAJOR_SOVERSION}.${LIBNETCONF2_MINOR_SOVERSION}.${LIBNETCONF2_MICRO_SOVERSION})
 set(LIBNETCONF2_SOVERSION ${LIBNETCONF2_MAJOR_SOVERSION})
 

src/server_config.c

@@ -6146,52 +6146,6 @@ nc_server_config_cert_exp_notif_thread_wakeup(void)
 
 #endif /* NC_ENABLED_SSH_TLS */
 
-/**
- * @brief Wait for any pending updates to complete, then mark the server as "applying configuration".
- *
- * @return 0 on success, 1 on timeout.
- */
-static int
-nc_server_config_update_start(void)
-{
-    if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) {
-        return 1;
-    }
-
-    if (!server_opts.applying_config) {
-        /* set the flag */
-        server_opts.applying_config = 1;
-    }
-
-    /* UNLOCK */
-    nc_rwlock_unlock(&server_opts.config_lock, __func__);
-    return 0;
-}
-
-/**
- * @brief Clear the "applying configuration" flag once the configuration update is done.
- */
-static void
-nc_server_config_update_end(void)
-{
-    int r;
-    struct timespec ts_timeout;
-
-    /* get the time point of timeout */
-    nc_timeouttime_get(&ts_timeout, NC_SERVER_CONFIG_UPDATE_WAIT_TIMEOUT_SEC * 1000);
-
-    /* WR LOCK */
-    r = nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_WRITE, NC_CONFIG_LOCK_TIMEOUT, __func__);
-
-    /* clear the flag */
-    server_opts.applying_config = 0;
-
-    if (r == 1) {
-        /* UNLOCK */
-        nc_rwlock_unlock(&server_opts.config_lock, __func__);
-    }
-}
-
 API int
 nc_server_config_setup_diff(const struct lyd_node *data)
 {
@@ -6200,8 +6154,13 @@ nc_server_config_setup_diff(const struct lyd_node *data)
 
     NC_CHECK_ARG_RET(NULL, data, 1);
 
-    /* wait until previous is done, then mark us as applying */
-    NC_CHECK_RET(nc_server_config_update_start());
+    /* CONFIG UPDATE LOCK
+     * - avoids concurrent updates
+     * - readers are still allowed to read the old config while we are applying the new one
+     */
+    if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) {
+        return 1;
+    }
 
     /* CONFIG RD LOCK */
     if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) {
@@ -6269,8 +6228,9 @@ nc_server_config_setup_diff(const struct lyd_node *data)
         /* free the new config in case of error */
         nc_server_config_free(&config_copy);
     }
-    nc_server_config_update_end();
 
+    /* CONFIG UPDATE UNLOCK */
+    nc_mutex_unlock(&server_opts.config_update_lock, __func__);
     return ret;
 }
 
@@ -6283,8 +6243,13 @@ nc_server_config_setup_data(const struct lyd_node *data)
 
     NC_CHECK_ARG_RET(NULL, data, 1);
 
-    /* wait until previous is done, then mark us as applying */
-    NC_CHECK_RET(nc_server_config_update_start());
+    /* CONFIG UPDATE LOCK
+     * - avoids concurrent updates
+     * - readers are still allowed to read the old config while we are applying the new one
+     */
+    if (nc_mutex_lock(&server_opts.config_update_lock, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) {
+        return 1;
+    }
 
     /* check that the config data are not diff (no op attr) */
     LY_LIST_FOR(data, tree) {
@@ -6356,8 +6321,9 @@ nc_server_config_setup_data(const struct lyd_node *data)
         /* free the new config in case of error */
         nc_server_config_free(&config);
     }
-    nc_server_config_update_end();
 
+    /* CONFIG UPDATE UNLOCK */
+    nc_mutex_unlock(&server_opts.config_update_lock, __func__);
     return ret;
 }
 

src/session.c

@@ -1116,8 +1116,16 @@ add_cpblt(const char *capab, char ***cpblts, int *size, int *count)
     ++(*count);
 }
 
-API char **
-nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version)
+/**
+ * @brief Get the server capabilities.
+ *
+ * @param[in] ctx libyang context.
+ * @param[in] version YANG version of the schemas to be included in result.
+ * @param[in] config_locked Whether the configuration lock is already held or should be acquired.
+ * @return Array of capabilities, NULL on error.
+ */
+static char **
+_nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version, int config_locked)
 {
     char **cpblts;
     const struct lys_module *mod;
@@ -1232,7 +1240,7 @@ nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version)
     /* models */
     u = 0;
     while ((mod = ly_ctx_get_module_iter(ctx, &u))) {
-        if (nc_server_is_mod_ignored(mod->name)) {
+        if (nc_server_is_mod_ignored(mod->name, config_locked)) {
             /* ignored, not part of the cababilities */
             continue;
         }
@@ -1337,10 +1345,16 @@ nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version)
     return NULL;
 }
 
+API char **
+nc_server_get_cpblts_version(const struct ly_ctx *ctx, LYS_VERSION version)
+{
+    return _nc_server_get_cpblts_version(ctx, version, 0);
+}
+
 API char **
 nc_server_get_cpblts(const struct ly_ctx *ctx)
 {
-    return nc_server_get_cpblts_version(ctx, LYS_VERSION_UNDEF);
+    return _nc_server_get_cpblts_version(ctx, LYS_VERSION_UNDEF, 0);
 }
 
 static int
@@ -1400,8 +1414,15 @@ parse_cpblts(struct lyd_node *capabilities, char ***list)
     return ver;
 }
 
+/**
+ * @brief Send NETCONF hello message on a session.
+ *
+ * @param[in] session Session to send the message on.
+ * @param[in] config_locked Whether the configuration READ lock is already held (only relevant for server side).
+ * @return Sent message type.
+ */
 static NC_MSG_TYPE
-nc_send_hello_io(struct nc_session *session)
+nc_send_hello_io(struct nc_session *session, int config_locked)
 {
     NC_MSG_TYPE ret;
     int i, timeout_io;
@@ -1419,7 +1440,7 @@ nc_send_hello_io(struct nc_session *session)
         timeout_io = NC_CLIENT_HELLO_TIMEOUT * 1000;
         sid = NULL;
     } else {
-        cpblts = nc_server_get_cpblts_version(session->ctx, LYS_VERSION_1_0);
+        cpblts = _nc_server_get_cpblts_version(session->ctx, LYS_VERSION_1_0, config_locked);
         if (!cpblts) {
             return NC_MSG_ERROR;
         }
@@ -1609,7 +1630,7 @@ nc_handshake_io(struct nc_session *session)
 {
     NC_MSG_TYPE type;
 
-    type = nc_send_hello_io(session);
+    type = nc_send_hello_io(session, 0);
     if (type != NC_MSG_HELLO) {
         return type;
     }
@@ -1623,6 +1644,26 @@ nc_handshake_io(struct nc_session *session)
     return type;
 }
 
+NC_MSG_TYPE
+nc_ch_handshake_io(struct nc_session *session)
+{
+    NC_MSG_TYPE type;
+
+    if ((session->side != NC_SERVER) || !(session->flags & NC_SESSION_CALLHOME)) {
+        ERR(session, "Call Home handshake can only be performed on a server session with Call Home flag set.");
+        return NC_MSG_ERROR;
+    }
+
+    type = nc_send_hello_io(session, 1);
+    if (type != NC_MSG_HELLO) {
+        return type;
+    }
+
+    type = nc_server_recv_hello_io(session);
+
+    return type;
+}
+
 #ifdef NC_ENABLED_SSH_TLS
 
 /**

src/session_p.h

@@ -781,7 +781,6 @@ struct nc_server_opts {
      *               - options read when accepting sessions - READ lock */
     pthread_rwlock_t config_lock;       /**< Lock for the server configuration. */
     struct nc_server_config config;     /**< YANG Server configuration. */
-    int applying_config;                /**< Flag indicating that the server configuration is currently being applied. */
 
 #ifdef NC_ENABLED_SSH_TLS
     char *authkey_path_fmt;             /**< Path to users' public keys that may contain tokens with special meaning. */
@@ -831,6 +830,10 @@ struct nc_server_opts {
     /* ACCESS unlocked - set from env */
     FILE *tls_keylog_file;                  /**< File to log TLS secrets to. */
 #endif
+
+    /* ACCESS locked - separate lock to allow concurrent reads of the config while an update is being applied */
+    pthread_mutex_t config_update_lock; /**< Lock for synchronizing configuration updates, used to wait for pending
+                                             configuration update to complete before accepting a new one. */
 };
 
 /**
@@ -1224,6 +1227,15 @@ int nc_ctx_check_and_fill(struct nc_session *session);
  */
 NC_MSG_TYPE nc_handshake_io(struct nc_session *session);
 
+/**
+ * @brief Perform NETCONF handshake on a server-side Call Home session.
+ *
+ * @param[in] session NETCONF session to use.
+ * @return NC_MSG_HELLO on success, NC_MSG_BAD_HELLO on client \<hello\> message parsing fail,
+ * NC_MSG_WOULDBLOCK on timeout, NC_MSG_ERROR on other error.
+ */
+NC_MSG_TYPE nc_ch_handshake_io(struct nc_session *session);
+
 /**
  * @brief Bind a socket to an address and a port.
  *
@@ -1431,9 +1443,10 @@ int nc_session_tls_crl_from_cert_ext_fetch(void *leaf_cert, void *cert_store, vo
  * @brief Check whether a module is not ignored by the server.
  *
  * @param[in] mod_name Module name to check.
+ * @param[in] config_locked Whether the configuration lock is already held or should be acquired in this function.
  * @return Whether the module is ignored.
  */
-int nc_server_is_mod_ignored(const char *mod_name);
+int nc_server_is_mod_ignored(const char *mod_name, int config_locked);
 
 /**
  * Functions

src/session_server.c

@@ -54,7 +54,8 @@
 
 struct nc_server_opts server_opts = {
     .hello_lock = PTHREAD_RWLOCK_INITIALIZER,
-    .config_lock = PTHREAD_RWLOCK_INITIALIZER
+    .config_lock = PTHREAD_RWLOCK_INITIALIZER,
+    .config_update_lock = PTHREAD_MUTEX_INITIALIZER,
 };
 
 static nc_rpc_clb global_rpc_clb = NULL;
@@ -3015,7 +3016,7 @@ nc_connect_ch_endpt(struct nc_ch_endpt *endpt, nc_server_ch_session_acquire_ctx_
     (*session)->id = ATOMIC_INC_RELAXED(server_opts.new_session_id);
 
     /* NETCONF handshake */
-    msgtype = nc_handshake_io(*session);
+    msgtype = nc_ch_handshake_io(*session);
     if (msgtype != NC_MSG_HELLO) {
         goto fail;
     }
@@ -4513,14 +4514,16 @@ nc_server_notif_cert_expiration_thread_stop(int wait)
 #endif /* NC_ENABLED_SSH_TLS */
 
 int
-nc_server_is_mod_ignored(const char *mod_name)
+nc_server_is_mod_ignored(const char *mod_name, int config_locked)
 {
     int ignored = 0;
     LY_ARRAY_COUNT_TYPE i;
 
-    /* LOCK */
-    if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) {
-        return 0;
+    if (!config_locked) {
+        /* LOCK */
+        if (nc_rwlock_lock(&server_opts.config_lock, NC_RWLOCK_READ, NC_CONFIG_LOCK_TIMEOUT, __func__) != 1) {
+            return 0;
+        }
     }
 
     LY_ARRAY_FOR(server_opts.config.ignored_modules, i) {
@@ -4530,8 +4533,10 @@ nc_server_is_mod_ignored(const char *mod_name)
         }
     }
 
-    /* UNLOCK */
-    nc_rwlock_unlock(&server_opts.config_lock, __func__);
+    if (!config_locked) {
+        /* UNLOCK */
+        nc_rwlock_unlock(&server_opts.config_lock, __func__);
+    }
 
     return ignored;
 }