On Sun 2026-08-23 06:23:12, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > > Pre-existing issues: > - [High] Initialization failures in klp_init_object_loaded() leave livepatch > module relocations uncleared, leading to state corruption and permanently > preventing target module loading. > -- > > --- Patch [1]: [PATCH v2] livepatch: Reject livepatches with aliased old_func > --- > commit 230a471112d181b2e9810c9877ffebb05a42d61c > Author: Harry Hsu <[email protected]> > > livepatch: Reject livepatches with aliased old_func > > This patch adds a check during livepatch initialization to reject > configurations where multiple functions resolve to the same alias address. > It prevents ambiguity when the redirection logic evaluates the func_stack > in the ftrace handler. > > Link: https://lore.kernel.org/all/[email protected]/ > > > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > > index 28d15ba58a26d..c35cf08c27c86 100644 > > --- a/kernel/livepatch/core.c > > +++ b/kernel/livepatch/core.c > > @@ -866,7 +866,7 @@ static void klp_clear_object_relocs(struct klp_patch > > *patch, > > static int klp_init_object_loaded(struct klp_patch *patch, > > struct klp_object *obj) > > { > > - struct klp_func *func; > > + struct klp_func *func, *prev_func; > > int ret; > > > > if (klp_is_module(obj)) { > [ ... ] > > @@ -888,6 +888,21 @@ static int klp_init_object_loaded(struct klp_patch > > *patch, > > if (ret) > > return ret; > > > > + /* > > + * Aliased symbols share one address, so they would resolve to > > + * the same klp_ops and stack up on a single ops->func_stack, > > + * leaving the redirection ambiguous. Reject the livepatch. > > + */ > > + klp_for_each_func(obj, prev_func) { > > + if (prev_func == func) > > + break; > > + if (prev_func->old_func == func->old_func) { > > + pr_err("'%s' and '%s' resolve to the same > > address, aliased symbols are not supported\n", > > + prev_func->old_name, func->old_name); > > + return -EINVAL; > > [Severity: High] > This is a pre-existing issue, but returning an error here leaves the > module relocations applied earlier in this function uncleared. > > When a module is loaded, klp_module_coming() iterates over patches and > calls klp_init_object_loaded(). If initialization fails, it delegates > cleanup to the error path: > > kernel/livepatch/core.c:klp_module_coming() { > ... > ret = klp_init_object_loaded(patch, obj); > if (ret) { > ... > goto err; > } > ... > err: > ... > klp_cleanup_module_patches_limited(mod, patch); > ... > } > > However, the cleanup loop skips the failing patch entirely: > > kernel/livepatch/core.c:klp_cleanup_module_patches_limited() { > ... > klp_for_each_patch(patch) { > if (patch == limit) > break; > ... > klp_clear_object_relocs(patch, obj); > } > } > > Because it breaks when reaching the limit patch, klp_clear_object_relocs() > is never called for the relocations that were successfully applied at the > start of klp_init_object_loaded() via klp_apply_object_relocs(). > > Could this permanently corrupt the livepatch module's state and prevent > the target module from safely loading again, since the relocations are > never reverted?
This is a good catch. Functions called in klp_module_coming() are supposed to revert their own changes on failure. Or klp_module_coming() is supposed to call the counter-part (pre/post scripts). It seems to work, except for the changes done by klp_init_object_loaded(). I think that it was not important in the original design. The only important revert is the clearing of relocations which was added later. I am working on a patch which would fix it. I want to do it the right way and call also klp_free_object_loaded() and clearing obj->mod separately... It is getting late here. I hope that I'll send it tomorrow... Best Regards, Petr
