[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