Re: [RFC PATCH 1/6] livepatch: Support scoped atomic replace using replace set

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