Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Medium] Failing `klp_apply_object_relocs()` triggers spurious architectural warnings due to unconditional clearing of unapplied relocations in the newly added error path. Pre-existing issues: - [High] If `klp_pre_patch_callback()` or `klp_patch_object()` fails during `klp_module_coming()`, the successfully initialized state from `klp_init_object_loaded()` is leaked. The cleanup function `klp_cleanup_module_patches_limited()` intentionally skips the currently failing patch, leaving applied relocations in the livepatch module uncleared and causing subsequent load attempts of the target module to persistently fail. -- commit a90c142b7a6f2c5551f83382ec1fcf15614ffbac Author: Petr Mladek <[email protected]> livepatch: Clean up klp_init_object_loaded() when fails When a module is loaded, klp_module_coming() iterates over patches and calls klp_init_object_loaded(). If initialization fails, it delegates cleanup to klp_cleanup_module_patches_limited(). This commit updates klp_free_object_loaded() to revert all and only changes made by klp_init_object_loaded(). Closes: https://lore.kernel.org/r/[email protected] > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > index a6796cd6b65fd..706a4a7193514 100644 > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c [ ... ] > @@ -916,11 +920,17 @@ static int klp_init_object_loaded(struct klp_patch > *patch, > if (!ret) { > pr_err("kallsyms size lookup failed for '%s' > replacement\n", > func->old_name); > - return -ENOENT; > + ret = -ENOENT; > + goto err; > } > } > > return 0; > + > +err: > + klp_free_object_loaded(patch, obj); > + > + return ret; > } [Severity: Medium] Does this unconditionally clear unapplied relocations? When klp_init_object_loaded() fails (for example, if a symbol lookup fails after klp_apply_object_relocs() succeeds, or if klp_apply_object_relocs() itself fails), this newly added error path calls klp_free_object_loaded(). This unconditionally invokes klp_clear_object_relocs(), which iterates over all .klp.rela sections and attempts to un-apply them via clear_relocate_add(). However, for sections that were never applied, the target memory remains unmodified (zeroed). On architectures like x86, clear_relocate_add() uses memcmp() to verify that the memory contains the expected relocated value. Since it does not, it prints an "Invalid relocation target" warning for every unapplied relocation entry, resulting in significant spurious warning spam in the kernel log. [ ... ] > @@ -1274,8 +1284,8 @@ static void klp_cleanup_module_patches_limited(struct > module *mod, > klp_unpatch_object(obj); > > klp_post_unpatch_callback(obj); > - klp_clear_object_relocs(patch, obj); > - klp_free_object_loaded(obj); > + klp_free_object_loaded(patch, obj); > + obj->mod = NULL; > break; > } > } [Severity: High] This is a pre-existing issue, but does this cleanup logic leak relocations if klp_module_coming() fails after klp_init_object_loaded() succeeds? If klp_pre_patch_callback() or klp_patch_object() fails during klp_module_coming(), the successfully initialized state from klp_init_object_loaded() is leaked. The cleanup function intentionally skips the currently failing patch: kernel/livepatch/core.c:klp_cleanup_module_patches_limited() { ... klp_for_each_patch(patch) { if (patch == limit) break; ... } This prevents its newly applied relocations from being cleared. Since these relocations modify the livepatch module's memory, they persist even after the target module's load attempt is aborted. Upon a subsequent attempt to load the target module, klp_apply_object_relocs() will attempt to apply the relocations again. On architectures like x86, apply_relocate_add() verifies that the target memory is zero; finding it non-zero, it returns -ENOEXEC. This causes klp_module_coming() to fail, permanently preventing the target module from being loaded as long as the livepatch is loaded. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
