On Fri, May 15, 2026 at 4:54 AM <[email protected]> wrote:
>
> 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?
As previously discussed, backward compatibility is not a concern.
>
> 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?
Selftests are currently excluded from this RFC
>
> [ ... ]
> > 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?
Should use "%u" instead.
>
> [ ... ]
> > 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?
Right. it should be :
klp_for_each_patch(patch) {
- if (patch != klp_transition_patch)
+ if (patch != klp_transition_patch &&
+ patch->replace_set ==
klp_transition_patch->replace_set)
patch->forced = true;
}
--
Regards
Yafang
