From 76fee17aaa7f710506caaf53473931fa8658144c Mon Sep 17 00:00:00 2001 From: Arran Cudbard-Bell Date: Wed, 8 May 2024 17:43:06 -0600 Subject: [PATCH] Make the dl_module code threadsafe in regards to loading and unloading modules Ugh... I hate writing this sort of code, but unless we want to have a master "load and unload" modules thread, that we signal using message passing, this is unfortunately the way we need to do it. Note: This only protects the dl_loader, not the instances. --- src/lib/server/dl_module.c | 97 ++++++++++++++++++++++++++++++++++---- src/lib/server/dl_module.h | 17 ++++++- 2 files changed, 104 insertions(+), 10 deletions(-) diff --git a/src/lib/server/dl_module.c b/src/lib/server/dl_module.c index 792d88ff8f..9a8a6f114a 100644 --- a/src/lib/server/dl_module.c +++ b/src/lib/server/dl_module.c @@ -34,6 +34,7 @@ RCSID("$Id$") #include #include +#include #include #include @@ -44,9 +45,10 @@ RCSID("$Id$") * Provides space to store instance data. */ struct dl_module_loader_s { - fr_rb_tree_t *module_tree; - fr_rb_tree_t *inst_data_tree; - dl_loader_t *dl_loader; + pthread_mutex_t lock; //!< + fr_rb_tree_t *module_tree; //!< Module's dl handles. + fr_rb_tree_t *inst_data_tree; //!< Module's instance data. + dl_loader_t *dl_loader; //!< A list of loaded libraries, and symbol to callback mappings. }; static dl_module_loader_t *dl_module_loader; @@ -368,6 +370,29 @@ static void dl_module_instance_data_alloc(dl_module_inst_t *dl_inst, dl_module_t */ static int _dl_module_free(dl_module_t *dl_module) { + /* + * Talloc destructors access the talloc chunk after + * calling the destructor, which could lead to a race + * if the mutex is acquired within the destructor + * itself. This unfortunately means that we have to + * free modules using a dedicated free function which + * locks the dl_module_loader mutex. + * + * Ensure this module is not being freed using the + * normal talloc hierarchy, or with talloc_free(). + */ + fr_assert_msg(pthread_mutex_trylock(&dl_module->loader->lock) != 0, + "dl_module_loader->lock not held when freeing module, " + "use dl_module_free() to free modules, not talloc_free"); + + /* + * Decrement refcounts, freeing at zero + */ + if (dl_module->refs > 0) { + dl_module->refs--; + return -1; + } + /* * dl is empty if we tried to load it and failed. */ @@ -390,6 +415,28 @@ static int _dl_module_free(dl_module_t *dl_module) return 0; } +/** Free a dl_module (when there are no more references to it) + * + * Decrement the reference count for a module, freeing it and unloading the module if there are no + * more references. + * + * @note This must be used to free modules, not talloc_free(). + * + * @return + * - 0 on success. + * - -1 if the module wasn't freed. This likely means there are more ferences held to it. + */ +int dl_module_free(dl_module_t *dl_module) +{ + int ret; + + pthread_mutex_lock(&dl_module->loader->lock); + ret = talloc_free(dl_module); + pthread_mutex_unlock(&dl_module->loader->lock); + + return ret; +} + /** Load a module library using dlopen() or return a previously loaded module from the cache * * When the dl_module_t is no longer used, talloc_free() may be used to free it. @@ -397,6 +444,11 @@ static int _dl_module_free(dl_module_t *dl_module) * When all references to the original dlhandle are freed, dlclose() will be called on the * dlhandle to unload the module. * + * @note This function is threadsafe. Multiple callers may attempt to load the same module + * at the same time, and the module will only be loaded once, and will not be freed + * until all callers have released their references to it. This is useful for dynamic/runtime + * loading of modules. + * * @param[in] parent The dl_module_t of the parent module, e.g. rlm_sql for rlm_sql_postgresql. * @param[in] name of the module e.g. sql for rlm_sql. * @param[in] type Used to determine module name prefixes. Must be one of: @@ -407,7 +459,7 @@ static int _dl_module_free(dl_module_t *dl_module) * - Module handle holding dlhandle, and module's public interface structure. * - NULL if module couldn't be loaded, or some other error occurred. */ -dl_module_t const *dl_module(dl_module_t const *parent, char const *name, dl_module_type_t type) +dl_module_t *dl_module_alloc(dl_module_t const *parent, char const *name, dl_module_type_t type) { dl_module_t *dl_module = NULL; dl_t *dl = NULL; @@ -432,18 +484,28 @@ dl_module_t const *dl_module(dl_module_t const *parent, char const *name, dl_mod for (p = module_name, q = p + talloc_array_length(p) - 1; p < q; p++) *p = tolower((uint8_t) *p); + pthread_mutex_lock(&dl_module_loader->lock); /* * If the module's already been loaded, increment the reference count. */ dl_module = fr_rb_find(dl_module_loader->module_tree, &(dl_module_t){ .dl = &(dl_t){ .name = module_name }}); if (dl_module) { + dl_module->refs++; + + /* + * Release the lock, the caller is guaranteed to have a completely + * loaded module, which won't be freed out from underneath them until + * the reference count drops to zero. + */ + pthread_mutex_unlock(&dl_module_loader->lock); talloc_free(module_name); - talloc_increase_ref_count(dl_module); + return dl_module; } - dl_module = talloc_zero(dl_module_loader, dl_module_t); + MEM(dl_module = talloc_zero(dl_module_loader, dl_module_t)); + dl_module->loader = dl_module_loader; dl_module->parent = parent; dl_module->type = type; talloc_set_destructor(dl_module, _dl_module_free); /* Do this late */ @@ -496,6 +558,15 @@ dl_module_t const *dl_module(dl_module_t const *parent, char const *name, dl_mod goto error; } + /* + * Hold the lock for the entire module loading process. + * + * This ensures that all the global resources the module has symbol callbacks + * registered for, are fully populated, before something else attempts to use + * it. + */ + pthread_mutex_unlock(&dl_module_loader->lock); + talloc_free(module_name); return dl_module; @@ -535,7 +606,7 @@ static int _dl_module_instance_free(dl_module_inst_t *dl_inst) * won't be unloaded until all instances of that module * have been destroyed. */ - talloc_decrease_ref_count(dl_inst->module); + dl_module_free(dl_inst->module); return 0; } @@ -587,7 +658,7 @@ int dl_module_instance(TALLOC_CTX *ctx, dl_module_inst_t **out, MEM(dl_inst = talloc_zero(ctx, dl_module_inst_t)); - dl_inst->module = dl_module(parent ? parent->module : NULL, mod_name, type); + dl_inst->module = dl_module_alloc(parent ? parent->module : NULL, mod_name, type); if (!dl_inst->module) { talloc_free(dl_inst); return -1; @@ -644,6 +715,12 @@ static int _dl_module_loader_free(dl_module_loader_t *dl_module_l) { int ret = 0; + /* + * Lock must not be held when freeing the loader list. + */ + fr_assert_msg(pthread_mutex_trylock(&dl_module_l->lock) == 0, + "dl_module_loader->lock held when attempting to free dL_module_loader_t"); + if (fr_rb_num_elements(dl_module_l->inst_data_tree) > 0) { #ifndef NDEBUG fr_rb_iter_inorder_t iter; @@ -679,6 +756,9 @@ finish: dl_module_loader = NULL; } + pthread_mutex_unlock(&dl_module_l->lock); + pthread_mutex_destroy(&dl_module_l->lock); + return ret; } @@ -749,6 +829,7 @@ dl_module_loader_t *dl_module_loader_init(char const *lib_dir) ERROR("Failed initialising uctx for dl_loader"); return NULL; } + pthread_mutex_init(&dl_module_loader->lock, NULL); dl_module_loader->dl_loader = dl_loader_init(NULL, dl_module_loader, false, true); if (!dl_module_loader) { diff --git a/src/lib/server/dl_module.h b/src/lib/server/dl_module.h index 19f1b1c7a1..d702e51400 100644 --- a/src/lib/server/dl_module.h +++ b/src/lib/server/dl_module.h @@ -140,6 +140,8 @@ typedef struct { */ typedef struct dl_module_s dl_module_t; struct dl_module_s { + dl_module_loader_t * _CONST loader; //!< Loader that owns this dl. + dl_t * _CONST dl; //!< Dynamic loader handle. dl_module_t const * _CONST parent; //!< of this module. @@ -152,6 +154,15 @@ struct dl_module_s { CONF_SECTION * _CONST conf; //!< The module's global configuration ///< (as opposed to the instance, configuration). ///< May be NULL. + + unsigned int refs; //!< Number of references to this module. + ///< This is maintained as a separate counter + ///< (instead of using talloc refs) because it needs + ///< to be thread safe. + ///< The talloc code accesses the chunk after calling + ///< the destructor, so we can't lock the loader mutex + ///< inside the destructor and expect things to work + ///< correctly. bool _CONST in_tree; }; @@ -161,7 +172,7 @@ struct dl_module_s { */ struct dl_module_instance_s { char const * _CONST name; //!< Instance name. - dl_module_t const * _CONST module; //!< Module + dl_module_t * _CONST module; //!< Module void * _CONST data; //!< Module instance's parsed configuration. CONF_SECTION * _CONST conf; //!< Module's instance configuration. dl_module_inst_t const * _CONST parent; //!< Parent module's instance (if any). @@ -171,7 +182,9 @@ struct dl_module_instance_s { extern fr_table_num_sorted_t const dl_module_type_prefix[]; extern size_t dl_module_type_prefix_len; -dl_module_t const *dl_module(dl_module_t const *parent, char const *name, dl_module_type_t type); +int dl_module_free(dl_module_t *dl_module); + +dl_module_t *dl_module_alloc(dl_module_t const *parent, char const *name, dl_module_type_t type); dl_module_inst_t const *dl_module_instance_root(dl_module_inst_t const *dl_inst); -- 2.47.3