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
