Re: [RFC PATCH 3/4] livepatch: Add "replaceable" attribute to klp_patch

[Yafang Shao]

On Tue, Apr 7, 2026 at 11:08 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Tue 2026-04-07 17:45:31, Yafang Shao wrote:
> > On Tue, Apr 7, 2026 at 11:16 AM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> > >
> > > On Tue, Apr 7, 2026 at 10:54 AM Song Liu <song@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Apr 6, 2026 at 2:12 PM Joe Lawrence <joe.lawrence@xxxxxxxxxx> wrote:
> > > > [...]
> > > > > > > > - The regular livepatches are cumulative, have the replace flag; and
> > > > > > > >   are replaceable.
> > > > > > > > - The occasional "off-band" livepatches do not have the replace flag,
> > > > > > > >   and are not replaceable.
> > > > > > > >
> > > > > > > > With this setup, for systems with off-band livepatches loaded, we can
> > > > > > > > still release a cumulative livepatch to replace the previous cumulative
> > > > > > > > livepatch. Is this the expected use case?
> > > > > > >
> > > > > > > That matches our expected use case.
> > > > > >
> > > > > > If we really want to serve use cases like this, I think we can introduce
> > > > > > some replace tag concept: Each livepatch will have a tag, u32 number.
> > > > > > Newly loaded livepatch will only replace existing livepatch with the
> > > > > > same tag. We can even reuse the existing "bool replace" in klp_patch,
> > > > > > and make it u32: replace=0 means no replace; replace > 0 are the
> > > > > > replace tag.
> > > > > >
> > > > > > For current users of cumulative patches, all the livepatch will have the
> > > > > > same tag, say 1. For your use case, you can assign each user a
> > > > > > unique tag. Then all these users can do atomic upgrades of their
> > > > > > own livepatches.
> > > > > >
> > > > > > We may also need to check whether two livepatches of different tags
> > > > > > touch the same kernel function. When that happens, the later
> > > > > > livepatch should fail to load.
>
> I still think how to make the hybrid mode more secure:
>
>     + The isolated sets of livepatched functions look like a good rule.
>     + What about isolating the shadow variables/states as well?

We might consider extending the klp_shadow_* API to support the new
livepatch tag.

>
> > > That sounds like a viable solution. I'll look into it and see how we
> > > can implement it.
> >
> > Does the following change look good to you ?
> >
> > Subject: [PATCH] livepatch: Support scoped atomic replace using replace tags
> >
> > Extend the replace attribute from a boolean to a u32 to act as a replace
> > tag. This introduces the following semantics:
> >
> >   replace = 0: Atomic replace is disabled. However, this patch remains
> >                eligible to be superseded by others.
> >   replace > 0: Enables tagged replace (default is 1). A newly loaded
> >                livepatch will only replace existing patches that share the
> >                same tag.
> >
> > To maintain backward compatibility, a patch with replace == 0 does not
> > trigger an outgoing atomic replace, but remains eligible to be superseded
> > by any incoming patch with a valid replace tag.
> >
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index ba9e3988c07c..417c67a17b99 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -123,7 +123,11 @@ struct klp_state {
> >   * @mod:       reference to the live patch module
> >   * @objs:      object entries for kernel objects to be patched
> >   * @states:    system states that can get modified
> > - * @replace:   replace all actively used patches
> > + * @replace:   replace tag:
> > + *             = 0: Atomic replace is disabled; however, this patch remains
> > + *                  eligible to be superseded by others.
>
> This is weird semantic. Which livepatch tag would be allowed to
> supersede it, please?
>
> Do we still need this category?

It can be superseded by any livepatch that has a non-zero tag set.

This ensures backward compatibility: while a non-atomic-replace
livepatch can be superseded by an atomic-replace one, the reverse is
not permitted—an atomic-replace livepatch cannot be superseded by a
non-atomic one.

>
> > + *             > 0: Atomic replace is enabled. Only existing patches with a
> > + *                  matching replace tag will be superseded.
> >   * @list:      list node for global list of actively used patches
> >   * @kobj:      kobject for sysfs resources
> >   * @obj_list:  dynamic list of the object entries
> > @@ -137,7 +141,7 @@ struct klp_patch {
> >         struct module *mod;
> >         struct klp_object *objs;
> >         struct klp_state *states;
> > -       bool replace;
> > +       unsigned int replace;
>
> This already breaks the backward compatibility

It doesn't break backward compatibility.

> by changing the type
> and semantic of this field. I would also change the name to better
> match the new semantic. What about using:
>
>
>   * @replace_set: Livepatch using the same @replace_set will get
>                 atomically replaced, see also conflicts[*].
>
>         unsigned int replace_set;
>
> [*] A livepatch module, livepatching an already livepatches function,
>     can be loaded only when it has the same @replace_set number.
>
>     By other words, two livepatches conflict when they have a different
>     @replace_set number and have at least one livepatched version
>     in common.

Renaming it to @replace_set makes sense to me.

>
> >
> >         /* internal */
> >         struct list_head list;
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index 28d15ba58a26..e4e5c03b0724 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -793,6 +793,8 @@ void klp_free_replaced_patches_async(struct
> > klp_patch *new_patch)
> >         klp_for_each_patch_safe(old_patch, tmp_patch) {
> >                 if (old_patch == new_patch)
> >                         return;
> > +               if (old_patch->replace && old_patch->replace !=
> > new_patch->replace)
> > +                       continue;
> >                 klp_free_patch_async(old_patch);
> >         }
> >  }
> > @@ -1194,6 +1196,8 @@ void klp_unpatch_replaced_patches(struct
> > klp_patch *new_patch)
> >         klp_for_each_patch(old_patch) {
> >                 if (old_patch == new_patch)
> >                         return;
> > +               if (old_patch->replace && old_patch->replace !=
> > new_patch->replace)
> > +                       continue;
> >
> >                 old_patch->enabled = false;
> >                 klp_unpatch_objects(old_patch);
>
> This handles only the freeing part. More changes will be
> necessary:
>
>    + klp_is_patch_compatible() must check also conflicts
>      between livepatches with different @replace_set.
>      The conflicts might be in the lists of:
>
>         + livepatched functions
>         + state IDs (aka callbacks and shadow variables IDs)
>
>    + klp_add_nops() must skip livepatches with another @replace_set
>
>    + klp_unpatch_replaced_patches() should unpatch only
>      patches with the same @replace_set

I appreciate the guidance on this.

>
> Finally, we would need to update existing selftests
> plus add new selftests.
>
> It is possible that I have missed something.
>
> Anyway, you should wait for more feedback before you do too much
> coding, especially the selftests are not needed at RFC stage.

Sure.

-- 
Regards
Yafang