[PATCH v2] livepatch: Fix races between stack check and module load/unload

[PATCH v2] livepatch: Fix races between stack check and module load/unload

[Andrea Parri]

Unloading a patched module while a livepatch transition is in progress
can crash the kernel in klp_find_ops(), called from the scheduler.

The scheduler checks the stack of a task that still has to switch
without klp_mutex: __klp_sched_try_switch() -> klp_try_switch_task() ->
klp_check_stack().  The check walks the global klp_ops list and each
func_stack, and reads obj->patched.

Loading or unloading a patched module changes these lists in
klp_patch_func() and klp_unpatch_func() under klp_mutex only, and
unloading frees the ops, so the walk can follow a freed entry.

Protect the lists and obj->patched with a raw spinlock, klp_ops_lock.
Writers take it only around the list updates, as the ftrace calls in
klp_patch_func() and klp_unpatch_func() can sleep, and free the ops
after dropping it.  klp_check_stack() only trylocks it, and checks the
task again later if that fails.

__klp_unpatch_object() now clears obj->patched before it unpatches the
functions, so that the stack check skips an object whose functions are
being removed.

The race was found with the help of an LLM.

Fixes: e3ff7c609f39 ("livepatch,sched: Add livepatch task switching to cond_resched()")
Cc: stable@vger.kernel.org
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
Changes in v2:
- Protect the klp_ops list, the func_stacks and obj->patched with a raw
  spinlock that klp_check_stack() only trylocks, instead of RCU and
  memory barriers (Petr)
- Check the locking with lockdep instead of describing it in comments
  (Peter)
- Rewrite the changelog without the traces (Miroslav)
- Drop the obj->patched barriers for module reload instead of splitting
  them out (Miroslav): obj->patched is now set under klp_ops_lock
- Note the LLM in the changelog instead of an Assisted-by tag, which
  was dropped upstream

v1: https://lore.kernel.org/r/20261012140546.154730-1-parri.andrea@gmail.com
Reproducer: https://lore.kernel.org/r/20261014133553.102324-1-parri.andrea@gmail.com

Tested on x86_64 in 4-CPU vng guests with five configs: KCSAN_STRICT;
KCSAN_STRICT with PROVE_LOCKING, PROVE_RAW_LOCK_NESTING,
DEBUG_ATOMIC_SLEEP and PROVE_RCU_LIST; KASAN with the same lock
debugging plus DEBUG_LIST, SLUB_DEBUG_ON and DEBUG_OBJECTS; and the
last two with PREEMPT_RT.

On each, the livepatch selftests were run, then the reproducer kept a
transition in progress while a patched module was unloaded and reloaded
for 30 minutes (229 to 358 reloads).  Without this patch, the
reproducer faults in klp_find_ops().

No KCSAN report involves livepatch code, and lockdep, RCU, KASAN and
the list and object debugging stay silent.

Two selftests fail, unrelated to this patch: test-syscall.sh catches a
KCSAN_STRICT report in osq_lock() from the test module's mutex, and on
PREEMPT_RT test-callbacks.sh catches a WARNING in klp_check_stack(),
from lockdep_assert_preemption_disabled(), also seen on v7.3-rc7.
---
 kernel/livepatch/patch.c      | 37 ++++++++++++++++++++++++++++++-----
 kernel/livepatch/patch.h      |  2 ++
 kernel/livepatch/transition.c | 31 ++++++++++++++++++++---------
 3 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 3f54a017bbf69..d75fc9a14488b 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -22,11 +22,21 @@
 
 static LIST_HEAD(klp_ops);
 
+/*
+ * klp_check_stack() walks klp_ops and the func_stacks from the scheduler,
+ * without klp_mutex, while a patched module can be loaded or unloaded.
+ * Changes to these lists and to obj->patched are made under this lock too.
+ */
+DEFINE_RAW_SPINLOCK(klp_ops_lock);
+
 struct klp_ops *klp_find_ops(void *old_func)
 {
 	struct klp_ops *ops;
 	struct klp_func *func;
 
+	lockdep_assert(lockdep_is_held(&klp_mutex) ||
+		       lockdep_is_held(&klp_ops_lock));
+
 	list_for_each_entry(ops, &klp_ops, node) {
 		func = list_first_entry(&ops->func_stack, struct klp_func,
 					stack_node);
@@ -147,11 +157,15 @@ static void klp_unpatch_func(struct klp_func *func)
 		WARN_ON(unregister_ftrace_function(&ops->fops));
 		WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
 
+		raw_spin_lock(&klp_ops_lock);
 		list_del_rcu(&func->stack_node);
 		list_del(&ops->node);
+		raw_spin_unlock(&klp_ops_lock);
 		kfree(ops);
 	} else {
+		raw_spin_lock(&klp_ops_lock);
 		list_del_rcu(&func->stack_node);
+		raw_spin_unlock(&klp_ops_lock);
 	}
 
 	func->patched = false;
@@ -191,11 +205,13 @@ static int klp_patch_func(struct klp_func *func)
 				  FTRACE_OPS_FL_IPMODIFY |
 				  FTRACE_OPS_FL_PERMANENT;
 
-		list_add(&ops->node, &klp_ops);
-
 		INIT_LIST_HEAD(&ops->func_stack);
 		list_add_rcu(&func->stack_node, &ops->func_stack);
 
+		raw_spin_lock(&klp_ops_lock);
+		list_add(&ops->node, &klp_ops);
+		raw_spin_unlock(&klp_ops_lock);
+
 		ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0);
 		if (ret) {
 			pr_err("failed to set ftrace filter for function '%s' (%d)\n",
@@ -213,7 +229,9 @@ static int klp_patch_func(struct klp_func *func)
 
 
 	} else {
+		raw_spin_lock(&klp_ops_lock);
 		list_add_rcu(&func->stack_node, &ops->func_stack);
+		raw_spin_unlock(&klp_ops_lock);
 	}
 
 	func->patched = true;
@@ -221,8 +239,10 @@ static int klp_patch_func(struct klp_func *func)
 	return 0;
 
 err:
+	raw_spin_lock(&klp_ops_lock);
 	list_del_rcu(&func->stack_node);
 	list_del(&ops->node);
+	raw_spin_unlock(&klp_ops_lock);
 	kfree(ops);
 	return ret;
 }
@@ -231,6 +251,13 @@ static void __klp_unpatch_object(struct klp_object *obj, bool nops_only)
 {
 	struct klp_func *func;
 
+	/* Make klp_check_stack() skip the object before its funcs go away. */
+	if (obj->dynamic || !nops_only) {
+		raw_spin_lock(&klp_ops_lock);
+		obj->patched = false;
+		raw_spin_unlock(&klp_ops_lock);
+	}
+
 	klp_for_each_func(obj, func) {
 		if (nops_only && !func->nop)
 			continue;
@@ -238,9 +265,6 @@ static void __klp_unpatch_object(struct klp_object *obj, bool nops_only)
 		if (func->patched)
 			klp_unpatch_func(func);
 	}
-
-	if (obj->dynamic || !nops_only)
-		obj->patched = false;
 }
 
 
@@ -264,7 +288,10 @@ int klp_patch_object(struct klp_object *obj)
 			return ret;
 		}
 	}
+
+	raw_spin_lock(&klp_ops_lock);
 	obj->patched = true;
+	raw_spin_unlock(&klp_ops_lock);
 
 	return 0;
 }
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index d5f2fbe373e05..6441373f1c2f2 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -25,6 +25,8 @@ struct klp_ops {
 	struct ftrace_ops fops;
 };
 
+extern raw_spinlock_t klp_ops_lock;
+
 struct klp_ops *klp_find_ops(void *old_func);
 
 int klp_patch_object(struct klp_object *obj);
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index 2351a19ac2a9e..7b5cc14037e59 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -204,11 +204,14 @@ void klp_update_patch_state(struct task_struct *task)
  */
 static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
 				unsigned int nr_entries)
+	__must_hold(&klp_ops_lock)
 {
 	unsigned long func_addr, func_size, address;
 	struct klp_ops *ops;
 	int i;
 
+	lockdep_assert_held(&klp_ops_lock);
+
 	if (klp_target_state == KLP_TRANSITION_UNPATCHED) {
 		 /*
 		  * Check for the to-be-unpatched function
@@ -266,14 +269,20 @@ static int klp_check_stack(struct task_struct *task, const char **oldname)
 		return -EINVAL;
 	nr_entries = ret;
 
-	klp_for_each_object(klp_transition_patch, obj) {
-		if (!obj->patched)
-			continue;
-		klp_for_each_func(obj, func) {
-			ret = klp_check_stack_func(func, entries, nr_entries);
-			if (ret) {
-				*oldname = func->old_name;
-				return -EADDRINUSE;
+	/*
+	 * The lock is taken by module load/unload and by stack checks on other
+	 * CPUs.  Do not spin on it from the scheduler, retry the task later.
+	 */
+	scoped_cond_guard(raw_spinlock_try, return -EAGAIN, &klp_ops_lock) {
+		klp_for_each_object(klp_transition_patch, obj) {
+			if (!obj->patched)
+				continue;
+			klp_for_each_func(obj, func) {
+				ret = klp_check_stack_func(func, entries, nr_entries);
+				if (ret) {
+					*oldname = func->old_name;
+					return -EADDRINUSE;
+				}
 			}
 		}
 	}
@@ -300,7 +309,7 @@ static int klp_check_and_switch_task(struct task_struct *task, void *arg)
 /*
  * Try to safely switch a task to the target patch state.  If it's currently
  * running, or it's sleeping on a to-be-patched or to-be-unpatched function, or
- * if the stack is unreliable, return false.
+ * if the stack is unreliable, or if klp_ops_lock is busy, return false.
  */
 static bool klp_try_switch_task(struct task_struct *task)
 {
@@ -344,6 +353,10 @@ static bool klp_try_switch_task(struct task_struct *task)
 		pr_debug("%s: %s:%d is sleeping on function %s\n",
 			 __func__, task->comm, task->pid, old_name);
 		break;
+	case -EAGAIN:	/* klp_check_and_switch_task() */
+		pr_debug("%s: %s:%d could not be checked, klp_ops_lock is busy\n",
+			 __func__, task->comm, task->pid);
+		break;
 
 	default:
 		pr_debug("%s: Unknown error code (%d) when trying to switch %s:%d\n",

base-commit: a804c5742bff224d4cb4d25dd383554d598be32b
-- 
2.53.0

Re: [PATCH v2] livepatch: Fix races between stack check and module load/unload

[Petr Mladek]

Hi,

I have finally found time and energy to scratch my head around this.
I am sorry that it took so long.

The solution is still a bit complicated. But maybe, we could simplify
it even more, see below.

On Thu 2026-10-22 15:44:31, Andrea Parri wrote:
> Unloading a patched module while a livepatch transition is in progress
> can crash the kernel in klp_find_ops(), called from the scheduler.
> 
> The scheduler checks the stack of a task that still has to switch
> without klp_mutex: __klp_sched_try_switch() -> klp_try_switch_task() ->
> klp_check_stack().  The check walks the global klp_ops list and each
> func_stack, and reads obj->patched.
> 
> Loading or unloading a patched module changes these lists in
> klp_patch_func() and klp_unpatch_func() under klp_mutex only, and
> unloading frees the ops, so the walk can follow a freed entry.

To make sure that I understand it correctly. I looked at [v1] and it
solved the problems:

  + racy access to func->old_func, func->new_func, obj->patched
    by using READ_ONCE() and WRITE_ONCE().

  + UAF access to ops->node by RCU.

There are few more details in v1 but they do not look that important
for what I want to say below.

[v1] https://lore.kernel.org/r/20261012140546.154730-1-parri.andrea@gmail.com

> Protect the lists and obj->patched with a raw spinlock, klp_ops_lock.
> Writers take it only around the list updates, as the ftrace calls in
> klp_patch_func() and klp_unpatch_func() can sleep, and free the ops
> after dropping it.  klp_check_stack() only trylocks it, and checks the
> task again later if that fails.
> 
> __klp_unpatch_object() now clears obj->patched before it unpatches the
> functions, so that the stack check skips an object whose functions are
> being removed.

So, v2 tries to solve the problems by:

  + taking klp_ops_lock around manipulating: ops->node, func->stack_node,
      ops->func_stack, obj->patched

  + manipulating obj->patched after obj->funcs are ready
    manipulating obj->patched before removing obj->funcs

  + taking klp_ops_lock around klp_for_each_object() in
     klp_check_stack()

You see how the two versions are different. In fact, they have
only the obj->patched manipulation in common.

IMHO, we need to better document the solution so that
we understand it later. Or maybe even make it easier,
see below.


> --- a/kernel/livepatch/patch.c
> +++ b/kernel/livepatch/patch.c
> @@ -22,11 +22,21 @@
>  
>  static LIST_HEAD(klp_ops);
>  
> +/*
> + * klp_check_stack() walks klp_ops and the func_stacks from the scheduler,
> + * without klp_mutex, while a patched module can be loaded or unloaded.
> + * Changes to these lists and to obj->patched are made under this lock too.
> + */
> +DEFINE_RAW_SPINLOCK(klp_ops_lock);
> +
>  struct klp_ops *klp_find_ops(void *old_func)
>  {
>  	struct klp_ops *ops;
>  	struct klp_func *func;
>  
> +	lockdep_assert(lockdep_is_held(&klp_mutex) ||
> +		       lockdep_is_held(&klp_ops_lock));
> +
>  	list_for_each_entry(ops, &klp_ops, node) {
>  		func = list_first_entry(&ops->func_stack, struct klp_func,
>  					stack_node);
> @@ -147,11 +157,15 @@ static void klp_unpatch_func(struct klp_func *func)
>  		WARN_ON(unregister_ftrace_function(&ops->fops));
>  		WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
>  
> +		raw_spin_lock(&klp_ops_lock);
>  		list_del_rcu(&func->stack_node);
>  		list_del(&ops->node);
> +		raw_spin_unlock(&klp_ops_lock);
>  		kfree(ops);

This is called for the very last entry when the 'ops' is freed.
Is the lock really needed here?

IMHO, klp_check_stack() should never see this entry because
obj->patched is disabled before, see below.


>  	} else {
> +		raw_spin_lock(&klp_ops_lock);
>  		list_del_rcu(&func->stack_node);
> +		raw_spin_unlock(&klp_ops_lock);

This is called when ops->func_stack has more entries and we
just remove remove one.

What exactly the lock synchronizes here, please?

IMHO, it is related to:

static int klp_check_stack_func(struct klp_func *func, unsigned long *entries,
				unsigned int nr_entries)
{
[...]
		/*
		 * Check for the to-be-patched function
		 * (the previous func).
		 */
		ops = klp_find_ops(func->old_func);

		if (list_is_singular(&ops->func_stack)) {
			/* original function */
			func_addr = (unsigned long)func->old_func;
			func_size = func->old_size;
		} else {
			/* previously patched function */
			struct klp_func *prev;

			prev = list_next_entry(func, stack_node);
			func_addr = (unsigned long)prev->new_func;
			func_size = prev->new_size;
		}
	}

Now, when I look what v1 did here:

  1. It used RCU to access ops and it expected that it might be NULL.
     Also it replaced list_next_entry() with list_next_or_null_rcu().

     IMHO, this is only relevant for the last entry in ops
     which klp_check_stack() should never see as explained above.


  2. It used READ_ONCE(func->old_func).

     IMHO, the spin lock would be needed here only when
     it synchronizes the modification of func->old_func here.

     I think that we do not need the spin lock here because
     func->old_func is cleared in klp_free_object_loaded()
     which is called after obj->patched is cleared under
     the spin lock.

>  	}

Summary: IMHO, we do not need to take the spin lock here in
	klp_unpatch_func(). It should be enough to take
	it around manipulating obj->patched.

>  
>  	func->patched = false;
> @@ -191,11 +205,13 @@ static int klp_patch_func(struct klp_func *func)
>  				  FTRACE_OPS_FL_IPMODIFY |
>  				  FTRACE_OPS_FL_PERMANENT;
>  
> -		list_add(&ops->node, &klp_ops);
> -
>  		INIT_LIST_HEAD(&ops->func_stack);
>  		list_add_rcu(&func->stack_node, &ops->func_stack);
>  
> +		raw_spin_lock(&klp_ops_lock);
> +		list_add(&ops->node, &klp_ops);
> +		raw_spin_unlock(&klp_ops_lock);

First, this is not consistent with the "unpatch" code path
where we take the lock around both:

		list_del_rcu(&func->stack_node);
		list_del(&ops->node);

But again, this is the code path for the first entry in the freshly
allocated "ops". Nobody could see it until obj->patched is enabled
in klp_patch_object().

   => the lock should not be needed here.

> +
>  		ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0);
>  		if (ret) {
>  			pr_err("failed to set ftrace filter for function '%s' (%d)\n",
> @@ -213,7 +229,9 @@ static int klp_patch_func(struct klp_func *func)
>  
>  
>  	} else {
> +		raw_spin_lock(&klp_ops_lock);
>  		list_add_rcu(&func->stack_node, &ops->func_stack);
> +		raw_spin_unlock(&klp_ops_lock);

This is the code patch for adding one more entry on the stack.
I am not sure why we would need the lock here.

It might be needed only for the race in func->old_addr. But I think
list_add_rcu() already includes the barrier.

>  	}
>  
>  	func->patched = true;
> @@ -221,8 +239,10 @@ static int klp_patch_func(struct klp_func *func)
>  	return 0;
>  
>  err:
> +	raw_spin_lock(&klp_ops_lock);
>  	list_del_rcu(&func->stack_node);
>  	list_del(&ops->node);
> +	raw_spin_unlock(&klp_ops_lock);

Like above.

>  	kfree(ops);
>  	return ret;
>  }
> @@ -231,6 +251,13 @@ static void __klp_unpatch_object(struct klp_object *obj, bool nops_only)
>  {
>  	struct klp_func *func;
>  
> +	/* Make klp_check_stack() skip the object before its funcs go away. */
> +	if (obj->dynamic || !nops_only) {
> +		raw_spin_lock(&klp_ops_lock);
> +		obj->patched = false;
> +		raw_spin_unlock(&klp_ops_lock);
> +	}

IMHO, this is the only location where we need to take the
spinlock in the klp_unpatch_object_loaded() code path.

> +
>  	klp_for_each_func(obj, func) {
>  		if (nops_only && !func->nop)
>  			continue;
> @@ -238,9 +265,6 @@ static void __klp_unpatch_object(struct klp_object *obj, bool nops_only)
>  		if (func->patched)
>  			klp_unpatch_func(func);
>  	}
> -
> -	if (obj->dynamic || !nops_only)
> -		obj->patched = false;
>  }
>  
>  
> @@ -264,7 +288,10 @@ int klp_patch_object(struct klp_object *obj)
>  			return ret;
>  		}
>  	}
> +

This would deserve similar comment like klp_unpatch_object(), e.g.:

	/*
	 * Allow klp_check_stack() to check the object only after funcs
	 * are initialized.
	 */

> +	raw_spin_lock(&klp_ops_lock);
>  	obj->patched = true;
> +	raw_spin_unlock(&klp_ops_lock);

Also this looks important to avoid races. But maybe not because
the main problem was when unloading the livepatched module.

>  	return 0;
>  }
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -266,14 +269,20 @@ static int klp_check_stack(struct task_struct *task, const char **oldname)
>  		return -EINVAL;
>  	nr_entries = ret;
>  
> -	klp_for_each_object(klp_transition_patch, obj) {
> -		if (!obj->patched)
> -			continue;
> -		klp_for_each_func(obj, func) {
> -			ret = klp_check_stack_func(func, entries, nr_entries);
> -			if (ret) {
> -				*oldname = func->old_name;
> -				return -EADDRINUSE;
> +	/*
> +	 * The lock is taken by module load/unload and by stack checks on other
> +	 * CPUs.  Do not spin on it from the scheduler, retry the task later.
> +	 */
> +	scoped_cond_guard(raw_spinlock_try, return -EAGAIN, &klp_ops_lock) {
> +		klp_for_each_object(klp_transition_patch, obj) {
> +			if (!obj->patched)
> +				continue;
> +			klp_for_each_func(obj, func) {
> +				ret = klp_check_stack_func(func, entries, nr_entries);
> +				if (ret) {
> +					*oldname = func->old_name;
> +					return -EADDRINUSE;
> +				}
>  			}
>  		}
>  	}

And of course, this is important.

> @@ -300,7 +309,7 @@ static int klp_check_and_switch_task(struct task_struct *task, void *arg)
>  /*
>   * Try to safely switch a task to the target patch state.  If it's currently
>   * running, or it's sleeping on a to-be-patched or to-be-unpatched function, or
> - * if the stack is unreliable, return false.
> + * if the stack is unreliable, or if klp_ops_lock is busy, return false.
>   */
>  static bool klp_try_switch_task(struct task_struct *task)
>  {
> @@ -344,6 +353,10 @@ static bool klp_try_switch_task(struct task_struct *task)
>  		pr_debug("%s: %s:%d is sleeping on function %s\n",
>  			 __func__, task->comm, task->pid, old_name);
>  		break;
> +	case -EAGAIN:	/* klp_check_and_switch_task() */
> +		pr_debug("%s: %s:%d could not be checked, klp_ops_lock is busy\n",
> +			 __func__, task->comm, task->pid);
> +		break;

And this.

>  
>  	default:
>  		pr_debug("%s: Unknown error code (%d) when trying to switch %s:%d\n",
> 
> base-commit: a804c5742bff224d4cb4d25dd383554d598be32b

I am sorry once again for the late reply. I hope that you still might
be able to handle the feedback and eventually test the simplified patch.

Best Regards,
Petr

Re: [PATCH v2] livepatch: Fix races between stack check and module load/unload

[Andrea Parri]

Hi Petr,

On Thu, Nov 12, 2026 at 06:50:09PM +0100, Petr Mladek wrote:
> I am sorry once again for the late reply. I hope that you still might
> be able to handle the feedback and eventually test the simplified patch.

I'm not planning to pursue this further, so there won't be a v3 from me.
I'd be happy to see you, or anybody else interested, pick this work up:
please feel free to reuse any part of v1, v2 and of the reproducer.

Thanks,
  Andrea