Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Module reference leak during forced transition. - [High] Silent semantic reversion for zero-initialized out-of-tree livepatches. - [High] Sysfs ABI breakage and incorrect type formatting for the `replace` attribute. - [Medium] Build failure in livepatch selftests. --
commit 8a05e8a3ebd9241541926ac6aba19452a820c899 Author: Yafang Shao <[email protected]> livepatch: Support scoped atomic replace using replace set This commit converts the boolean replace attribute to a u32 replace_set to allow livepatches to atomically replace only existing patches within the same set. Patches with different replace sets can now remain active simultaneously. > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h [ ... ] > @@ -137,7 +138,7 @@ struct klp_patch { > struct module *mod; > struct klp_object *objs; > struct klp_state *states; > - bool replace; > + unsigned int replace_set; By removing the boolean replace flag, out-of-tree livepatches that zero-initialize their patch structures will now default to a replace_set of 0. Will this cause them to unintentionally replace all other zero-initialized patches instead of stacking safely? Additionally, does this rename break the livepatch selftests in tools/testing/selftests/livepatch/test_modules/ (such as test_klp_state2.c), which still explicitly set patch.replace = true? [ ... ] > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c > --- a/kernel/livepatch/core.c > +++ b/kernel/livepatch/core.c > @@ -454,7 +454,7 @@ static ssize_t replace_show(struct kobject *kobj, > struct klp_patch *patch; > > patch = container_of(kobj, struct klp_patch, kobj); > - return sysfs_emit(buf, "%d\n", patch->replace); > + return sysfs_emit(buf, "%d\n", patch->replace_set); Does changing the sysfs output from a boolean to a replace set ID break compatibility for userspace tools expecting strict 0 or 1 values from the replace attribute? Also, since replace_set is declared as an unsigned int, will using the signed %d format specifier cause large replace set IDs to overflow and display as negative numbers? [ ... ] > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c > --- a/kernel/livepatch/transition.c > +++ b/kernel/livepatch/transition.c [ ... ] > @@ -720,11 +720,11 @@ void klp_force_transition(void) > klp_update_patch_state(idle_task(cpu)); > > /* Set forced flag for patches being removed. */ > - if (klp_target_state == KLP_TRANSITION_UNPATCHED) > + if (klp_target_state == KLP_TRANSITION_UNPATCHED) { > klp_transition_patch->forced = true; > - else if (klp_transition_patch->replace) { > + } else { > klp_for_each_patch(patch) { > - if (patch != klp_transition_patch) > + if (patch->replace_set == > klp_transition_patch->replace_set) > patch->forced = true; > } > } Could this inadvertently set forced = true on the klp_transition_patch itself? Since klp_transition_patch was already added to the global list during klp_init_patch(), it will match its own replace_set here. If it is marked as forced during a patching transition, wouldn't we leak its module reference in klp_free_patch_finish() when it is cleanly disabled later? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
