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

On Tue, May 26, 2026 at 8:52 PM Petr Mladek <[email protected]> wrote:
>
> On Wed 2026-05-13 22:33:16, Yafang Shao wrote:
> > Convert the replace attribute from a boolean to a u32 to function as a
> > "replace set." A newly loaded livepatch will now atomically replace
> > existing patches that belong to the same set.
> >
> > This change currently supports function replacement only; support for
> > state and shadow variables will be introduced in subsequent patches.
> >
> > --- a/Documentation/livepatch/cumulative-patches.rst
> > +++ b/Documentation/livepatch/cumulative-patches.rst
> > @@ -17,18 +17,20 @@ from all older livepatches and completely replace them in one transition.
> >  Usage
> >  -----
> >
> > -The atomic replace can be enabled by setting "replace" flag in struct klp_patch,
> > -for example::
> > +The "replace_set" attribute in ``struct klp_patch`` acts as a **replace set**,
> > +defining the scope of the replacement. By default, the replace set is 1.
>
> Why "1" by default, please?
>
> I guess that you wanted to make it "compatible" with the original
> "replace" flag. It makes some sense. But it is weird in the long term.
>
> This patchset is changing the whole semantic. Every livepatch is able
> to replace an older one. It is not longer "no replace" vs "replace
> all". Instead, a livepatch with a particular "replace_set" number
> replaces an older livepatch with the same "replace_set" number.
>
> It brings the question whether "replace_set" is a good name. There
> is always only one enabled livepatch with a particular "replace_set"
> number. It would make sense to call it "replace_tag" or "replace_id".
>
> That said, the "set" might mean a set of livepatched functions.
> And we should make sure that each set is separate. We should refuse
> loading a livepatch which would patch a function already patched
> by another livepatch with another another "replace_set".
>
> Summary:
>
> I would keep "replace_set" name. But I would use "0" by default.

I will update it.

>
> > +For example::
> >
> >       static struct klp_patch patch = {
> >               .mod = THIS_MODULE,
> >               .objs = objs,
> > -             .replace = true,
> > +             .replace_set = 1,
> >       };
> >
> >  All processes are then migrated to use the code only from the new patch.
> > -Once the transition is finished, all older patches are automatically
> > -disabled.
> > +Once the transition is finished, all older patches with the same replace
> > +set are automatically disabled. Patches with different tags remain active.
> >
> >  Ftrace handlers are transparently removed from functions that are no
> >  longer modified by the new cumulative patch.
> > @@ -62,9 +64,10 @@ Limitations:
> >  ------------
> >
> >    - Once the operation finishes, there is no straightforward way
> > -    to reverse it and restore the replaced patches atomically.
> > +    to reverse it and restore the replaced patches (with the same set)
> > +    atomically.
> >
> > -    A good practice is to set .replace flag in any released livepatch.
> > +    A good practice is to set a consistent .replace set in related livepatches.
>
> I would say something like:
>
>      "A good practice is to use only one (default) "replace_set". It
>      makes sure that there always will be only one enabled livepatch
>      on the system. The consistency model will ensure a safe update
>      between two versions. It prevents potential problems with installing
>      two livepatches doing incompatible functional changes."

Thanks for your suggestion.

>
> >      Then re-adding an older livepatch is equivalent to downgrading
> >      to that patch. This is safe as long as the livepatches do _not_ do
> >      extra modifications in (un)patching callbacks or in the module_init()
> > diff --git a/Documentation/livepatch/livepatch.rst b/Documentation/livepatch/livepatch.rst
> > index acb90164929e..07c8d5a13003 100644
> > --- a/Documentation/livepatch/livepatch.rst
> > +++ b/Documentation/livepatch/livepatch.rst
> > @@ -347,15 +347,20 @@ to '0'.
> >  5.3. Replacing
> >  --------------
> >
> > -All enabled patches might get replaced by a cumulative patch that
> > -has the .replace flag set.
> > -
> > -Once the new patch is enabled and the 'transition' finishes then
> > -all the functions (struct klp_func) associated with the replaced
> > -patches are removed from the corresponding struct klp_ops. Also
> > -the ftrace handler is unregistered and the struct klp_ops is
> > -freed when the related function is not modified by the new patch
> > -and func_stack list becomes empty.
> > +All currently enabled patches may be superseded by a cumulative patch that
>
> In fact, there always can be only one livepatch with a given
> "replace_set" number. They always replace each other.

Thanks for the clarification.

>
> > +has the same ``.replace_set`` attribute. Once the new patch is enabled and
> > +the transition finishes, the livepatching core identifies all existing
> > +patches that share the same replace set.
> > +
> > +Once the transition is complete, all functions (``struct klp_func``)
> > +associated with the matching replaced patches are removed from the
> > +corresponding ``struct klp_ops``. If a function is no longer modified by
> > +the new patch and its ``func_stack`` list becomes empty, the ftrace
> > +handler is unregistered and the ``struct klp_ops`` is freed.
> > +
> > +Patches with a different replace set are not affected by this process
> > +and remain active. This allows for the independent management and
> > +stacking of multiple, non-conflicting livepatch sets.
> >
> >  See Documentation/livepatch/cumulative-patches.rst for more details.
> >
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -454,7 +454,7 @@ static ssize_t replace_show(struct kobject *kobj,
>
> The function should get renamed to replace_set_show()...

sure.

>
> >       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);
> >  }
> >
> >  static ssize_t stack_order_show(struct kobject *kobj,
> > --- a/kernel/livepatch/state.c
> > +++ b/kernel/livepatch/state.c
> > @@ -85,24 +85,25 @@ EXPORT_SYMBOL_GPL(klp_get_prev_state);
> >
> >  /* Check if the patch is able to deal with the existing system state. */
> >  static bool klp_is_state_compatible(struct klp_patch *patch,
> > +                                 struct klp_patch *old_patch,
> >                                   struct klp_state *old_state)
> >  {
> >       struct klp_state *state;
> >
> >       state = klp_get_state(patch, old_state->id);
> >
> > -     /* A cumulative livepatch must handle all already modified states. */
> > +     /*
> > +      * If the new livepatch shares a state set with an existing one, it
> > +      * must maintain compatibility with all states modified by the old
> > +      * patch.
> > +      */
> >       if (!state)
> > -             return !patch->replace;
> > +             return patch->replace_set != old_patch->replace_set;
>
>
> >       return state->version >= old_state->version;
>
> Also I would enforce that two livepatches with a different "replace_set"
> must _not_ use the same "state->id".

I will update it.

>
> >  }
> >
> > -/*
> > - * Check that the new livepatch will not break the existing system states.
> > - * Cumulative patches must handle all already modified states.
> > - * Non-cumulative patches can touch already modified states.
> > - */
> > +/* Check that the new livepatch will not break the existing system states. */
> >  bool klp_is_patch_compatible(struct klp_patch *patch)
> >  {
> >       struct klp_patch *old_patch;
> > @@ -110,7 +111,7 @@ bool klp_is_patch_compatible(struct klp_patch *patch)
> >
> >       klp_for_each_patch(old_patch) {
> >               klp_for_each_state(old_patch, old_state) {
> > -                     if (!klp_is_state_compatible(patch, old_state))
> > +                     if (!klp_is_state_compatible(patch, old_patch, old_state))
> >                               return false;
> >               }
> >       }
>
> In addition, I strictly recommend to compare the set of livepatched
> functions. We should refuse loading a livepatch which would want to modify
> a function which is already livepatched with the livepatch with
> another "replace_set".

I will update it.

>
> Aka, the "set" means a set of livepatched functions. And the sets
> should be independent.
>
> > --- 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)
>
> We still need to skip klp_transition patch as suggested by Sashiko AI.

sure

>
> >                               patch->forced = true;
> >               }
> >       }
> > --- a/scripts/livepatch/init.c
> > +++ b/scripts/livepatch/init.c
> > @@ -72,12 +72,7 @@ static int __init livepatch_mod_init(void)
> >
> >       /* TODO patch->states */
> >
> > -#ifdef KLP_NO_REPLACE
> > -     patch->replace = false;
> > -#else
> > -     patch->replace = true;
> > -#endif
> > -
> > +     patch->replace_set = KLP_REPLACE_TAG;
>
> It should be KLP_REPLACE_SET to keep the naming consistent.

I will fix it.

>
> Is KLP_REPLACE_SET always defined, please?

Right, it is always defined:

  cflags+=("-DKLP_REPLACE_SET=$REPLACE")

>
> >       return klp_enable_patch(patch);
> >
> >  err_free_objs:
> > diff --git a/scripts/livepatch/klp-build b/scripts/livepatch/klp-build
> > index 7b82c7503c2b..66d4a0631f1b 100755
> > --- a/scripts/livepatch/klp-build
> > +++ b/scripts/livepatch/klp-build
> > @@ -172,9 +172,9 @@ process_args() {
> >                               NAME="$(module_name_string "$NAME")"
> >                               shift 2
> >                               ;;
> > -                     --no-replace)
> > -                             REPLACE=0
> > -                             shift
> > +                     -s | --replace-set)
> > +                             REPLACE="$2"
>
> I would rename it to REPLACE_SET.

sure

>
> > +                             shift 2
> >                               ;;
> >                       -v | --verbose)
> >                               VERBOSE="V=1"
> > @@ -759,7 +759,7 @@ build_patch_module() {
> >
> >       cflags=("-ffunction-sections")
> >       cflags+=("-fdata-sections")
> > -     [[ $REPLACE -eq 0 ]] && cflags+=("-DKLP_NO_REPLACE")
> > +     cflags+=("-DKLP_REPLACE_TAG=$REPLACE")
>
> with a consisten naming scheme:
>
>         cflags+=("-DKLP_REPLACE_SET=$REPLACE_SET")
>
> Is there a default value?

The default value is currently 1, but I will update it to 0 as suggested.

  REPLACE=1

>
>
> >       cmd=("make")
> >       cmd+=("$VERBOSE")
>
>
> In general, I am fine with this change. Well, it would require also
> adding/fixing selftests.

I will update it.

>
> That said, I would prefer to rework the klp callbacks, shadow, and
> state API first. But it is not a strict requirement.

I will submit this as a standalone patch, as function replacement
works seamlessly with it.

-- 
Regards
Yafang