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

[Petr Mladek <pmladek@suse.com>]

On Wed 2026-04-08 10:40:10, Yafang Shao wrote:
> On Tue, Apr 7, 2026 at 11:08 PM Petr Mladek <pmladek@suse.com> 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@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 7, 2026 at 10:54 AM Song Liu <song@kernel.org> wrote:
> > > > >
> > > > > On Mon, Apr 6, 2026 at 2:12 PM Joe Lawrence <joe.lawrence@redhat.com> 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.

It would be nice to associate shadow variables with states so that
we could check which shadow variables are used by each livepatch.

It is partially implemented in my earlier RFC, see
https://lore.kernel.org/all/20250115082431.5550-3-pmladek@suse.com/


> > > > 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.

And this exactly the weird thing.

A patch with the .replace flag set is supposed to obsolete all already
installed livepatches. It means that it should provide all existing
fixes and features.

Now, we want to introduce a replace flag/set which would allow to
replace/obsolete only the livepatch with the same tag/set number.
And we want to prevent conflicts by making sure that livepatches with
different tag/set number will never livepatch the same function.

Obviously, livepatches with different tag/set number could not
obsolete the same no-replace livepatch. They would need to livepatch
the same functions touched by the no-replace livepatch and would
conflict.

So, I suggest to remove the no-replace mode completely. It should
not be needed. A livepatch which should be installed in parallel
will simply use another unique tag/set number.

> 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.

IMHO, the backward compatibility would just create complexity and mess
in this case.

> > > + *             > 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.

It does. Livepatches with .replace flag set would need to define:

	struct livepatch patch = {
		.replace = <number>,
	}

instead of

	struct livepatch patch = {
		.replace = true,
	}

Best Regards,
Petr