[RFC PATCH 0/6] livepatch: Introduce replace set support

[RFC PATCH 0/6] livepatch: Introduce replace set support

[Yafang Shao]

We previously proposed a BPF+livepatch method to enable rapid
experimentation with new kernel features without interrupting production
workloads:

  https://lore.kernel.org/live-patching/20260402092607.96430-1-laoar.shao@gmail.com/

In the resulting discussion, Song and Petr suggested adding a "replace set"
to support scenarios where specific livepatches can be selectively replaced
or skipped.

- Patch #1:
  Adds replace set support for livepatch functions.

- Patch #2~#5:
  Derived from Petr's original patchset:

    https://lore.kernel.org/all/20250115082431.5550-3-pmladek@suse.com/

  All the selftests are not included in this RFC.
  Note: Due to a significant refactor in Patch #5, I have omitted Petr's
  Signed-off-by for that specific patch. Please let me know if this is not
  the preferred approach.

- Patch #6:
  Adds replace set support for the shadow variable API.

Petr Mladek (3):
  livepatch: Add callbacks for introducing and removing states
  livepatch: Allow to handle lifetime of shadow variables using the
    livepatch state
  livepatch: Remove "data" from struct klp_state

Yafang Shao (3):
  livepatch: Support scoped atomic replace using replace set
  livepatch: Remove obsolete per-object callbacks
  livepatch: Support replace_set in shadow variable API

 .../livepatch/cumulative-patches.rst          |  17 +-
 Documentation/livepatch/livepatch.rst         |  23 ++-
 include/linux/livepatch.h                     |  30 ++--
 include/linux/livepatch_external.h            |  62 ++++---
 kernel/livepatch/core.c                       |  51 ++----
 kernel/livepatch/core.h                       |  33 ----
 kernel/livepatch/shadow.c                     |  70 +++++---
 kernel/livepatch/state.c                      | 165 +++++++++++++++++-
 kernel/livepatch/state.h                      |   8 +
 kernel/livepatch/transition.c                 |  29 +--
 scripts/livepatch/init.c                      |   9 +-
 scripts/livepatch/klp-build                   |  14 +-
 tools/include/linux/livepatch_external.h      |  62 ++++---
 tools/objtool/klp-diff.c                      |  16 +-
 14 files changed, 373 insertions(+), 216 deletions(-)

-- 
2.47.3

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

[Yafang Shao]

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.

Suggested-by: Song Liu <song@kernel.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 .../livepatch/cumulative-patches.rst          | 17 ++++++++------
 Documentation/livepatch/livepatch.rst         | 23 +++++++++++--------
 include/linux/livepatch.h                     |  5 ++--
 kernel/livepatch/core.c                       | 16 ++++++++-----
 kernel/livepatch/state.c                      | 17 +++++++-------
 kernel/livepatch/transition.c                 | 10 ++++----
 scripts/livepatch/init.c                      |  7 +-----
 scripts/livepatch/klp-build                   | 14 +++++------
 8 files changed, 59 insertions(+), 50 deletions(-)

diff --git a/Documentation/livepatch/cumulative-patches.rst b/Documentation/livepatch/cumulative-patches.rst
index 1931f318976a..6ef49748110e 100644
--- 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.
+
+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.
     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
+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.
 
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index ba9e3988c07c..171c08328299 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -123,7 +123,8 @@ 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_set:Livepatch using the same @replace_set will get atomically
+ *		replaced.
  * @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 +138,7 @@ struct klp_patch {
 	struct module *mod;
 	struct klp_object *objs;
 	struct klp_state *states;
-	bool replace;
+	unsigned int replace_set;
 
 	/* internal */
 	struct list_head list;
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 28d15ba58a26..9eeded1f9cf0 100644
--- 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);
 }
 
 static ssize_t stack_order_show(struct kobject *kobj,
@@ -621,6 +621,8 @@ static int klp_add_nops(struct klp_patch *patch)
 		klp_for_each_object(old_patch, old_obj) {
 			int err;
 
+			if (patch->replace_set != old_patch->replace_set)
+				continue;
 			err = klp_add_object_nops(patch, old_obj);
 			if (err)
 				return err;
@@ -793,6 +795,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_set != new_patch->replace_set)
+			continue;
 		klp_free_patch_async(old_patch);
 	}
 }
@@ -988,11 +992,9 @@ static int klp_init_patch(struct klp_patch *patch)
 	if (ret)
 		return ret;
 
-	if (patch->replace) {
-		ret = klp_add_nops(patch);
-		if (ret)
-			return ret;
-	}
+	ret = klp_add_nops(patch);
+	if (ret)
+		return ret;
 
 	klp_for_each_object(patch, obj) {
 		ret = klp_init_object(patch, obj);
@@ -1195,6 +1197,8 @@ void klp_unpatch_replaced_patches(struct klp_patch *new_patch)
 		if (old_patch == new_patch)
 			return;
 
+		if (old_patch->replace_set != new_patch->replace_set)
+			continue;
 		old_patch->enabled = false;
 		klp_unpatch_objects(old_patch);
 	}
diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
index 2565d039ade0..a2d223f2bbc0 100644
--- 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;
 }
 
-/*
- * 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;
 		}
 	}
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 2351a19ac2a9..d9f5968fecdc 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -89,7 +89,7 @@ static void klp_complete_transition(void)
 		 klp_transition_patch->mod->name,
 		 klp_target_state == KLP_TRANSITION_PATCHED ? "patching" : "unpatching");
 
-	if (klp_transition_patch->replace && klp_target_state == KLP_TRANSITION_PATCHED) {
+	if (klp_target_state == KLP_TRANSITION_PATCHED) {
 		klp_unpatch_replaced_patches(klp_transition_patch);
 		klp_discard_nops(klp_transition_patch);
 	}
@@ -498,7 +498,7 @@ void klp_try_complete_transition(void)
 	 */
 	if (!patch->enabled)
 		klp_free_patch_async(patch);
-	else if (patch->replace)
+	else
 		klp_free_replaced_patches_async(patch);
 }
 
@@ -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;
 		}
 	}
diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
index f14d8c8fb35f..659db21a5b53 100644
--- 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;
 	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
@@ -117,7 +117,7 @@ Options:
    -f, --show-first-changed	Show address of first changed instruction
    -j, --jobs=<jobs>		Build jobs to run simultaneously [default: $JOBS]
    -o, --output=<file.ko>	Output file [default: livepatch-<patch-name>.ko]
-       --no-replace		Disable livepatch atomic replace
+   -s, --replace-set=<set>	Set the atomic replace set for this livepatch
    -v, --verbose		Pass V=1 to kernel/module builds
 
 Advanced Options:
@@ -142,8 +142,8 @@ process_args() {
 	local long
 	local args
 
-	short="hfj:o:vdS:T"
-	long="help,show-first-changed,jobs:,output:,no-replace,verbose,debug,short-circuit:,keep-tmp"
+	short="hfj:o:s:vdS:T"
+	long="help,show-first-changed,jobs:,output:,replace-set:,verbose,debug,short-circuit:,keep-tmp"
 
 	args=$(getopt --options "$short" --longoptions "$long" -- "$@") || {
 		echo; usage; exit
@@ -172,9 +172,9 @@ process_args() {
 				NAME="$(module_name_string "$NAME")"
 				shift 2
 				;;
-			--no-replace)
-				REPLACE=0
-				shift
+			-s | --replace-set)
+				REPLACE="$2"
+				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")
 
 	cmd=("make")
 	cmd+=("$VERBOSE")
-- 
2.47.3

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

[sashiko-bot]

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 <laoar.shao@gmail.com>

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?

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?

[ ... ]
> 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?

[ ... ]
> 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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=1

[RFC PATCH 2/6] livepatch: Add callbacks for introducing and removing states

[Yafang Shao]

From: Petr Mladek <pmladek@suse.com>

The basic livepatch functionality is to redirect problematic functions
to a fixed or improved variants. In addition, there are two features
helping with more problematic situations:

  + pre_patch(), post_patch(), pre_unpatch(), post_unpatch() callbacks
    might be called before and after the respective transitions.
    For example, post_patch() callback might enable some functionality
    at the end of the transition when the entire system is using
    the new code.

  + Shadow variables allow to add new items into structures or other
    data objects.

The practice has shown that these features were hard to use with the atomic
replace feature. The new livepatch usually just adds more fixes. But it
might also remove problematic ones.

Originally, any version of the livepatch was allowed to replace any older
or newer version of the patch. It was not clear how to handle the extra
features. The new patch did not know whether to run the callbacks or
if the changes were already done by the current livepatch. Or if it has
to revert some changes or free shadow variables whey they would no longer
be supported.

It was even more complicated because only the callbacks from the newly
installed livepatch were called. It means that older livepatch might
not be able to revert changes supported only by newer livepatches.

The above problems were supposed to be solved by adding livepatch
states. Each livepatch might define which states are supported. The states
are versioned. The livepatch core checks if the newly installed livepatch
is able to handle all states used by the currently installed livepatch.

Though the practice has shown that the states API was not easy to use
either. It was not well connected with the callbacks and shadow variables.
The states are per-patch. The callbacks are per-object. The livepatch
does not know about the supported shadow variables at all.

As a first step, new per-state callbacks are introduced:

  + "pre_patch" is called before the livepatch is applied but only when
      the state is new.

      It might be used to allocate some memory. Or it might
      check if the state change is safe on the running system.

      If it fails, the patch will not be enabled.

  + "post_patch" is called after the livepatch is applied but only when
      the state is new.

      It might be used to enable using some functionality provided by
      the livepatch after the entire system is livepatched.

  + "pre_unpatch" is called before the livepatch is disabled or replaced.

      When using the atomic replace, the callback is called only when
      the new livepatch does not support the related state. And it uses
      the implementation from the to-be-replaced livepatch.

      The to-be-replaced livepatch needed the callback to allow disabling
      the livepatch anyway. The new livepatch does not need to know
      anything about the state.

      It might be used to disable some functionality which will no longer
      be supported after the livepatch gets disabled.

  + "post_unpatch" is called after the livepatch was disabled or replaced.
     There are the same rules for the atomic replace replacement as for
     "pre_patch" callback.

     It might be used for freeing some memory or unused shadow variables.

These callbacks are going to replace the existing ones. It would cause
some changes:

   + The new callbacks are not called when a livepatched object is
     loaded or removed later.

     The practice shows that per-object callbacks are not worth
     supporting. In a rare case, when a per-object callback is needed.
     the livepatch might register a custom module notifier.

   + The new callbacks are called only when the state is introduced
     or removed. It does not handle the situation when the newly
     installed livepatch continues using an existing state.

     The practice shows that this is exactly what is needed. In the rare
     case when this is not enough, an extra takeover might be done in
     the module->init() callback.

The per-state callbacks are called in similar code paths as the per-object
ones. Especially, the ordering against the other operations is the same.
Though, there are some obvious and less obvious changes:

  + The per-state callbacks are called for the entire patch instead
    of loaded object. It means that they called outside the for-each-object
    cycle.

  + The per-state callbacks are called when a state is introduced
    or obsoleted. Both variants might happen when the atomic replace
    is used.

  + In __klp_enable_patch(), the per-state callbacks are called before
    the smp_wmb() while the per-object ones are called later.

    The new location makes more sense. The setup of the state should
    be ready before the system processes start being transitioned.

    The per-object callbacks were called after the barrier. They were
    using and already existing for-cycle. Nobody though about the potential
    ordering problem when it was implemented.

Signed-off-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/livepatch.h     |  34 ++++++++
 kernel/livepatch/core.c       |   8 ++
 kernel/livepatch/state.c      | 144 ++++++++++++++++++++++++++++++++++
 kernel/livepatch/state.h      |   8 ++
 kernel/livepatch/transition.c |  14 ++++
 5 files changed, 208 insertions(+)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 171c08328299..f43bf2676597 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -106,15 +106,49 @@ struct klp_object {
 	bool patched;
 };
 
+struct klp_patch;
+struct klp_state;
+
+/**
+ * struct klp_state_callbacks - callbacks manipulating the state
+ * @pre_patch:		 executed only when the state is being enabled
+ *			 before code patching
+ * @post_patch:		 executed only when the state is being enabled
+ *			 after code patching
+ * @pre_unpatch:	 executed only when the state is being disabled
+ *			 before code unpatching
+ * @post_unpatch:	 executed only when the state is being disabled
+ *			 after code unpatching
+ * @pre_patch_succeeded: internal state used by a rollback on error
+ *
+ * All callbacks are optional.
+ *
+ * @pre_patch callback returns 0 on success and an error code otherwise.
+ *
+ * Any error prevents enabling the livepatch. @post_unpatch() callbacks are
+ * then called to rollback @pre_patch callbacks which has already succeeded
+ * before. Also @post_patch callbacks are called for to-be-removed states
+ * to rollback pre_unpatch() callbacks when they were called.
+ */
+struct klp_state_callbacks {
+	int (*pre_patch)(struct klp_patch *patch, struct klp_state *state);
+	void (*post_patch)(struct klp_patch *patch, struct klp_state *state);
+	void (*pre_unpatch)(struct klp_patch *patch, struct klp_state *state);
+	void (*post_unpatch)(struct klp_patch *patch, struct klp_state *state);
+	bool pre_patch_succeeded;
+};
+
 /**
  * struct klp_state - state of the system modified by the livepatch
  * @id:		system state identifier (non-zero)
  * @version:	version of the change
+ * @callbacks:	optional callbacks used when enabling or disabling the state
  * @data:	custom data
  */
 struct klp_state {
 	unsigned long id;
 	unsigned int version;
+	struct klp_state_callbacks callbacks;
 	void *data;
 };
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 9eeded1f9cf0..95c099a8f594 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1019,6 +1019,8 @@ static int __klp_disable_patch(struct klp_patch *patch)
 
 	klp_init_transition(patch, KLP_TRANSITION_UNPATCHED);
 
+	klp_states_pre_unpatch(patch);
+
 	klp_for_each_object(patch, obj)
 		if (obj->patched)
 			klp_pre_unpatch_callback(obj);
@@ -1054,6 +1056,12 @@ static int __klp_enable_patch(struct klp_patch *patch)
 
 	klp_init_transition(patch, KLP_TRANSITION_PATCHED);
 
+	ret = klp_states_pre_patch(patch);
+	if (ret)
+		goto err;
+
+	klp_states_pre_unpatch_replaced(patch);
+
 	/*
 	 * Enforce the order of the func->transition writes in
 	 * klp_init_transition() and the ops->func_stack writes in
diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
index a2d223f2bbc0..a90c24d79084 100644
--- a/kernel/livepatch/state.c
+++ b/kernel/livepatch/state.c
@@ -118,3 +118,147 @@ bool klp_is_patch_compatible(struct klp_patch *patch)
 
 	return true;
 }
+
+static bool is_state_in_other_patches(struct klp_patch *patch,
+				      struct klp_state *state)
+{
+	struct klp_patch *p;
+	struct klp_state *s;
+
+	klp_for_each_patch(p) {
+		if (p == patch)
+			continue;
+
+		klp_for_each_state(p, s) {
+			if (s->id == state->id)
+				return true;
+		}
+	}
+
+	return false;
+}
+
+int klp_states_pre_patch(struct klp_patch *patch)
+{
+	struct klp_state *state;
+
+	klp_for_each_state(patch, state) {
+		if (!is_state_in_other_patches(patch, state) &&
+		    state->callbacks.pre_patch) {
+			int err;
+
+			err = state->callbacks.pre_patch(patch, state);
+			if (err)
+				return err;
+		}
+
+		state->callbacks.pre_patch_succeeded = true;
+	}
+
+	return 0;
+}
+
+void klp_states_post_patch(struct klp_patch *patch)
+{
+	struct klp_state *state;
+
+	klp_for_each_state(patch, state) {
+		if (is_state_in_other_patches(patch, state))
+			continue;
+
+		if (state->callbacks.post_patch)
+			state->callbacks.post_patch(patch, state);
+	}
+}
+
+void klp_states_pre_unpatch(struct klp_patch *patch)
+{
+	struct klp_state *state;
+
+	klp_for_each_state(patch, state) {
+		if (is_state_in_other_patches(patch, state))
+			continue;
+
+		if (state->callbacks.pre_unpatch)
+			state->callbacks.pre_unpatch(patch, state);
+	}
+}
+
+void klp_states_post_unpatch(struct klp_patch *patch)
+{
+	struct klp_state *state;
+
+	klp_for_each_state(patch, state) {
+		if (is_state_in_other_patches(patch, state))
+			continue;
+
+		/*
+		 * This only occurs when a transition is canceled after
+		 * a preparation step failed.
+		 */
+		if (!state->callbacks.pre_patch_succeeded)
+			continue;
+
+		if (state->callbacks.post_unpatch)
+			state->callbacks.post_unpatch(patch, state);
+
+		state->callbacks.pre_patch_succeeded = 0;
+	}
+}
+
+/*
+ * Make it clear when pre_unpatch() callbacks need to be reverted
+ * in case of failure.
+ */
+static bool klp_states_pre_unpatch_replaced_called;
+
+void klp_states_pre_unpatch_replaced(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+
+	/* Make sure that it was cleared at the end of the last transition. */
+	WARN_ON(klp_states_pre_unpatch_replaced_called);
+
+	klp_for_each_patch(old_patch) {
+		if (old_patch->replace_set == patch->replace_set &&
+		    old_patch != patch)
+			klp_states_pre_unpatch(old_patch);
+	}
+
+	klp_states_pre_unpatch_replaced_called = true;
+}
+
+void klp_states_post_unpatch_replaced(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+
+	klp_for_each_patch(old_patch) {
+		if (old_patch->replace_set == patch->replace_set &&
+		    old_patch != patch)
+			klp_states_post_unpatch(old_patch);
+	}
+
+	/* Reset for the next transition. */
+	klp_states_pre_unpatch_replaced_called = false;
+}
+
+void klp_states_post_patch_replaced(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+
+	/*
+	 * This only occurs when a transition is canceled after
+	 * a preparation step failed.
+	 */
+	if (!klp_states_pre_unpatch_replaced_called)
+		return;
+
+	klp_for_each_patch(old_patch) {
+		if (old_patch->replace_set == patch->replace_set &&
+		    old_patch != patch)
+			klp_states_post_patch(old_patch);
+	}
+
+	/* Reset for the next transition. */
+	klp_states_pre_unpatch_replaced_called = false;
+}
diff --git a/kernel/livepatch/state.h b/kernel/livepatch/state.h
index 49d9c16e8762..65c0c2cde04c 100644
--- a/kernel/livepatch/state.h
+++ b/kernel/livepatch/state.h
@@ -5,5 +5,13 @@
 #include <linux/livepatch.h>
 
 bool klp_is_patch_compatible(struct klp_patch *patch);
+int klp_states_pre_patch(struct klp_patch *patch);
+void klp_states_post_patch(struct klp_patch *patch);
+void klp_states_pre_unpatch(struct klp_patch *patch);
+void klp_states_post_unpatch(struct klp_patch *patch);
+
+void klp_states_pre_unpatch_replaced(struct klp_patch *patch);
+void klp_states_post_unpatch_replaced(struct klp_patch *patch);
+void klp_states_post_patch_replaced(struct klp_patch *patch);
 
 #endif /* _LIVEPATCH_STATE_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index d9f5968fecdc..1a2b11be7b5a 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -12,6 +12,7 @@
 #include <linux/static_call.h>
 #include "core.h"
 #include "patch.h"
+#include "state.h"
 #include "transition.h"
 
 #define MAX_STACK_ENTRIES  100
@@ -92,6 +93,7 @@ static void klp_complete_transition(void)
 	if (klp_target_state == KLP_TRANSITION_PATCHED) {
 		klp_unpatch_replaced_patches(klp_transition_patch);
 		klp_discard_nops(klp_transition_patch);
+		klp_states_post_unpatch_replaced(klp_transition_patch);
 	}
 
 	if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
@@ -131,6 +133,18 @@ static void klp_complete_transition(void)
 		task->patch_state = KLP_TRANSITION_IDLE;
 	}
 
+	if (klp_target_state == KLP_TRANSITION_PATCHED) {
+		klp_states_post_patch(klp_transition_patch);
+	} else if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
+		/*
+		 * Re-enable states which should have been replaced but
+		 * the transition was cancelled or reverted.
+		 */
+		klp_states_post_patch_replaced(klp_transition_patch);
+
+		klp_states_post_unpatch(klp_transition_patch);
+	}
+
 	klp_for_each_object(klp_transition_patch, obj) {
 		if (!klp_is_object_loaded(obj))
 			continue;
-- 
2.47.3

[RFC PATCH 3/6] livepatch: Allow to handle lifetime of shadow variables using the livepatch state

[Yafang Shao]

From: Petr Mladek <pmladek@suse.com>

Managing the lifetime of shadow variables becomes challenging when atomic
replace is used. The new patch cannot determine whether a shadow variable
has already been used by a previous live patch or if there is a shadow
variable that is no longer in use.

Shadow variables are typically used alongside callbacks. At a minimum,
the @post_unpatch callback is called to free shadow variables that are
no longer needed. Additionally, @post_patch and @pre_unpatch callbacks
are sometimes used to enable or disable the use of shadow variables.
This is necessary when the shadow variable can only be used when
the entire system is capable of handling it.

The complexity increases when using the atomic replace feature,
as only the callbacks from the new live patch are executed.
Newly created live patches might manage obsolete shadow variables,
ensuring the upgrade functions correctly. However, older live
patches are unaware of shadow variables introduced later, which
could lead to leaks during a downgrade. Additionally, these
leaked variables might retain outdated information, potentially
causing issues if those variables are reused in a subsequent upgrade.

These issues are better addressed with the new callbacks associated
with a live patch state. These callbacks are triggered both when
the states are first introduced and when they become obsolete.
Additionally, the callbacks are invoked from the patch that
originally supported the state, ensuring that even downgrades are
handled safely.

Let’s formalize the process: Associate a shadow variable with a live
patch state by setting the "state.is_shadow" flag and using the same
"id" in both struct klp_shadow and struct klp_state.

The shadow variable will then share the same lifetime as the livepatch
state, allowing obsolete shadow variables to be automatically freed
without requiring an additional callback.

A generic callback will free the shadow variables using
the state->callbacks.shadow_dtor callback, if provided.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h | 15 ++++++++++-----
 kernel/livepatch/state.c  |  3 +++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index f43bf2676597..be4584044cf4 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -109,6 +109,11 @@ struct klp_object {
 struct klp_patch;
 struct klp_state;
 
+typedef int (*klp_shadow_ctor_t)(void *obj,
+				 void *shadow_data,
+				 void *ctor_data);
+typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
+
 /**
  * struct klp_state_callbacks - callbacks manipulating the state
  * @pre_patch:		 executed only when the state is being enabled
@@ -119,6 +124,7 @@ struct klp_state;
  *			 before code unpatching
  * @post_unpatch:	 executed only when the state is being disabled
  *			 after code unpatching
+ * @shadow_dtor:	 destructor for the related shadow variable
  * @pre_patch_succeeded: internal state used by a rollback on error
  *
  * All callbacks are optional.
@@ -135,6 +141,7 @@ struct klp_state_callbacks {
 	void (*post_patch)(struct klp_patch *patch, struct klp_state *state);
 	void (*pre_unpatch)(struct klp_patch *patch, struct klp_state *state);
 	void (*post_unpatch)(struct klp_patch *patch, struct klp_state *state);
+	klp_shadow_dtor_t shadow_dtor;
 	bool pre_patch_succeeded;
 };
 
@@ -143,12 +150,15 @@ struct klp_state_callbacks {
  * @id:		system state identifier (non-zero)
  * @version:	version of the change
  * @callbacks:	optional callbacks used when enabling or disabling the state
+ * @is_shadow:	the state handles lifetime of a shadow variable with
+ *		the same @id
  * @data:	custom data
  */
 struct klp_state {
 	unsigned long id;
 	unsigned int version;
 	struct klp_state_callbacks callbacks;
+	bool is_shadow;
 	void *data;
 };
 
@@ -227,11 +237,6 @@ static inline bool klp_have_reliable_stack(void)
 	       IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
 }
 
-typedef int (*klp_shadow_ctor_t)(void *obj,
-				 void *shadow_data,
-				 void *ctor_data);
-typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
-
 void *klp_shadow_get(void *obj, unsigned long id);
 void *klp_shadow_alloc(void *obj, unsigned long id,
 		       size_t size, gfp_t gfp_flags,
diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
index a90c24d79084..43115e8e8453 100644
--- a/kernel/livepatch/state.c
+++ b/kernel/livepatch/state.c
@@ -202,6 +202,9 @@ void klp_states_post_unpatch(struct klp_patch *patch)
 		if (state->callbacks.post_unpatch)
 			state->callbacks.post_unpatch(patch, state);
 
+		if (state->is_shadow)
+			klp_shadow_free_all(state->id, state->callbacks.shadow_dtor);
+
 		state->callbacks.pre_patch_succeeded = 0;
 	}
 }
-- 
2.47.3

Re: [RFC PATCH 3/6] livepatch: Allow to handle lifetime of shadow variables using the livepatch state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Shadow variables and state are permanently leaked during an atomic replace if multiple replaced patches share the same state.
- [High] Tying shadow variable lifetime to the patch's global state causes shadow variable leaks and use-after-free risks for loadable module objects.
- [High] The new `shadow_dtor` callback in `struct klp_state_callbacks` executes in atomic context, creating a severe API trap that leads to kernel panics.
--

commit e19c79edd4d439e568084dd175307ac754ab4007
Author: Petr Mladek <pmladek@suse.com>

livepatch: Allow to handle lifetime of shadow variables using the livepatch state

This commit introduces a new mechanism to associate the lifetime of
shadow variables with the livepatch state. Obsolete shadow variables
are automatically freed during state unpatching using a new shadow_dtor
callback, reducing manual cleanup overhead.

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 248e1a388818e..fef0c2f970838 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
[ ... ]
> @@ -135,6 +141,7 @@ struct klp_state_callbacks {
>  	void (*post_patch)(struct klp_patch *patch, struct klp_state *state);
>  	void (*pre_unpatch)(struct klp_patch *patch, struct klp_state *state);
>  	void (*post_unpatch)(struct klp_patch *patch, struct klp_state *state);
> +	klp_shadow_dtor_t shadow_dtor;

Will this callback run in an atomic context, unlike the other state callbacks?

All existing state callbacks execute in standard process context and are
permitted to sleep. However, looking at klp_shadow_free_all(), it invokes
the destructor while holding klp_shadow_lock:

kernel/livepatch/shadow.c:klp_shadow_free_all() {
	...
	spin_lock_irqsave(&klp_shadow_lock, flags);

	hash_for_each(klp_shadow_hash, i, shadow, node) {
		if (klp_shadow_match(shadow, shadow->obj, id))
			klp_shadow_free_struct(shadow, dtor);
	}

	spin_unlock_irqrestore(&klp_shadow_lock, flags);
}

If a user assumes this callback runs in process context like the other
state callbacks and includes sleeping operations, couldn't this trigger
a scheduling while atomic panic?

>  	bool pre_patch_succeeded;
>  };
>  
> @@ -143,12 +150,15 @@ struct klp_state_callbacks {
>   * @id:		system state identifier (non-zero)
>   * @version:	version of the change
>   * @callbacks:	optional callbacks used when enabling or disabling the state
> + * @is_shadow:	the state handles lifetime of a shadow variable with
> + *		the same @id

Does tying shadow variable lifetime to the global patch state cause leaks when
dealing with loadable modules?

If a livepatch applies to a loadable module, it may allocate shadow variables
keyed to the module's dynamically allocated objects.

When the target module is unloaded, klp_module_going() unpatches the module's
objects. However, because the state is a global property of the patch, no
state callbacks are invoked and the state is not considered obsolete. This
leaves the shadow variables for the unloaded module orphaned in the global
hash table.

If the module is subsequently reloaded and allocates new objects at the same
memory addresses, wouldn't klp_shadow_alloc() fail with a duplicate warning,
or klp_shadow_get() incorrectly return stale data?

>   * @data:	custom data
>   */
[ ... ]
> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> index a90c24d79084b..43115e8e8453c 100644
> --- a/kernel/livepatch/state.c
> +++ b/kernel/livepatch/state.c
> @@ -202,6 +202,9 @@ void klp_states_post_unpatch(struct klp_patch *patch)
>  		if (state->callbacks.post_unpatch)
>  			state->callbacks.post_unpatch(patch, state);
>  
> +		if (state->is_shadow)
> +			klp_shadow_free_all(state->id, state->callbacks.shadow_dtor);
> +

Can this permanently leak shadow variables and state during an atomic replace?

Livepatch supports stacking, allowing multiple active patches (e.g., Patch A
and Patch B) to define the same state. If an atomic replace patch (Patch C)
is loaded, it replaces both A and B.

During the transition completion, klp_complete_transition() calls
klp_states_post_unpatch_replaced(), which iterates over the replaced
patches and calls klp_states_post_unpatch() for each.

For Patch A, klp_states_post_unpatch() calls is_state_in_other_patches().
Since Patch B is also still in the klp_patches list at this point (they are
not removed until klp_free_replaced_patches_async() runs later), the function
returns true and skips the cleanup block.

For Patch B, the same check finds Patch A in the list, returns true, and
also skips cleanup. Both patches are subsequently removed from the list,
permanently leaking the shadow variables and state cleanup logic.

>  		state->callbacks.pre_patch_succeeded = 0;
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=3

[RFC PATCH 4/6] livepatch: Remove "data" from struct klp_state

[Yafang Shao]

From: Petr Mladek <pmladek@suse.com>

The "data" pointer in "struct klp_state" is associated with the lifetime of
the livepatch module, not the livepatch state. This means it's lost when a
livepatch is replaced, even if the new livepatch supports the same state.

Shadow variables provide a more reliable way to attach data to a livepatch
state. Their lifetime can be tied to the state's lifetime by:

- Sharing the same "id"
- Setting "is_shadow" in "struct klp_state"

Removing the "data" pointer prevents potential issues once per-object
callbacks are removed, as it cannot be used securely in that context.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/livepatch.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index be4584044cf4..340b04a0de83 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -152,14 +152,12 @@ struct klp_state_callbacks {
  * @callbacks:	optional callbacks used when enabling or disabling the state
  * @is_shadow:	the state handles lifetime of a shadow variable with
  *		the same @id
- * @data:	custom data
  */
 struct klp_state {
 	unsigned long id;
 	unsigned int version;
 	struct klp_state_callbacks callbacks;
 	bool is_shadow;
-	void *data;
 };
 
 /**
-- 
2.47.3

Re: [RFC PATCH 4/6] livepatch: Remove "data" from struct klp_state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing `data` from `struct klp_state` breaks compilation of the livepatch selftests and leaves kernel documentation explicitly referencing a non-existent API.
--

commit 792ddcf269c47d4de395b3873cfd5f02cbf282ef
Author: Petr Mladek <pmladek@suse.com>

livepatch: Remove "data" from struct klp_state

The commit removes the data pointer from struct klp_state since its lifetime
is tied to the module rather than the state. This pointer is lost during a
livepatch replacement, so shadow variables are recommended instead.

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index fef0c2f970838..56b7e4fe01bc8 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -152,14 +152,12 @@ struct klp_state_callbacks {
>   * @callbacks:	optional callbacks used when enabling or disabling the state
>   * @is_shadow:	the state handles lifetime of a shadow variable with
>   *		the same @id
> - * @data:	custom data
>   */
>  struct klp_state {
>  	unsigned long id;
>  	unsigned int version;
>  	struct klp_state_callbacks callbacks;
>  	bool is_shadow;
> -	void *data;

Does this code removal cause a regression by breaking the compilation of
livepatch selftests?

For example, test_klp_state.c and test_klp_state2.c actively use this field
to store and transfer state.

In tools/testing/selftests/livepatch/test_modules/test_klp_state.c:

allocate_loglevel_state() {
	...
	loglevel_state->data = kzalloc(sizeof(console_loglevel), GFP_KERNEL);
	if (!loglevel_state->data)
		return -ENOMEM;
	...
}

Additionally, does Documentation/livepatch/system-state.rst still reference
this non-existent API? It continues to provide instructions like:

In Documentation/livepatch/system-state.rst:
  - Allocate *state->data* when necessary. The allocation might fail
  ...
  - Make sure that *state->data* has all necessary information.

Should the selftests and documentation be updated as part of this change to
prevent build failures and confusion?

>  };
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=4

[RFC PATCH 5/6] livepatch: Remove obsolete per-object callbacks

[Yafang Shao]

This commit removes the obsolete per-object callbacks from the livepatch
framework.  All selftests have been migrated to the new per-state
callbacks, making the per-object callbacks redundant.

Instead, use the new per-state callbacks. They offer improved semantics
by associating callbacks and shadow variables with a specific state,
enabling better lifetime management of changes.

Originally-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/livepatch.h                | 40 ---------------
 include/linux/livepatch_external.h       | 62 +++++++++++++++---------
 kernel/livepatch/core.c                  | 29 -----------
 kernel/livepatch/core.h                  | 33 -------------
 kernel/livepatch/transition.c            |  9 ----
 scripts/livepatch/init.c                 |  2 -
 tools/include/linux/livepatch_external.h | 62 +++++++++++++++---------
 tools/objtool/klp-diff.c                 | 16 +++---
 8 files changed, 84 insertions(+), 169 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 340b04a0de83..221f176f1f51 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -95,7 +95,6 @@ struct klp_object {
 	/* external */
 	const char *name;
 	struct klp_func *funcs;
-	struct klp_callbacks callbacks;
 
 	/* internal */
 	struct kobject kobj;
@@ -106,45 +105,6 @@ struct klp_object {
 	bool patched;
 };
 
-struct klp_patch;
-struct klp_state;
-
-typedef int (*klp_shadow_ctor_t)(void *obj,
-				 void *shadow_data,
-				 void *ctor_data);
-typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
-
-/**
- * struct klp_state_callbacks - callbacks manipulating the state
- * @pre_patch:		 executed only when the state is being enabled
- *			 before code patching
- * @post_patch:		 executed only when the state is being enabled
- *			 after code patching
- * @pre_unpatch:	 executed only when the state is being disabled
- *			 before code unpatching
- * @post_unpatch:	 executed only when the state is being disabled
- *			 after code unpatching
- * @shadow_dtor:	 destructor for the related shadow variable
- * @pre_patch_succeeded: internal state used by a rollback on error
- *
- * All callbacks are optional.
- *
- * @pre_patch callback returns 0 on success and an error code otherwise.
- *
- * Any error prevents enabling the livepatch. @post_unpatch() callbacks are
- * then called to rollback @pre_patch callbacks which has already succeeded
- * before. Also @post_patch callbacks are called for to-be-removed states
- * to rollback pre_unpatch() callbacks when they were called.
- */
-struct klp_state_callbacks {
-	int (*pre_patch)(struct klp_patch *patch, struct klp_state *state);
-	void (*post_patch)(struct klp_patch *patch, struct klp_state *state);
-	void (*pre_unpatch)(struct klp_patch *patch, struct klp_state *state);
-	void (*post_unpatch)(struct klp_patch *patch, struct klp_state *state);
-	klp_shadow_dtor_t shadow_dtor;
-	bool pre_patch_succeeded;
-};
-
 /**
  * struct klp_state - state of the system modified by the livepatch
  * @id:		system state identifier (non-zero)
diff --git a/include/linux/livepatch_external.h b/include/linux/livepatch_external.h
index 138af19b0f5c..d9123d0c5dff 100644
--- a/include/linux/livepatch_external.h
+++ b/include/linux/livepatch_external.h
@@ -21,33 +21,48 @@
 #define KLP_PRE_UNPATCH_PREFIX		__stringify(__KLP_PRE_UNPATCH_PREFIX)
 #define KLP_POST_UNPATCH_PREFIX		__stringify(__KLP_POST_UNPATCH_PREFIX)
 
-struct klp_object;
-
-typedef int (*klp_pre_patch_t)(struct klp_object *obj);
-typedef void (*klp_post_patch_t)(struct klp_object *obj);
-typedef void (*klp_pre_unpatch_t)(struct klp_object *obj);
-typedef void (*klp_post_unpatch_t)(struct klp_object *obj);
+struct klp_state;
+struct klp_patch;
+typedef int (*klp_shadow_ctor_t)(void *obj,
+				 void *shadow_data,
+				 void *ctor_data);
+typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
 
 /**
- * struct klp_callbacks - pre/post live-(un)patch callback structure
- * @pre_patch:		executed before code patching
- * @post_patch:		executed after code patching
- * @pre_unpatch:	executed before code unpatching
- * @post_unpatch:	executed after code unpatching
- * @post_unpatch_enabled:	flag indicating if post-unpatch callback
- *				should run
+ * struct klp_state_callbacks - callbacks manipulating the state
+ * @pre_patch:		 executed only when the state is being enabled
+ *			 before code patching
+ * @post_patch:		 executed only when the state is being enabled
+ *			 after code patching
+ * @pre_unpatch:	 executed only when the state is being disabled
+ *			 before code unpatching
+ * @post_unpatch:	 executed only when the state is being disabled
+ *			 after code unpatching
+ * @shadow_dtor:	 destructor for the related shadow variable
+ * @pre_patch_succeeded: internal state used by a rollback on error
+ *
+ * All callbacks are optional.
+ *
+ * @pre_patch callback returns 0 on success and an error code otherwise.
  *
- * All callbacks are optional.  Only the pre-patch callback, if provided,
- * will be unconditionally executed.  If the parent klp_object fails to
- * patch for any reason, including a non-zero error status returned from
- * the pre-patch callback, no further callbacks will be executed.
+ * Any error prevents enabling the livepatch. @post_unpatch() callbacks are
+ * then called to rollback @pre_patch callbacks which has already succeeded
+ * before. Also @post_patch callbacks are called for to-be-removed states
+ * to rollback pre_unpatch() callbacks when they were called.
  */
-struct klp_callbacks {
-	klp_pre_patch_t		pre_patch;
-	klp_post_patch_t	post_patch;
-	klp_pre_unpatch_t	pre_unpatch;
-	klp_post_unpatch_t	post_unpatch;
-	bool post_unpatch_enabled;
+struct klp_state_callbacks {
+	int (*pre_patch)(struct klp_patch *patch, struct klp_state *state);
+	void (*post_patch)(struct klp_patch *patch, struct klp_state *state);
+	void (*pre_unpatch)(struct klp_patch *patch, struct klp_state *state);
+	void (*post_unpatch)(struct klp_patch *patch, struct klp_state *state);
+	klp_shadow_dtor_t shadow_dtor;
+	bool pre_patch_succeeded;
+};
+
+struct klp_state_ext {
+	unsigned long id;
+	unsigned int version;
+	struct klp_state_callbacks callbacks;
 };
 
 /*
@@ -69,7 +84,6 @@ struct klp_func_ext {
 struct klp_object_ext {
 	const char *name;
 	struct klp_func_ext *funcs;
-	struct klp_callbacks callbacks;
 	unsigned int nr_funcs;
 };
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 95c099a8f594..eae807916ca0 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -1009,8 +1009,6 @@ static int klp_init_patch(struct klp_patch *patch)
 
 static int __klp_disable_patch(struct klp_patch *patch)
 {
-	struct klp_object *obj;
-
 	if (WARN_ON(!patch->enabled))
 		return -EINVAL;
 
@@ -1021,10 +1019,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
 
 	klp_states_pre_unpatch(patch);
 
-	klp_for_each_object(patch, obj)
-		if (obj->patched)
-			klp_pre_unpatch_callback(obj);
-
 	/*
 	 * Enforce the order of the func->transition writes in
 	 * klp_init_transition() and the TIF_PATCH_PENDING writes in
@@ -1075,13 +1069,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
 		if (!klp_is_object_loaded(obj))
 			continue;
 
-		ret = klp_pre_patch_callback(obj);
-		if (ret) {
-			pr_warn("pre-patch callback failed for object '%s'\n",
-				klp_is_module(obj) ? obj->name : "vmlinux");
-			goto err;
-		}
-
 		ret = klp_patch_object(obj);
 		if (ret) {
 			pr_warn("failed to patch object '%s'\n",
@@ -1253,14 +1240,10 @@ static void klp_cleanup_module_patches_limited(struct module *mod,
 			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
 				continue;
 
-			if (patch != klp_transition_patch)
-				klp_pre_unpatch_callback(obj);
-
 			pr_notice("reverting patch '%s' on unloading module '%s'\n",
 				  patch->mod->name, obj->mod->name);
 			klp_unpatch_object(obj);
 
-			klp_post_unpatch_callback(obj);
 			klp_clear_object_relocs(patch, obj);
 			klp_free_object_loaded(obj);
 			break;
@@ -1307,25 +1290,13 @@ int klp_module_coming(struct module *mod)
 			pr_notice("applying patch '%s' to loading module '%s'\n",
 				  patch->mod->name, obj->mod->name);
 
-			ret = klp_pre_patch_callback(obj);
-			if (ret) {
-				pr_warn("pre-patch callback failed for object '%s'\n",
-					obj->name);
-				goto err;
-			}
-
 			ret = klp_patch_object(obj);
 			if (ret) {
 				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
 					patch->mod->name, obj->mod->name, ret);
-
-				klp_post_unpatch_callback(obj);
 				goto err;
 			}
 
-			if (patch != klp_transition_patch)
-				klp_post_patch_callback(obj);
-
 			break;
 		}
 	}
diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h
index 38209c7361b6..02b8364f6779 100644
--- a/kernel/livepatch/core.h
+++ b/kernel/livepatch/core.h
@@ -23,37 +23,4 @@ static inline bool klp_is_object_loaded(struct klp_object *obj)
 	return !obj->name || obj->mod;
 }
 
-static inline int klp_pre_patch_callback(struct klp_object *obj)
-{
-	int ret = 0;
-
-	if (obj->callbacks.pre_patch)
-		ret = (*obj->callbacks.pre_patch)(obj);
-
-	obj->callbacks.post_unpatch_enabled = !ret;
-
-	return ret;
-}
-
-static inline void klp_post_patch_callback(struct klp_object *obj)
-{
-	if (obj->callbacks.post_patch)
-		(*obj->callbacks.post_patch)(obj);
-}
-
-static inline void klp_pre_unpatch_callback(struct klp_object *obj)
-{
-	if (obj->callbacks.pre_unpatch)
-		(*obj->callbacks.pre_unpatch)(obj);
-}
-
-static inline void klp_post_unpatch_callback(struct klp_object *obj)
-{
-	if (obj->callbacks.post_unpatch_enabled &&
-	    obj->callbacks.post_unpatch)
-		(*obj->callbacks.post_unpatch)(obj);
-
-	obj->callbacks.post_unpatch_enabled = false;
-}
-
 #endif /* _LIVEPATCH_CORE_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 1a2b11be7b5a..f844283b5423 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -145,15 +145,6 @@ static void klp_complete_transition(void)
 		klp_states_post_unpatch(klp_transition_patch);
 	}
 
-	klp_for_each_object(klp_transition_patch, obj) {
-		if (!klp_is_object_loaded(obj))
-			continue;
-		if (klp_target_state == KLP_TRANSITION_PATCHED)
-			klp_post_patch_callback(obj);
-		else if (klp_target_state == KLP_TRANSITION_UNPATCHED)
-			klp_post_unpatch_callback(obj);
-	}
-
 	pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
 		  klp_target_state == KLP_TRANSITION_PATCHED ? "patching" : "unpatching");
 
diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
index 659db21a5b53..04e8d20bab2a 100644
--- a/scripts/livepatch/init.c
+++ b/scripts/livepatch/init.c
@@ -63,8 +63,6 @@ static int __init livepatch_mod_init(void)
 
 		obj->name = obj_ext->name;
 		obj->funcs = funcs;
-
-		memcpy(&obj->callbacks, &obj_ext->callbacks, sizeof(struct klp_callbacks));
 	}
 
 	patch->mod = THIS_MODULE;
diff --git a/tools/include/linux/livepatch_external.h b/tools/include/linux/livepatch_external.h
index 138af19b0f5c..d9123d0c5dff 100644
--- a/tools/include/linux/livepatch_external.h
+++ b/tools/include/linux/livepatch_external.h
@@ -21,33 +21,48 @@
 #define KLP_PRE_UNPATCH_PREFIX		__stringify(__KLP_PRE_UNPATCH_PREFIX)
 #define KLP_POST_UNPATCH_PREFIX		__stringify(__KLP_POST_UNPATCH_PREFIX)
 
-struct klp_object;
-
-typedef int (*klp_pre_patch_t)(struct klp_object *obj);
-typedef void (*klp_post_patch_t)(struct klp_object *obj);
-typedef void (*klp_pre_unpatch_t)(struct klp_object *obj);
-typedef void (*klp_post_unpatch_t)(struct klp_object *obj);
+struct klp_state;
+struct klp_patch;
+typedef int (*klp_shadow_ctor_t)(void *obj,
+				 void *shadow_data,
+				 void *ctor_data);
+typedef void (*klp_shadow_dtor_t)(void *obj, void *shadow_data);
 
 /**
- * struct klp_callbacks - pre/post live-(un)patch callback structure
- * @pre_patch:		executed before code patching
- * @post_patch:		executed after code patching
- * @pre_unpatch:	executed before code unpatching
- * @post_unpatch:	executed after code unpatching
- * @post_unpatch_enabled:	flag indicating if post-unpatch callback
- *				should run
+ * struct klp_state_callbacks - callbacks manipulating the state
+ * @pre_patch:		 executed only when the state is being enabled
+ *			 before code patching
+ * @post_patch:		 executed only when the state is being enabled
+ *			 after code patching
+ * @pre_unpatch:	 executed only when the state is being disabled
+ *			 before code unpatching
+ * @post_unpatch:	 executed only when the state is being disabled
+ *			 after code unpatching
+ * @shadow_dtor:	 destructor for the related shadow variable
+ * @pre_patch_succeeded: internal state used by a rollback on error
+ *
+ * All callbacks are optional.
+ *
+ * @pre_patch callback returns 0 on success and an error code otherwise.
  *
- * All callbacks are optional.  Only the pre-patch callback, if provided,
- * will be unconditionally executed.  If the parent klp_object fails to
- * patch for any reason, including a non-zero error status returned from
- * the pre-patch callback, no further callbacks will be executed.
+ * Any error prevents enabling the livepatch. @post_unpatch() callbacks are
+ * then called to rollback @pre_patch callbacks which has already succeeded
+ * before. Also @post_patch callbacks are called for to-be-removed states
+ * to rollback pre_unpatch() callbacks when they were called.
  */
-struct klp_callbacks {
-	klp_pre_patch_t		pre_patch;
-	klp_post_patch_t	post_patch;
-	klp_pre_unpatch_t	pre_unpatch;
-	klp_post_unpatch_t	post_unpatch;
-	bool post_unpatch_enabled;
+struct klp_state_callbacks {
+	int (*pre_patch)(struct klp_patch *patch, struct klp_state *state);
+	void (*post_patch)(struct klp_patch *patch, struct klp_state *state);
+	void (*pre_unpatch)(struct klp_patch *patch, struct klp_state *state);
+	void (*post_unpatch)(struct klp_patch *patch, struct klp_state *state);
+	klp_shadow_dtor_t shadow_dtor;
+	bool pre_patch_succeeded;
+};
+
+struct klp_state_ext {
+	unsigned long id;
+	unsigned int version;
+	struct klp_state_callbacks callbacks;
 };
 
 /*
@@ -69,7 +84,6 @@ struct klp_func_ext {
 struct klp_object_ext {
 	const char *name;
 	struct klp_func_ext *funcs;
-	struct klp_callbacks callbacks;
 	unsigned int nr_funcs;
 };
 
diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
index c2c4e4968bc2..128fbe054417 100644
--- a/tools/objtool/klp-diff.c
+++ b/tools/objtool/klp-diff.c
@@ -1606,8 +1606,8 @@ static int create_klp_sections(struct elfs *e)
 		reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
 
 		if (!elf_create_reloc(e->out, obj_sec,
-				      offsetof(struct klp_object_ext, callbacks) +
-				      offsetof(struct klp_callbacks, pre_patch),
+				      offsetof(struct klp_state_ext, callbacks) +
+				      offsetof(struct klp_state_callbacks, pre_patch),
 				      reloc->sym, reloc_addend(reloc), R_ABS64))
 			return -1;
 	}
@@ -1622,8 +1622,8 @@ static int create_klp_sections(struct elfs *e)
 		reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
 
 		if (!elf_create_reloc(e->out, obj_sec,
-				      offsetof(struct klp_object_ext, callbacks) +
-				      offsetof(struct klp_callbacks, post_patch),
+				      offsetof(struct klp_state_ext, callbacks) +
+				      offsetof(struct klp_state_callbacks, post_patch),
 				      reloc->sym, reloc_addend(reloc), R_ABS64))
 			return -1;
 	}
@@ -1638,8 +1638,8 @@ static int create_klp_sections(struct elfs *e)
 		reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
 
 		if (!elf_create_reloc(e->out, obj_sec,
-				      offsetof(struct klp_object_ext, callbacks) +
-				      offsetof(struct klp_callbacks, pre_unpatch),
+				      offsetof(struct klp_state_ext, callbacks) +
+				      offsetof(struct klp_state_callbacks, pre_unpatch),
 				      reloc->sym, reloc_addend(reloc), R_ABS64))
 			return -1;
 	}
@@ -1654,8 +1654,8 @@ static int create_klp_sections(struct elfs *e)
 		reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
 
 		if (!elf_create_reloc(e->out, obj_sec,
-				      offsetof(struct klp_object_ext, callbacks) +
-				      offsetof(struct klp_callbacks, post_unpatch),
+				      offsetof(struct klp_state_ext, callbacks) +
+				      offsetof(struct klp_state_callbacks, post_unpatch),
 				      reloc->sym, reloc_addend(reloc), R_ABS64))
 			return -1;
 	}
-- 
2.47.3

Re: [RFC PATCH 5/6] livepatch: Remove obsolete per-object callbacks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Out-of-bounds ELF relocations causing memory corruption
- [High] Silent failure of livepatch state callbacks due to missing initialization
- [Low] Build failures due to unmigrated selftests and samples
--

commit deb761bd65e9a96a95798282bde72ddb362e1c0d
Author: Yafang Shao <laoar.shao@gmail.com>

livepatch: Remove obsolete per-object callbacks

This commit removes obsolete per-object callbacks from the livepatch framework
in favor of per-state callbacks. It drops the callback fields from the klp
object structures and updates objtool and the livepatch core scripts.

> All selftests have been migrated to the new per-state callbacks, making the
> per-object callbacks redundant.

Is this description accurate?

The callbacks field was removed from struct klp_object, but if modules like
samples/livepatch/livepatch-callbacks-demo.c or the selftests still use it
in their klp_object declarations, won't this cause build failures?

> diff --git a/scripts/livepatch/init.c b/scripts/livepatch/init.c
> index 9265e0e504e0c..570366b9268c4 100644
> --- a/scripts/livepatch/init.c
> +++ b/scripts/livepatch/init.c
> @@ -63,8 +63,6 @@ static int __init livepatch_mod_init(void)
>  
>  		obj->name = obj_ext->name;
>  		obj->funcs = funcs;
> -
> -		memcpy(&obj->callbacks, &obj_ext->callbacks, sizeof(struct klp_callbacks));
>  	}
>  
>  	patch->mod = THIS_MODULE;

Will the state callbacks be executed when the patch is enabled?

Looking at livepatch_mod_init() after these changes, patch->states is never
initialized:

scripts/livepatch/init.c:livepatch_mod_init() {
        ...
        patch->mod = THIS_MODULE;
        patch->objs = objs;

        /* TODO patch->states */

        patch->replace_set = KLP_REPLACE_TAG;
        return klp_enable_patch(patch);
}

When klp_states_pre_patch() iterates over patch->states, won't it silently
skip the uninitialized callbacks?

> diff --git a/tools/objtool/klp-diff.c b/tools/objtool/klp-diff.c
> index 4d1f9e9977eb9..6e2762de3d512 100644
> --- a/tools/objtool/klp-diff.c
> +++ b/tools/objtool/klp-diff.c
[ ... ]
> @@ -1566,8 +1566,8 @@ static int create_klp_sections(struct elfs *e)
>  		reloc = find_reloc_by_dest(e->out, sym->sec, sym->offset);
>  
>  		if (!elf_create_reloc(e->out, obj_sec,
> -				      offsetof(struct klp_object_ext, callbacks) +
> -				      offsetof(struct klp_callbacks, pre_patch),
> +				      offsetof(struct klp_state_ext, callbacks) +
> +				      offsetof(struct klp_state_callbacks, pre_patch),
>  				      reloc->sym, reloc_addend(reloc), R_ABS64))
>  			return -1;
>  	}

Does this result in out-of-bounds memory writes during ELF relocation?

Earlier in create_klp_sections(), obj_sec is allocated using the size of
struct klp_object_ext, which is 24 bytes:

tools/objtool/klp-diff.c:create_klp_sections() {
	size_t obj_size  = sizeof(struct klp_object_ext);
	...
	obj_sec  = elf_create_section_pair(e->out, KLP_OBJECTS_SEC, obj_size, 0, 0);

However, the new relocations are created using offsets from a completely
different structure, struct klp_state_ext. 

Because offsetof(struct klp_state_ext, callbacks) is 16, and the callback
offsets go up to 24, this applies relocations at offsets 16, 24, 32, and 40
within obj_sec.

Won't the relocation at offset 16 corrupt klp_object_ext.nr_funcs, and the
subsequent relocations write entirely out of bounds of the 24-byte obj_sec
buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=5

[RFC PATCH 6/6] livepatch: Support replace_set in shadow variable API

[Yafang Shao]

To support more complex livepatching scenarios where multiple
replacement sets might coexist, extend the klp_shadow API to
include a 'replace_set' identifier.

To maintain compatibility with the existing 64-bit storage in
'struct klp_shadow', the internal @id is now treated as a composite
value. The 64-bit identifier is constructed by packing two 32-bit
values:

  MSB (63-32)          LSB (31-0)
  +--------------------+--------------------+
  |    replace_set     |    original @id    |
  +--------------------+--------------------+

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 include/linux/livepatch.h | 12 ++++---
 kernel/livepatch/shadow.c | 70 ++++++++++++++++++++++++---------------
 kernel/livepatch/state.c  |  3 +-
 3 files changed, 52 insertions(+), 33 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 221f176f1f51..2dd9fca8c01c 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -195,15 +195,17 @@ static inline bool klp_have_reliable_stack(void)
 	       IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
 }
 
-void *klp_shadow_get(void *obj, unsigned long id);
-void *klp_shadow_alloc(void *obj, unsigned long id,
+void *klp_shadow_get(void *obj, unsigned int replace_set, unsigned int id);
+void *klp_shadow_alloc(void *obj, unsigned int replace_set, unsigned int id,
 		       size_t size, gfp_t gfp_flags,
 		       klp_shadow_ctor_t ctor, void *ctor_data);
-void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
+void *klp_shadow_get_or_alloc(void *obj, unsigned int replace_set, unsigned int id,
 			      size_t size, gfp_t gfp_flags,
 			      klp_shadow_ctor_t ctor, void *ctor_data);
-void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor);
-void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor);
+void klp_shadow_free(void *obj, unsigned int replace_set, unsigned int id,
+		     klp_shadow_dtor_t dtor);
+void klp_shadow_free_all(unsigned int replace_set, unsigned int id,
+			 klp_shadow_dtor_t dtor);
 
 struct klp_state *klp_get_state(struct klp_patch *patch, unsigned long id);
 struct klp_state *klp_get_prev_state(unsigned long id);
diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index c2e724d97ddf..35e507fae445 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -48,7 +48,8 @@ static DEFINE_SPINLOCK(klp_shadow_lock);
  * @node:	klp_shadow_hash hash table node
  * @rcu_head:	RCU is used to safely free this structure
  * @obj:	pointer to parent object
- * @id:		data identifier
+ * @id:		combined data identifier
+ *		higher 32 bits: replace_set, lower 32 bits: resource ID
  * @data:	data area
  */
 struct klp_shadow {
@@ -59,6 +60,11 @@ struct klp_shadow {
 	char data[];
 };
 
+static unsigned long klp_shadow_combined_id(unsigned int set, unsigned int id)
+{
+	return ((unsigned long)set << 32) | id;
+}
+
 /**
  * klp_shadow_match() - verify a shadow variable matches given <obj, id>
  * @shadow:	shadow variable to match
@@ -76,11 +82,12 @@ static inline bool klp_shadow_match(struct klp_shadow *shadow, void *obj,
 /**
  * klp_shadow_get() - retrieve a shadow variable data pointer
  * @obj:	pointer to parent object
+ * @replace_set:identifier for the livepatch replacement set
  * @id:		data identifier
  *
  * Return: the shadow variable data element, NULL on failure.
  */
-void *klp_shadow_get(void *obj, unsigned long id)
+void *klp_shadow_get(void *obj, unsigned int replace_set, unsigned int id)
 {
 	struct klp_shadow *shadow;
 
@@ -89,7 +96,8 @@ void *klp_shadow_get(void *obj, unsigned long id)
 	hash_for_each_possible_rcu(klp_shadow_hash, shadow, node,
 				   (unsigned long)obj) {
 
-		if (klp_shadow_match(shadow, obj, id)) {
+		if (klp_shadow_match(shadow, obj,
+				     klp_shadow_combined_id(replace_set, id))) {
 			rcu_read_unlock();
 			return shadow->data;
 		}
@@ -101,7 +109,7 @@ void *klp_shadow_get(void *obj, unsigned long id)
 }
 EXPORT_SYMBOL_GPL(klp_shadow_get);
 
-static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
+static void *__klp_shadow_get_or_alloc(void *obj, unsigned int set, unsigned int id,
 				       size_t size, gfp_t gfp_flags,
 				       klp_shadow_ctor_t ctor, void *ctor_data,
 				       bool warn_on_exist)
@@ -111,7 +119,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 	unsigned long flags;
 
 	/* Check if the shadow variable already exists */
-	shadow_data = klp_shadow_get(obj, id);
+	shadow_data = klp_shadow_get(obj, set, id);
 	if (shadow_data)
 		goto exists;
 
@@ -126,7 +134,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 
 	/* Look for <obj, id> again under the lock */
 	spin_lock_irqsave(&klp_shadow_lock, flags);
-	shadow_data = klp_shadow_get(obj, id);
+	shadow_data = klp_shadow_get(obj, set, id);
 	if (unlikely(shadow_data)) {
 		/*
 		 * Shadow variable was found, throw away speculative
@@ -147,8 +155,8 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 		if (err) {
 			spin_unlock_irqrestore(&klp_shadow_lock, flags);
 			kfree(new_shadow);
-			pr_err("Failed to construct shadow variable <%p, %lx> (%d)\n",
-			       obj, id, err);
+			pr_err("Failed to construct shadow variable <%p, %x, %x> (%d)\n",
+			       obj, set, id, err);
 			return NULL;
 		}
 	}
@@ -162,7 +170,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 
 exists:
 	if (warn_on_exist) {
-		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
+		WARN(1, "Duplicate shadow variable <%p, %x, %x>\n", obj, set, id);
 		return NULL;
 	}
 
@@ -172,6 +180,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 /**
  * klp_shadow_alloc() - allocate and add a new shadow variable
  * @obj:	pointer to parent object
+ * @replace_set:identifier for the livepatch replacement set
  * @id:		data identifier
  * @size:	size of attached data
  * @gfp_flags:	GFP mask for allocation
@@ -183,8 +192,8 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
  * function if it is not NULL.  The new shadow variable is then added
  * to the global hashtable.
  *
- * If an existing <obj, id> shadow variable can be found, this routine will
- * issue a WARN, exit early and return NULL.
+ * If an existing <obj, replace_set, id> shadow variable can be found, this
+ * routine will issue a WARN, exit early and return NULL.
  *
  * This function guarantees that the constructor function is called only when
  * the variable did not exist before.  The cost is that @ctor is called
@@ -193,11 +202,11 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
  * Return: the shadow variable data element, NULL on duplicate or
  * failure.
  */
-void *klp_shadow_alloc(void *obj, unsigned long id,
+void *klp_shadow_alloc(void *obj, unsigned int replace_set, unsigned int id,
 		       size_t size, gfp_t gfp_flags,
 		       klp_shadow_ctor_t ctor, void *ctor_data)
 {
-	return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
+	return __klp_shadow_get_or_alloc(obj, replace_set, id, size, gfp_flags,
 					 ctor, ctor_data, true);
 }
 EXPORT_SYMBOL_GPL(klp_shadow_alloc);
@@ -205,28 +214,29 @@ EXPORT_SYMBOL_GPL(klp_shadow_alloc);
 /**
  * klp_shadow_get_or_alloc() - get existing or allocate a new shadow variable
  * @obj:	pointer to parent object
+ * @replace_set:identifier for the livepatch replacement set
  * @id:		data identifier
  * @size:	size of attached data
  * @gfp_flags:	GFP mask for allocation
  * @ctor:	custom constructor to initialize the shadow data (optional)
  * @ctor_data:	pointer to any data needed by @ctor (optional)
  *
- * Returns a pointer to existing shadow data if an <obj, id> shadow
+ * Returns a pointer to existing shadow data if an <obj, replace_set, id> shadow
  * variable is already present.  Otherwise, it creates a new shadow
  * variable like klp_shadow_alloc().
  *
  * This function guarantees that only one shadow variable exists with the given
- * @id for the given @obj.  It also guarantees that the constructor function
- * will be called only when the variable did not exist before.  The cost is
- * that @ctor is called in atomic context under a spin lock.
+ * @id for the given @obj within the same replace_set.  It also guarantees that
+ * the constructor function will be called only when the variable did not exist
+ * before.  The cost is that @ctor is called in atomic context under a spin lock.
  *
  * Return: the shadow variable data element, NULL on failure.
  */
-void *klp_shadow_get_or_alloc(void *obj, unsigned long id,
-			      size_t size, gfp_t gfp_flags,
+void *klp_shadow_get_or_alloc(void *obj, unsigned int replace_set,
+			      unsigned int id, size_t size, gfp_t gfp_flags,
 			      klp_shadow_ctor_t ctor, void *ctor_data)
 {
-	return __klp_shadow_get_or_alloc(obj, id, size, gfp_flags,
+	return __klp_shadow_get_or_alloc(obj, replace_set, id, size, gfp_flags,
 					 ctor, ctor_data, false);
 }
 EXPORT_SYMBOL_GPL(klp_shadow_get_or_alloc);
@@ -243,14 +253,16 @@ static void klp_shadow_free_struct(struct klp_shadow *shadow,
 /**
  * klp_shadow_free() - detach and free a <obj, id> shadow variable
  * @obj:	pointer to parent object
+ * @replace_set:identifier for the livepatch replacement set
  * @id:		data identifier
  * @dtor:	custom callback that can be used to unregister the variable
  *		and/or free data that the shadow variable points to (optional)
  *
- * This function releases the memory for this <obj, id> shadow variable
+ * This function releases the memory for this <obj, replace_set, id> shadow variable
  * instance, callers should stop referencing it accordingly.
  */
-void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
+void klp_shadow_free(void *obj, unsigned int replace_set, unsigned int id,
+		     klp_shadow_dtor_t dtor)
 {
 	struct klp_shadow *shadow;
 	unsigned long flags;
@@ -261,7 +273,8 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
 	hash_for_each_possible(klp_shadow_hash, shadow, node,
 			       (unsigned long)obj) {
 
-		if (klp_shadow_match(shadow, obj, id)) {
+		if (klp_shadow_match(shadow, obj,
+				     klp_shadow_combined_id(replace_set, id))) {
 			klp_shadow_free_struct(shadow, dtor);
 			break;
 		}
@@ -272,15 +285,17 @@ void klp_shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
 EXPORT_SYMBOL_GPL(klp_shadow_free);
 
 /**
- * klp_shadow_free_all() - detach and free all <_, id> shadow variables
+ * klp_shadow_free_all() - detach and free all <_, replace_set, id> shadow variables
+ * @replace_set:identifier for the livepatch replacement set
  * @id:		data identifier
  * @dtor:	custom callback that can be used to unregister the variable
  *		and/or free data that the shadow variable points to (optional)
  *
- * This function releases the memory for all <_, id> shadow variable
+ * This function releases the memory for all <_, replace_set, id> shadow variable
  * instances, callers should stop referencing them accordingly.
  */
-void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
+void klp_shadow_free_all(unsigned int replace_set, unsigned int id,
+			 klp_shadow_dtor_t dtor)
 {
 	struct klp_shadow *shadow;
 	unsigned long flags;
@@ -290,7 +305,8 @@ void klp_shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
 
 	/* Delete all <_, id> from hash */
 	hash_for_each(klp_shadow_hash, i, shadow, node) {
-		if (klp_shadow_match(shadow, shadow->obj, id))
+		if (klp_shadow_match(shadow, shadow->obj,
+				     klp_shadow_combined_id(replace_set, id)))
 			klp_shadow_free_struct(shadow, dtor);
 	}
 
diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
index 43115e8e8453..6e3d6fb92e64 100644
--- a/kernel/livepatch/state.c
+++ b/kernel/livepatch/state.c
@@ -203,7 +203,8 @@ void klp_states_post_unpatch(struct klp_patch *patch)
 			state->callbacks.post_unpatch(patch, state);
 
 		if (state->is_shadow)
-			klp_shadow_free_all(state->id, state->callbacks.shadow_dtor);
+			klp_shadow_free_all(patch->replace_set, state->id,
+					    state->callbacks.shadow_dtor);
 
 		state->callbacks.pre_patch_succeeded = 0;
 	}
-- 
2.47.3

Re: [RFC PATCH 6/6] livepatch: Support replace_set in shadow variable API

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Newly allocated shadow variables fail to store the `replace_set` identifier, breaking all subsequent lookups and leading to unbounded memory leaks.
- [High] The `klp_shadow_combined_id()` function invokes undefined behavior and loses data on 32-bit architectures by shifting a 32-bit integer by 32 bits.
- [High] Changing the shadow API `id` parameter to `unsigned int` silently truncates 64-bit `klp_state` identifiers, leading to memory leaks.
- [Low] Modifying the `klp_shadow_*` function signatures breaks the build for in-tree callers.
--

commit 91116e31e667d0beea437b5048ab5a62099a5eed
Author: Yafang Shao <laoar.shao@gmail.com>

livepatch: Support replace_set in shadow variable API
    
To support more complex livepatching scenarios where multiple
replacement sets might coexist, extend the klp_shadow API to
include a replace_set identifier.

> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index c9f2036dfefcf..cee35f5cae9ad 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -192,15 +192,17 @@ static inline bool klp_have_reliable_stack(void)
>  	       IS_ENABLED(CONFIG_HAVE_RELIABLE_STACKTRACE);
>  }
>  
> -void *klp_shadow_get(void *obj, unsigned long id);
> -void *klp_shadow_alloc(void *obj, unsigned long id,
> +void *klp_shadow_get(void *obj, unsigned int replace_set, unsigned int id);
> +void *klp_shadow_alloc(void *obj, unsigned int replace_set, unsigned int id,
>  		       size_t size, gfp_t gfp_flags,
>  		       klp_shadow_ctor_t ctor, void *ctor_data);

Does this signature change break the build for existing in-tree callers?

It looks like samples/livepatch/livepatch-shadow-fix1.c and
tools/testing/selftests/livepatch/test_modules/test_klp_shadow_vars.c
might not have been updated to pass the new replace_set argument.

[ ... ]

> diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
> index c2e724d97ddf2..35e507fae4458 100644
> --- a/kernel/livepatch/shadow.c
> +++ b/kernel/livepatch/shadow.c
> @@ -59,6 +60,11 @@ struct klp_shadow {
>  	char data[];
>  };
>  
> +static unsigned long klp_shadow_combined_id(unsigned int set, unsigned int id)
> +{
> +	return ((unsigned long)set << 32) | id;
> +}

On 32-bit architectures that support livepatching (such as 32-bit PowerPC),
unsigned long is 32 bits wide. Does shifting a 32-bit integer by 32 bits
invoke undefined behavior here?

Furthermore, since struct klp_shadow->id remains defined as an unsigned long,
it cannot physically store a 64-bit combined ID on these systems. The
replace_set value might be entirely truncated, resulting in hash collisions
and subsequent issues when multiple sets manage the same resource.

[ ... ]

> @@ -126,7 +134,7 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
>  
>  	/* Look for <obj, id> again under the lock */
>  	spin_lock_irqsave(&klp_shadow_lock, flags);
> -	shadow_data = klp_shadow_get(obj, id);
> +	shadow_data = klp_shadow_get(obj, set, id);
>  	if (unlikely(shadow_data)) {
>  		/*
>  		 * Shadow variable was found, throw away speculative
> @@ -147,8 +155,8 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
[ ... ]
>  	}
>  
>  	new_shadow->obj = obj;
>  	new_shadow->id = id;

Because the id argument is now a 32-bit integer, the upper 32 bits of
new_shadow->id will be zero. Should this be initialized using
klp_shadow_combined_id(set, id) instead?

Lookup functions like klp_shadow_get() and klp_shadow_free() search using
the 64-bit composite ID klp_shadow_combined_id(replace_set, id). When
replace_set is non-zero, this lookup will always fail.

This means klp_shadow_get() will erroneously return NULL, and
klp_shadow_get_or_alloc() could duplicate variables indefinitely, leading
to unbounded memory leaks.

[ ... ]

> diff --git a/kernel/livepatch/state.c b/kernel/livepatch/state.c
> index 43115e8e8453c..6e3d6fb92e64a 100644
> --- a/kernel/livepatch/state.c
> +++ b/kernel/livepatch/state.c
> @@ -203,7 +203,8 @@ void klp_states_post_unpatch(struct klp_patch *patch)
>  			state->callbacks.post_unpatch(patch, state);
>  
>  		if (state->is_shadow)
> -			klp_shadow_free_all(state->id, state->callbacks.shadow_dtor);
> +			klp_shadow_free_all(patch->replace_set, state->id,
> +					    state->callbacks.shadow_dtor);

The shadow variable APIs previously accepted an unsigned long for the id,
allowing 64-bit identifiers. The new klp_shadow_free_all() now accepts an
unsigned int, but struct klp_state still defines id as an unsigned long.

Will the implicit cast from unsigned long to unsigned int silently truncate
the upper 32 bits of state->id on 64-bit platforms?

If the original ID exceeded 32 bits, the core might fail to locate and free
the correct shadow variables, causing a permanent memory leak during
garbage collection.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513143321.26185-1-laoar.shao@gmail.com?part=6