diff --git a/Modules/app-esp32-module/source/module.cpp b/Modules/app-esp32-module/source/module.cpp index 66fdfc5d4..20792ccff 100644 --- a/Modules/app-esp32-module/source/module.cpp +++ b/Modules/app-esp32-module/source/module.cpp @@ -47,7 +47,7 @@ void module_refs_end() { void module_refs_release(ModuleRefs* refs) { for (auto* module : refs->modules) { - module_put(module); + module_put_code(module); } refs->modules.clear(); } @@ -57,7 +57,9 @@ extern "C" { // Overrides elf_loader's default KConfig-based symbol resolver with one that looks up symbols // across every started kernel module's own symbol table instead. This module's own symbols come // first, as they replace libc functions for apps (see app_symbols.cpp). -// While an ELF is relocated, the modules it resolves symbols from are referenced, so they can't stop while it runs. +// While an ELF is relocated, the modules it resolves symbols from are referenced, so that their +// code stays mapped while it runs. It's a code reference, so it doesn't stop an app from stopping +// such a module on purpose (e.g. to take over the display) and starting it again afterwards. uintptr_t app_esp32_symbol_resolver(const char* symbolName) { uintptr_t symbol_address; if (module_resolve_symbol(&app_esp32_module, symbolName, &symbol_address)) { @@ -69,13 +71,13 @@ uintptr_t app_esp32_symbol_resolver(const char* symbolName) { } Module* owner = nullptr; - if (!module_resolve_symbol_global_get(symbolName, &symbol_address, &owner)) { + if (!module_resolve_symbol_global_get_code(symbolName, &symbol_address, &owner)) { return 0; } // One reference per module is enough for (auto* module : current_refs->modules) { if (module == owner) { - module_put(owner); + module_put_code(owner); return symbol_address; } } diff --git a/Modules/app-esp32-module/source/module_refs.h b/Modules/app-esp32-module/source/module_refs.h index 919f92d83..ed1008d90 100644 --- a/Modules/app-esp32-module/source/module_refs.h +++ b/Modules/app-esp32-module/source/module_refs.h @@ -5,7 +5,7 @@ #include -/** The kernel modules whose symbols an ELF resolved while it was relocated. Each one is held with module_get(). */ +/** The kernel modules whose symbols an ELF resolved while it was relocated. Each one is held with a code reference (module_get_code()). */ struct ModuleRefs { std::vector modules; }; diff --git a/TactilityKernel/include/tactility/module.h b/TactilityKernel/include/tactility/module.h index ab076ba38..831a87913 100644 --- a/TactilityKernel/include/tactility/module.h +++ b/TactilityKernel/include/tactility/module.h @@ -103,7 +103,7 @@ error_t module_add(struct Module* module); * @brief Remove a module from the system. * @param module module to remove * @retval ERROR_NONE if successful - * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get()). It stays added. + * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get() and module_get_code()). It stays added. */ error_t module_remove(struct Module* module); @@ -118,7 +118,8 @@ error_t module_start(struct Module* module); * @brief Stop the module. * @param module module * @retval ERROR_NONE if successful or if the module wasn't started - * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get()) or a driver of it is bound to a device. The module keeps running. + * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get(), or module_get_code() for a + * module that came from a package) or a driver of it is bound to a device. The module keeps running. * @return otherwise the result of the module's stop function */ error_t module_stop(struct Module* module); @@ -187,6 +188,26 @@ error_t module_get(struct Module* module); */ void module_put(struct Module* module); +/** + * @brief Take a reference on a module for code that was relocated against its symbols. + * @details Unlike module_get(), this reference doesn't prevent module_stop(): relocated code only + * needs the module's code and data to stay mapped, which only module_remove() would break. It does + * prevent module_remove() for as long as the reference is held. + * This is what an app's ELF gets for every module it resolves symbols from, so that the app can + * still stop a module it was linked against (e.g. to take exclusive control of a device) and start + * it again afterwards. Every successful call must be paired with one module_put_code(). + * @param module module to reference + * @retval ERROR_NONE the module is referenced + * @retval ERROR_INVALID_STATE the module isn't added, so its symbols can't be resolved + */ +error_t module_get_code(struct Module* module); + +/** + * @brief Release a reference taken with a successful module_get_code() or module_resolve_symbol_global_get_code(). + * @param module module that was referenced + */ +void module_put_code(struct Module* module); + /** * @brief Resolve a symbol from the module. * @details The module must be added with module_add() for symbol resolution to succeed. It doesn't have to be started. @@ -216,6 +237,19 @@ bool module_resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_add */ bool module_resolve_symbol_global_get(const char* symbol_name, uintptr_t* symbol_address, struct Module** out_module); +/** + * @brief Same as module_resolve_symbol_global(), but also takes a code reference on the module + * that has the symbol (see module_get_code()). + * @details Use this when the resolved symbol ends up in code that outlives the lookup (e.g. an ELF + * relocation), where the module's code must stay mapped but where the module itself may still be + * stopped and started again. + * @param symbol_name name of the symbol to resolve + * @param symbol_address pointer to store the address of the resolved symbol + * @param out_module pointer to store the module that has the symbol. Call module_put_code() on it. + * @return true if the symbol was found. The module is referenced then. + */ +bool module_resolve_symbol_global_get_code(const char* symbol_name, uintptr_t* symbol_address, struct Module** out_module); + #ifdef __cplusplus } #endif diff --git a/TactilityKernel/source/module.cpp b/TactilityKernel/source/module.cpp index 7197226d2..36cd5a9d6 100644 --- a/TactilityKernel/source/module.cpp +++ b/TactilityKernel/source/module.cpp @@ -17,6 +17,9 @@ struct ModuleInternal { bool external = false; // All three are guarded by the ledger mutex int use_count = 0; + // References from relocated code (see module_get_code()): they keep the module mapped, but + // they don't keep it running + int code_count = 0; bool stopping = false; // In the ledger. Its symbols can be resolved from then on, also while it hasn't started yet. bool added = false; @@ -76,7 +79,7 @@ error_t module_add(Module* module) { error_t module_remove(Module* module) { mutex_lock(&ledger.mutex); // Checked under the same lock as the removal, so no reference can be taken in between - if (module->internal != nullptr && module->internal->use_count != 0) { + if (module->internal != nullptr && (module->internal->use_count != 0 || module->internal->code_count != 0)) { mutex_unlock(&ledger.mutex); return ERROR_RESOURCE_BUSY; } @@ -140,6 +143,18 @@ static void end_stopping(ModuleInternal* internal) { mutex_unlock(&ledger.mutex); } +/** + * A usage reference always blocks stopping. A code reference (from relocated code) doesn't: an app + * stops a firmware module it was linked against on purpose, to take over the device that module + * drives, and starts it again when it's done. + * Modules from packages are the exception: the user can stop those from Kernel settings while an + * app is running, so there the references from that app must keep it running. + * Caller must hold the ledger mutex. + */ +static bool is_stop_blocked(const ModuleInternal* internal) { + return internal->use_count != 0 || (internal->external && internal->code_count != 0); +} + error_t module_stop(Module* module) { LOG_I(TAG, "stop %s", module->name); @@ -148,7 +163,7 @@ error_t module_stop(Module* module) { // A module that hasn't started can be referenced too, through its symbols mutex_lock(&ledger.mutex); - const bool referenced = internal->use_count != 0; + const bool referenced = is_stop_blocked(internal); mutex_unlock(&ledger.mutex); if (referenced) { LOG_W(TAG, "%s can't stop: it is in use", module->name); @@ -159,7 +174,7 @@ error_t module_stop(Module* module) { // Checking and refusing new references is one atomic step, so a module_get() can't slip in afterwards mutex_lock(&ledger.mutex); - if (internal->use_count != 0) { + if (is_stop_blocked(internal)) { mutex_unlock(&ledger.mutex); LOG_W(TAG, "%s can't stop: it is in use", module->name); return ERROR_RESOURCE_BUSY; @@ -266,6 +281,27 @@ void module_put(Module* module) { mutex_unlock(&ledger.mutex); } +error_t module_get_code(Module* module) { + auto* internal = module->internal; + if (internal == nullptr) { return ERROR_INVALID_STATE; } + + mutex_lock(&ledger.mutex); + // Also allowed while the module isn't started: relocated code needs the symbols, not the state + const bool available = internal->added && !internal->stopping; + if (available) { + internal->code_count++; + } + mutex_unlock(&ledger.mutex); + return available ? ERROR_NONE : ERROR_INVALID_STATE; +} + +void module_put_code(Module* module) { + mutex_lock(&ledger.mutex); + check(module->internal != nullptr && module->internal->code_count > 0); + module->internal->code_count--; + mutex_unlock(&ledger.mutex); +} + bool module_resolve_symbol(Module* module, const char* symbol_name, uintptr_t* symbol_address) { if (module->internal == nullptr || !module->internal->added) return false; auto* symbol_ptr = module->symbols; @@ -280,28 +316,29 @@ bool module_resolve_symbol(Module* module, const char* symbol_name, uintptr_t* s return false; } -bool module_resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_address) { - mutex_lock(&ledger.mutex); - for (auto* module : ledger.modules) { - if (module_resolve_symbol(module, symbol_name, symbol_address)) { - mutex_unlock(&ledger.mutex); - return true; - } - } - mutex_unlock(&ledger.mutex); - return false; -} +enum class RefKind { NONE, USE, CODE }; -bool module_resolve_symbol_global_get(const char* symbol_name, uintptr_t* symbol_address, Module** out_module) { +static bool resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_address, Module** out_module, RefKind ref_kind) { mutex_lock(&ledger.mutex); for (auto* module : ledger.modules) { auto* internal = module->internal; - if (internal == nullptr || internal->stopping) + if (internal == nullptr) { continue; + } + // A module that's being stopped may still be resolvable, but no new reference may be taken + if (ref_kind != RefKind::NONE && internal->stopping) { + continue; + } if (module_resolve_symbol(module, symbol_name, symbol_address)) { - internal->use_count++; + if (ref_kind == RefKind::USE) { + internal->use_count++; + } else if (ref_kind == RefKind::CODE) { + internal->code_count++; + } mutex_unlock(&ledger.mutex); - *out_module = module; + if (out_module != nullptr) { + *out_module = module; + } return true; } } @@ -309,4 +346,16 @@ bool module_resolve_symbol_global_get(const char* symbol_name, uintptr_t* symbol return false; } +bool module_resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_address) { + return resolve_symbol_global(symbol_name, symbol_address, nullptr, RefKind::NONE); +} + +bool module_resolve_symbol_global_get(const char* symbol_name, uintptr_t* symbol_address, Module** out_module) { + return resolve_symbol_global(symbol_name, symbol_address, out_module, RefKind::USE); +} + +bool module_resolve_symbol_global_get_code(const char* symbol_name, uintptr_t* symbol_address, Module** out_module) { + return resolve_symbol_global(symbol_name, symbol_address, out_module, RefKind::CODE); +} + } diff --git a/TactilityKernel/source/symbols.c b/TactilityKernel/source/symbols.c index 424f516fe..cf150072d 100644 --- a/TactilityKernel/source/symbols.c +++ b/TactilityKernel/source/symbols.c @@ -620,8 +620,11 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = { DEFINE_MODULE_SYMBOL(module_resolve_symbol), DEFINE_MODULE_SYMBOL(module_resolve_symbol_global), DEFINE_MODULE_SYMBOL(module_resolve_symbol_global_get), + DEFINE_MODULE_SYMBOL(module_resolve_symbol_global_get_code), DEFINE_MODULE_SYMBOL(module_get), DEFINE_MODULE_SYMBOL(module_put), + DEFINE_MODULE_SYMBOL(module_get_code), + DEFINE_MODULE_SYMBOL(module_put_code), // bundle DEFINE_MODULE_SYMBOL(bundle_alloc), DEFINE_MODULE_SYMBOL(bundle_clone), diff --git a/TactilityKernel/tests/source/module_test.cpp b/TactilityKernel/tests/source/module_test.cpp index 29168199b..00e2cc78b 100644 --- a/TactilityKernel/tests/source/module_test.cpp +++ b/TactilityKernel/tests/source/module_test.cpp @@ -229,6 +229,73 @@ TEST_CASE("module_stop is refused while the module is referenced") { CHECK_EQ(module_destruct(&module), ERROR_NONE); } +TEST_CASE("A code reference keeps a module mapped without preventing a stop") { + static const struct ModuleSymbol symbols[] = { + DEFINE_MODULE_SYMBOL(symbol_test_function), + MODULE_SYMBOL_TERMINATOR + }; + start_called = false; + stop_called = false; + test_start_result = ERROR_NONE; + test_stop_result = ERROR_NONE; + struct Module module = { + .name = "code_referenced_test", + .start = test_start, + .stop = test_stop, + .symbols = symbols, + .internal = nullptr + }; + + REQUIRE_EQ(module_construct_add_start(&module), ERROR_NONE); + + uintptr_t address = 0; + struct Module* owner = nullptr; + REQUIRE(module_resolve_symbol_global_get_code("symbol_test_function", &address, &owner)); + CHECK_EQ(owner, &module); + + // Relocated code only needs the symbols, so it doesn't keep the module running + CHECK_EQ(module_stop(&module), ERROR_NONE); + CHECK_EQ(stop_called, true); + + // It does keep the module from being removed or destructed + CHECK_EQ(module_remove(&module), ERROR_RESOURCE_BUSY); + CHECK_EQ(module_ensure_destructed(&module), ERROR_RESOURCE_BUSY); + + // And it can be started again while the code reference is held + CHECK_EQ(module_start(&module), ERROR_NONE); + CHECK_EQ(module_is_started(&module), true); + + module_put_code(owner); + CHECK_EQ(module_ensure_destructed(&module), ERROR_NONE); +} + +TEST_CASE("A code reference prevents stopping a module from a package") { + stop_called = false; + test_start_result = ERROR_NONE; + test_stop_result = ERROR_NONE; + struct Module module = { + .name = "external_code_referenced_test", + .start = test_start, + .stop = test_stop, + .symbols = nullptr, + .internal = nullptr + }; + + REQUIRE_EQ(module_construct_add_start(&module), ERROR_NONE); + REQUIRE_EQ(module_set_external(&module, true), ERROR_NONE); + REQUIRE_EQ(module_get_code(&module), ERROR_NONE); + + // The user can stop package modules from Kernel settings, so an app's use of one must protect it + CHECK_EQ(module_stop(&module), ERROR_RESOURCE_BUSY); + CHECK_EQ(stop_called, false); + + module_put_code(&module); + CHECK_EQ(module_stop(&module), ERROR_NONE); + CHECK_EQ(stop_called, true); + CHECK_EQ(module_remove(&module), ERROR_NONE); + CHECK_EQ(module_destruct(&module), ERROR_NONE); +} + TEST_CASE("module_remove is refused while the module is referenced") { static const struct ModuleSymbol symbols[] = { DEFINE_MODULE_SYMBOL(symbol_test_function),