livepatch: hybrid consistency model

For live patching and possibly other use cases, a stack trace is only useful if it can be assured that it's completely reliable. Add a new save_stack_trace_tsk_reliable() function to achieve that.

Note that if the target task isn't the current task, and the target task is allowed to run, then it could be writing the stack while the unwinder is reading it, resulting in possible corruption. So the caller of save_stack_trace_tsk_reliable() must ensure that the task is either 'current' or inactive.

save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection of pt_regs on the stack. If the pt_regs are not user-mode registers from a syscall, then they indicate an in-kernel interrupt or exception (e.g. preemption or a page fault), in which case the stack is considered unreliable due to the nature of frame pointers.

It also relies on the x86 unwinder's detection of other issues, such as:

- corrupted stack data
- stack grows the wrong way
- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

Such issues are reported by checking unwind_error() and !unwind_done().

Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can determine at build time whether the function is implemented.

Signed-off-by Josh Poimboeuf <[email protected]>

Quoted 25 lines · Josh Poimboeuf

For live patching and possibly other use cases, a stack trace is only useful if it can be assured that it's completely reliable. Add a new save_stack_trace_tsk_reliable() function to achieve that.

Note that if the target task isn't the current task, and the target task is allowed to run, then it could be writing the stack while the unwinder is reading it, resulting in possible corruption. So the caller of save_stack_trace_tsk_reliable() must ensure that the task is either 'current' or inactive.

save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection of pt_regs on the stack. If the pt_regs are not user-mode registers from a syscall, then they indicate an in-kernel interrupt or exception (e.g. preemption or a page fault), in which case the stack is considered unreliable due to the nature of frame pointers.

It also relies on the x86 unwinder's detection of other issues, such as:

- corrupted stack data
- stack grows the wrong way
- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

Such issues are reported by checking unwind_error() and !unwind_done().

Quoted 4 lines

Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can determine at build time whether the function is implemented.

Signed-off-by Josh Poimboeuf <[email protected]>

I do not see any difference from 4.1 version, so my

Reviewed-by Miroslav Benes <[email protected]>

For live patching and possibly other use cases, a stack trace is only useful if it can be assured that it's completely reliable. Add a new save_stack_trace_tsk_reliable() function to achieve that.

Note that if the target task isn't the current task, and the target task is allowed to run, then it could be writing the stack while the unwinder is reading it, resulting in possible corruption. So the caller of save_stack_trace_tsk_reliable() must ensure that the task is either 'current' or inactive.

save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection of pt_regs on the stack. If the pt_regs are not user-mode registers from a syscall, then they indicate an in-kernel interrupt or exception (e.g. preemption or a page fault), in which case the stack is considered unreliable due to the nature of frame pointers.

It also relies on the x86 unwinder's detection of other issues, such as:

- corrupted stack data
- stack grows the wrong way
- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

Such issues are reported by checking unwind_error() and !unwind_done().

Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can determine at build time whether the function is implemented.

Signed-off-by Josh Poimboeuf <[email protected]>

---

Quoted 25 lines · Josh Poimboeuf

For live patching and possibly other use cases, a stack trace is only useful if it can be assured that it's completely reliable. Add a new save_stack_trace_tsk_reliable() function to achieve that.

Note that if the target task isn't the current task, and the target task is allowed to run, then it could be writing the stack while the unwinder is reading it, resulting in possible corruption. So the caller of save_stack_trace_tsk_reliable() must ensure that the task is either 'current' or inactive.

save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection of pt_regs on the stack. If the pt_regs are not user-mode registers from a syscall, then they indicate an in-kernel interrupt or exception (e.g. preemption or a page fault), in which case the stack is considered unreliable due to the nature of frame pointers.

It also relies on the x86 unwinder's detection of other issues, such as:

- corrupted stack data
- stack grows the wrong way
- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

Such issues are reported by checking unwind_error() and !unwind_done().

Quoted 5 lines

Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can determine at build time whether the function is implemented.

Signed-off-by Josh Poimboeuf <[email protected]>

---

Could you comment on why we need a reliable trace for live-patching? Are we in any way reliant on the stack trace to patch something broken?

Quoted 31 lines · Balbir Singh

Quoted 30 lines · Josh Poimboeuf

For live patching and possibly other use cases, a stack trace is only useful if it can be assured that it's completely reliable.  Add a new save_stack_trace_tsk_reliable() function to achieve that.

Note that if the target task isn't the current task, and the target task is allowed to run, then it could be writing the stack while the unwinder is reading it, resulting in possible corruption.  So the caller of save_stack_trace_tsk_reliable() must ensure that the task is either 'current' or inactive.

save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection of pt_regs on the stack.  If the pt_regs are not user-mode registers from a syscall, then they indicate an in-kernel interrupt or exception (e.g. preemption or a page fault), in which case the stack is considered unreliable due to the nature of frame pointers.

It also relies on the x86 unwinder's detection of other issues, such as:

- corrupted stack data
- stack grows the wrong way
- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

Such issues are reported by checking unwind_error() and !unwind_done().
Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can determine at build time whether the function is implemented.

Signed-off-by Josh Poimboeuf <[email protected]>

---

Quoted 3 lines

Could you comment on why we need a reliable trace for live-patching? Are we in any way reliant on the stack trace to patch something broken?

I tried to cover this comprehensively in patch 13/15 in Documentation/livepatch/livepatch.txt.  Does that answer your questions?

For live patching and possibly other use cases, a stack trace is only useful if it can be assured that it's completely reliable. Add a new save_stack_trace_tsk_reliable() function to achieve that.

Note that if the target task isn't the current task, and the target task is allowed to run, then it could be writing the stack while the unwinder is reading it, resulting in possible corruption. So the caller of save_stack_trace_tsk_reliable() must ensure that the task is either 'current' or inactive.

save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection of pt_regs on the stack. If the pt_regs are not user-mode registers from a syscall, then they indicate an in-kernel interrupt or exception (e.g. preemption or a page fault), in which case the stack is considered unreliable due to the nature of frame pointers.

It also relies on the x86 unwinder's detection of other issues, such as:

- corrupted stack data
- stack grows the wrong way
- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

Such issues are reported by checking unwind_error() and !unwind_done().

Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can determine at build time whether the function is implemented.

Signed-off-by Josh Poimboeuf <[email protected]>

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for powerpc. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel.

The bit is included in the _TIF_USER_WORK_MASK macro so that do_notify_resume() and klp_update_patch_state() get called when the bit is set.

Signed-off-by Josh Poimboeuf <[email protected]>

Reviewed-by Petr Mladek <[email protected]>

Reviewed-by Miroslav Benes <[email protected]>

Reviewed-by Kamalesh Babulal <[email protected]>

---

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for powerpc. The bit getting set indicates the thread has a pending patch which needs to be applied when the thread exits the kernel.

The bit is included in the _TIF_USER_WORK_MASK macro so that do_notify_resume() and klp_update_patch_state() get called when the bit is set.

Signed-off-by Josh Poimboeuf <[email protected]>

Reviewed-by Petr Mladek <[email protected]>

Reviewed-by Miroslav Benes <[email protected]>

Reviewed-by Kamalesh Babulal <[email protected]>

---
 arch/powerpc/include/asm/thread_info.h | 4 +++-
 arch/powerpc/kernel/signal.c           | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)

kernel/livepatch/core.c +18 -2

@@ -347,22 +356,36 @@ static int __klp_enable_patch(struct klp_patch *patch)
 
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
+	klp_init_transition(patch, KLP_PATCHED);
+
+	/*
+	 * Enforce the order of the func->transition writes in
+	 * klp_init_transition() and the ops->func_stack writes in
+	 * klp_patch_object(), so that klp_ftrace_handler() will see the
+	 * func->transition updates before the handler is registered and the
+	 * new funcs become visible to the handler.
+	 */
+	smp_wmb();
+
 	klp_for_each_object(patch, obj) {
 		if (!klp_is_object_loaded(obj))
 			continue;
 
 		ret = klp_patch_object(obj);
-		if (ret)
-			goto unregister;
+		if (ret) {
+			pr_warn("failed to enable patch '%s'\n",
+				patch->mod->name);
+
+			klp_cancel_transition();
+			return ret;
+		}

kernel/livepatch/transition.c new file +53 -0

+static void klp_complete_transition(void)
+{
+	struct klp_object *obj;
+	struct klp_func *func;
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	if (klp_target_state == KLP_UNPATCHED) {
+		/*
+		 * All tasks have transitioned to KLP_UNPATCHED so we can now
+		 * remove the new functions from the func_stack.
+		 */
+		klp_unpatch_objects(klp_transition_patch);
+
+		/*
+		 * Make sure klp_ftrace_handler() can no longer see functions
+		 * from this patch on the ops->func_stack.  Otherwise, after
+		 * func->transition gets cleared, the handler may choose a
+		 * removed function.
+		 */
+		synchronize_rcu();
+	}
+
+	if (klp_transition_patch->immediate)
+		goto done;
+
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = false;
+
+	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
+	if (klp_target_state == KLP_PATCHED)
+		synchronize_rcu();
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task) {
+		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
+		task->patch_state = KLP_UNDEFINED;
+	}
+	read_unlock(&tasklist_lock);
+
+	for_each_possible_cpu(cpu) {
+		task = idle_task(cpu);
+		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
+		task->patch_state = KLP_UNDEFINED;
+	}
+
+done:
+	klp_target_state = KLP_UNDEFINED;
+	klp_transition_patch = NULL;
+}
+
+/*

Quoted 30 lines

(quoted diff) +18 -2

@@ -347,22 +356,36 @@ static int __klp_enable_patch(struct klp_patch *patch)
 
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
+	klp_init_transition(patch, KLP_PATCHED);
+
+	/*
+	 * Enforce the order of the func->transition writes in
+	 * klp_init_transition() and the ops->func_stack writes in
+	 * klp_patch_object(), so that klp_ftrace_handler() will see the
+	 * func->transition updates before the handler is registered and the
+	 * new funcs become visible to the handler.
+	 */
+	smp_wmb();
+
 	klp_for_each_object(patch, obj) {
 		if (!klp_is_object_loaded(obj))
 			continue;
 
 		ret = klp_patch_object(obj);
-		if (ret)
-			goto unregister;
+		if (ret) {
+			pr_warn("failed to enable patch '%s'\n",
+				patch->mod->name);
+
+			klp_cancel_transition();
+			return ret;
+		}

[...]

Quoted 53 lines

(quoted diff) +53 -0

+static void klp_complete_transition(void)
+{
+	struct klp_object *obj;
+	struct klp_func *func;
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	if (klp_target_state == KLP_UNPATCHED) {
+		/*
+		 * All tasks have transitioned to KLP_UNPATCHED so we can now
+		 * remove the new functions from the func_stack.
+		 */
+		klp_unpatch_objects(klp_transition_patch);
+
+		/*
+		 * Make sure klp_ftrace_handler() can no longer see functions
+		 * from this patch on the ops->func_stack.  Otherwise, after
+		 * func->transition gets cleared, the handler may choose a
+		 * removed function.
+		 */
+		synchronize_rcu();
+	}
+
+	if (klp_transition_patch->immediate)
+		goto done;
+
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = false;
+
+	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
+	if (klp_target_state == KLP_PATCHED)
+		synchronize_rcu();
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task) {
+		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
+		task->patch_state = KLP_UNDEFINED;
+	}
+	read_unlock(&tasklist_lock);
+
+	for_each_possible_cpu(cpu) {
+		task = idle_task(cpu);
+		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
+		task->patch_state = KLP_UNDEFINED;
+	}
+
+done:
+	klp_target_state = KLP_UNDEFINED;
+	klp_transition_patch = NULL;
+}
+
+/*

Review

Quoted 10 lines

(quoted diff) +10 -0

+ * This is called in the error path, to cancel a transition before it has
+ * started, i.e. klp_init_transition() has been called but
+ * klp_start_transition() hasn't.  If the transition *has* been started,
+ * klp_reverse_transition() should be used instead.
+ */
+void klp_cancel_transition(void)
+{
+	klp_target_state = !klp_target_state;
+	klp_complete_transition();
+}

If we fail to enable patch in __klp_enable_patch(), we call klp_cancel_transition() and get to klp_complete_transition(). If the patch has immediate set to true, the module would not be allowed to go (the changes are in the last patch unfortunately, but my remark is closely related to klp_cancel_transition() and __klp_enable_patch()). This could annoy a user if it was due to a trivial reason. So we could call module_put() in this case. It should be safe as no task could be in a new function thanks to klp_ftrace_handler() logic.

Pity I have not spotted this earlier.

Putting module_put(patch->mod) right after klp_cancel_transition() call in __klp_enable_patch() would be the simplest fix (as a part of 15/15 patch). Maybe with a comment that it is safe to do it there.

What do you think?

Quoted 112 lines · Miroslav Benes

Quoted 30 lines

(quoted diff) +18 -2

@@ -347,22 +356,36 @@ static int __klp_enable_patch(struct klp_patch *patch)
 
 	pr_notice("enabling patch '%s'\n", patch->mod->name);
 
+	klp_init_transition(patch, KLP_PATCHED);
+
+	/*
+	 * Enforce the order of the func->transition writes in
+	 * klp_init_transition() and the ops->func_stack writes in
+	 * klp_patch_object(), so that klp_ftrace_handler() will see the
+	 * func->transition updates before the handler is registered and the
+	 * new funcs become visible to the handler.
+	 */
+	smp_wmb();
+
 	klp_for_each_object(patch, obj) {
 		if (!klp_is_object_loaded(obj))
 			continue;
 
 		ret = klp_patch_object(obj);
-		if (ret)
-			goto unregister;
+		if (ret) {
+			pr_warn("failed to enable patch '%s'\n",
+				patch->mod->name);
+
+			klp_cancel_transition();
+			return ret;
+		}

[...]

Quoted 53 lines

(quoted diff) +53 -0

+static void klp_complete_transition(void)
+{
+	struct klp_object *obj;
+	struct klp_func *func;
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	if (klp_target_state == KLP_UNPATCHED) {
+		/*
+		 * All tasks have transitioned to KLP_UNPATCHED so we can now
+		 * remove the new functions from the func_stack.
+		 */
+		klp_unpatch_objects(klp_transition_patch);
+
+		/*
+		 * Make sure klp_ftrace_handler() can no longer see functions
+		 * from this patch on the ops->func_stack.  Otherwise, after
+		 * func->transition gets cleared, the handler may choose a
+		 * removed function.
+		 */
+		synchronize_rcu();
+	}
+
+	if (klp_transition_patch->immediate)
+		goto done;
+
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = false;
+
+	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
+	if (klp_target_state == KLP_PATCHED)
+		synchronize_rcu();
+
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task) {
+		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
+		task->patch_state = KLP_UNDEFINED;
+	}
+	read_unlock(&tasklist_lock);
+
+	for_each_possible_cpu(cpu) {
+		task = idle_task(cpu);
+		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
+		task->patch_state = KLP_UNDEFINED;
+	}
+
+done:
+	klp_target_state = KLP_UNDEFINED;
+	klp_transition_patch = NULL;
+}
+
+/*

Review

Quoted 10 lines

(quoted diff) +10 -0

+ * This is called in the error path, to cancel a transition before it has
+ * started, i.e. klp_init_transition() has been called but
+ * klp_start_transition() hasn't.  If the transition *has* been started,
+ * klp_reverse_transition() should be used instead.
+ */
+void klp_cancel_transition(void)
+{
+	klp_target_state = !klp_target_state;
+	klp_complete_transition();
+}

If we fail to enable patch in __klp_enable_patch(), we call klp_cancel_transition() and get to klp_complete_transition(). If the patch has immediate set to true, the module would not be allowed to go (the changes are in the last patch unfortunately, but my remark is closely related to klp_cancel_transition() and __klp_enable_patch()). This could annoy a user if it was due to a trivial reason. So we could call module_put() in this case. It should be safe as no task could be in a new function thanks to klp_ftrace_handler() logic.

Pity I have not spotted this earlier.

Putting module_put(patch->mod) right after klp_cancel_transition() call in __klp_enable_patch() would be the simplest fix (as a part of 15/15 patch). Maybe with a comment that it is safe to do it there.

Quoted 1 line

What do you think?

Good catch. I agree that 15/15 should have something like that.

Also, the module_put() will be needed for non-immediate patches which have a func->immediate set.

What do you think about the following? I tried to put the logic in klp_complete_transition(), so the module_put()'s would be in one place. But it was too messy, so I put it in klp_cancel_transition() instead.

kernel/livepatch/transition.c +0 -0

index e96346e..bd1c1fd 100644

kernel/livepatch/transition.c +5 -0

+
+	/*
+	 * In the enable error path, even immediate patches can be safely
+	 * removed because the transition hasn't been started yet.
+	 *

Quoted 16 lines · Josh Poimboeuf

What do you think about the following? I tried to put the logic in klp_complete_transition(), so the module_put()'s would be in one place. But it was too messy, so I put it in klp_cancel_transition() instead.

kernel/livepatch/transition.c +2 -0

index e96346e..bd1c1fd 100644
@@ -121,8 +121,28 @@ static void klp_complete_transition(void)
  */
 void klp_cancel_transition(void)
 {
+	bool immediate_func = false;
+
 	klp_target_state = !klp_target_state;
 	klp_complete_transition();

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 130

Quoted 3 lines

kernel/livepatch/transition.c +3 -0

+
+	if (klp_target_state == KLP_PATCHED)
+		return;

This is not needed, I think. We call klp_cancel_transition() in __klp_enable_patch() only. klp_target_state is set to KLP_PATCHED there (through klp_init_transition()) and negated here. We know it must be KLP_UNPATCHED.

Moreover, due to klp_complete_transition() klp_target_state is always KLP_UNDEFINED after it.

Quoted 5 lines

kernel/livepatch/transition.c +5 -0

+
+	/*
+	 * In the enable error path, even immediate patches can be safely
+	 * removed because the transition hasn't been started yet.
+	 *

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 145

Quoted 10 lines

kernel/livepatch/transition.c +10 -0

+	 * klp_complete_transition() doesn't have a module_put() for immediate
+	 * patches, so do it here.
+	 */
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			if (func->immediate)
+				immediate_func = true;
+
+	if (klp_transition_patch->immediate || immediate_func)
+		module_put(klp_transition_patch->mod);

Almost correct. The only problem is that klp_transition_patch is NULL at this point. klp_complete_transition() does that and it should stay there in my opinion to keep it simple.

So we could either move all this to __klp_enable_patch(), where patch variable is defined, or we could store klp_transition_patch to a local variable here in klp_cancel_transition() before klp_complete_transition() is called. That should be safe. I like the latter more, because it keeps the code in klp_cancel_transition() where it belongs.

Quoted 17 lines · Miroslav Benes

Quoted 16 lines · Josh Poimboeuf

What do you think about the following? I tried to put the logic in klp_complete_transition(), so the module_put()'s would be in one place. But it was too messy, so I put it in klp_cancel_transition() instead.

kernel/livepatch/transition.c +2 -0

index e96346e..bd1c1fd 100644
@@ -121,8 +121,28 @@ static void klp_complete_transition(void)
  */
 void klp_cancel_transition(void)
 {
+	bool immediate_func = false;
+
 	klp_target_state = !klp_target_state;
 	klp_complete_transition();

Quoted 8 lines

kernel/livepatch/transition.c static void klp_complete_transition(void) · line 130

Quoted 3 lines

kernel/livepatch/transition.c +3 -0

+
+	if (klp_target_state == KLP_PATCHED)
+		return;

This is not needed, I think. We call klp_cancel_transition() in __klp_enable_patch() only. klp_target_state is set to KLP_PATCHED there (through klp_init_transition()) and negated here. We know it must be KLP_UNPATCHED.

Yeah, I was trying to hedge against the possibility of future code calling this function in the disable path. But that probably won't happen and it would probably be cleaner to just add a warning if klp_target_state isn't KLP_PATCHED.

Quoted 23 lines

Moreover, due to klp_complete_transition() klp_target_state is always KLP_UNDEFINED after it.

Quoted 5 lines

kernel/livepatch/transition.c +5 -0

+
+	/*
+	 * In the enable error path, even immediate patches can be safely
+	 * removed because the transition hasn't been started yet.
+	 *

kernel/livepatch/transition.c static void klp_complete_transition(void)

Quoted 10 lines

kernel/livepatch/transition.c +10 -0

+	 * klp_complete_transition() doesn't have a module_put() for immediate
+	 * patches, so do it here.
+	 */
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			if (func->immediate)
+				immediate_func = true;
+
+	if (klp_transition_patch->immediate || immediate_func)
+		module_put(klp_transition_patch->mod);

Almost correct. The only problem is that klp_transition_patch is NULL at this point. klp_complete_transition() does that and it should stay there in my opinion to keep it simple.

Quoted 5 lines

So we could either move all this to __klp_enable_patch(), where patch variable is defined, or we could store klp_transition_patch to a local variable here in klp_cancel_transition() before klp_complete_transition() is called. That should be safe. I like the latter more, because it keeps the code in klp_cancel_transition() where it belongs.

Good points. Here's another try:

kernel/livepatch/transition.c +15 -1

index e96346e..a23c63c 100644
@@ -121,8 +121,31 @@ static void klp_complete_transition(void)
  */
 void klp_cancel_transition(void)
 {
-	klp_target_state = !klp_target_state;
+	struct klp_patch *patch = klp_transition_patch;
+	struct klp_object *obj;
+	struct klp_func *func;
+	bool immediate_func = false;
+
+	if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
+		return;
+
+	klp_target_state = KLP_UNPATCHED;
 	klp_complete_transition();
+
+	/*
+	 * In the enable error path, even immediate patches can be safely
+	 * removed because the transition hasn't been started yet.
+	 *
+	 * klp_complete_transition() doesn't have a module_put() for immediate

Change livepatch to use a basic per-task consistency model. This is the foundation which will eventually enable us to patch those ~10% of security patches which change function or data semantics. This is the biggest remaining piece needed to make livepatch more generally useful.

This code stems from the design proposal made by Vojtech [1] in November 2014. It's a hybrid of kGraft and kpatch: it uses kGraft's per-task consistency and syscall barrier switching combined with kpatch's stack trace switching. There are also a number of fallback options which make it quite flexible.

Patches are applied on a per-task basis, when the task is deemed safe to switch over. When a patch is enabled, livepatch enters into a transition state where tasks are converging to the patched state. Usually this transition state can complete in a few seconds. The same sequence occurs when a patch is disabled, except the tasks converge from the patched state to the unpatched state.

An interrupt handler inherits the patched state of the task it interrupts. The same is true for forked tasks: the child inherits the patched state of the parent.

Livepatch uses several complementary approaches to determine when it's safe to patch tasks:

1. The first and most effective approach is stack checking of sleeping tasks. If no affected functions are on the stack of a given task, the task is patched. In most cases this will patch most or all of the tasks on the first try. Otherwise it'll keep trying periodically. This option is only available if the architecture has reliable stacks (HAVE_RELIABLE_STACKTRACE).

2. The second approach, if needed, is kernel exit switching. A task is switched when it returns to user space from a system call, a user space IRQ, or a signal. It's useful in the following cases:

a) Patching I/O-bound user tasks which are sleeping on an affected function. In this case you have to send SIGSTOP and SIGCONT to force it to exit the kernel and be patched.
b) Patching CPU-bound user tasks. If the task is highly CPU-bound then it will get patched the next time it gets interrupted by an IRQ.
c) In the future it could be useful for applying patches for architectures which don't yet have HAVE_RELIABLE_STACKTRACE. In this case you would have to signal most of the tasks on the system. However this isn't supported yet because there's currently no way to patch kthreads without HAVE_RELIABLE_STACKTRACE.

3. For idle "swapper" tasks, since they don't ever exit the kernel, they instead have a klp_update_patch_state() call in the idle loop which allows them to be patched before the CPU enters the idle state.

(Note there's not yet such an approach for kthreads.)

All the above approaches may be skipped by setting the 'immediate' flag in the 'klp_patch' struct, which will disable per-task consistency and patch all tasks immediately. This can be useful if the patch doesn't change any function or data semantics. Note that, even with this flag set, it's possible that some tasks may still be running with an old version of the function, until that function returns.

There's also an 'immediate' flag in the 'klp_func' struct which allows you to specify that certain functions in the patch can be applied without per-task consistency. This might be useful if you want to patch a common function like schedule(), and the function change doesn't need consistency but the rest of the patch does.

For architectures which don't have HAVE_RELIABLE_STACKTRACE, the user must set patch->immediate which causes all tasks to be patched immediately. This option should be used with care, only when the patch doesn't change any function or data semantics.

In the future, architectures which don't have HAVE_RELIABLE_STACKTRACE may be allowed to use per-task consistency if we can come up with another way to patch kthreads.

The /sys/kernel/livepatch/<patch>/transition file shows whether a patch is in transition. Only a single patch (the topmost patch on the stack) can be in transition at a given time. A patch can remain in transition indefinitely, if any of the tasks are stuck in the initial patch state.

A transition can be reversed and effectively canceled by writing the opposite value to the /sys/kernel/livepatch/<patch>/enabled file while the transition is in progress. Then all the tasks will attempt to converge back to the original patch state.

[1] https://lkml.kernel.org/r/[email protected]

Signed-off-by Josh Poimboeuf <[email protected]>

Change livepatch to use a basic per-task consistency model. This is the foundation which will eventually enable us to patch those ~10% of security patches which change function or data semantics. This is the biggest remaining piece needed to make livepatch more generally useful.

Signed-off-by Josh Poimboeuf <[email protected]>

kernel/sched/idle.c +1 -0

index 6a4bae0..a8b3f1a 100644
@@ -9,6 +9,7 @@
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
 #include <linux/suspend.h>
+#include <linux/livepatch.h>
 
 #include <asm/tlb.h>
 

Quoted 14 lines · Josh Poimboeuf

kernel/sched/idle.c +1 -0

index 6a4bae0..a8b3f1a 100644
@@ -9,6 +9,7 @@
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
 #include <linux/suspend.h>
+#include <linux/livepatch.h>
 
 #include <asm/tlb.h>
 

@@ -264,6 +265,9 @@ static void do_idle(void)
 

kernel/sched/idle.c static void do_idle(void) · line 271

Quoted 6 lines

kernel/sched/idle.c +3 -0

 	sched_ttwu_pending();
 	schedule_preempt_disabled();
+
+	if (unlikely(klp_patch_pending(current)))
+		klp_update_patch_state(current);
 }

Can someone explain this one? This is a very weird place to add things. What was the expectation?

Quoted 15 lines · Peter Zijlstra

Quoted 14 lines · Josh Poimboeuf

kernel/sched/idle.c +1 -0

index 6a4bae0..a8b3f1a 100644
@@ -9,6 +9,7 @@
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
 #include <linux/suspend.h>
+#include <linux/livepatch.h>
 
 #include <asm/tlb.h>
 

@@ -264,6 +265,9 @@ static void do_idle(void)
 

Quoted 9 lines

kernel/sched/idle.c static void do_idle(void) · line 271

Quoted 6 lines

kernel/sched/idle.c +3 -0

 	sched_ttwu_pending();
 	schedule_preempt_disabled();
+
+	if (unlikely(klp_patch_pending(current)))
+		klp_update_patch_state(current);
 }

Can someone explain this one? This is a very weird place to add things. What was the expectation?

AFAIK, it was the least ugly and minimalist solution that we came with.

The tasks are migrated to the new patch when neither of the patched functions is on the stack. The stack can be checked safely only when the task is not running. It might be very hard to catch the idle task on a such a place if we patch something that is used there.

If the idle task is scheduled, you would need to create some fake load on the system, try to migrate the idle task, stop the fake load on the CPU.

The above code makes the idle task to migrate itself on a sane place. You just need to schedule some minimalist job on the CPU. The idle task will do one loop, gets migrated, and might be scheduled again immediately.

Quoted 37 lines · Petr Mladek

Quoted 15 lines · Peter Zijlstra

Quoted 14 lines · Josh Poimboeuf

kernel/sched/idle.c +1 -0

index 6a4bae0..a8b3f1a 100644
@@ -9,6 +9,7 @@
 #include <linux/mm.h>
 #include <linux/stackprotector.h>
 #include <linux/suspend.h>
+#include <linux/livepatch.h>
 
 #include <asm/tlb.h>
 

@@ -264,6 +265,9 @@ static void do_idle(void)
 

Quoted 9 lines

kernel/sched/idle.c static void do_idle(void) · line 271

Quoted 6 lines

kernel/sched/idle.c +3 -0

 	sched_ttwu_pending();
 	schedule_preempt_disabled();
+
+	if (unlikely(klp_patch_pending(current)))
+		klp_update_patch_state(current);
 }

Can someone explain this one? This is a very weird place to add things. What was the expectation?

AFAIK, it was the least ugly and minimalist solution that we came with.

The tasks are migrated to the new patch when neither of the patched functions is on the stack. The stack can be checked safely only when the task is not running. It might be very hard to catch the idle task on a such a place if we patch something that is used there.

If the idle task is scheduled, you would need to create some fake load on the system, try to migrate the idle task, stop the fake load on the CPU.

Quoted 4 lines

The above code makes the idle task to migrate itself on a sane place. You just need to schedule some minimalist job on the CPU. The idle task will do one loop, gets migrated, and might be scheduled again immediately.

What I was getting at, the klp stuff is the very first thing we run when we schedule the idle task, but its placed at the very end of the function. This is confusing.

The above still doesn't help with solving that. Do you want to run something before we go idle, or before we leave idle, in neither cases would I place it where it is.

Quoted 3 lines

What I was getting at, the klp stuff is the very first thing we run when we schedule the idle task, but its placed at the very end of the function. This is confusing.

I see.

Quoted 3 lines

The above still doesn't help with solving that. Do you want to run something before we go idle, or before we leave idle, in neither cases would I place it where it is.

In fact, both ways are fine. We require going the idle task through the entire cycle anyway. It is because both situations, too long idling or non-idling, would block finishing the patch transition.

Feel free to move it right before schedule_idle() or __current_set_polling().

Currently we do not allow patch module to unload since there is no method to determine if a task is still running in the patched code.

The consistency model gives us the way because when the unpatching finishes we know that all tasks were marked as safe to call an original function. Thus every new call to the function calls the original code and at the same time no task can be somewhere in the patched code, because it had to leave that code to be marked as safe.

We can safely let the patch module go after that.

Completion is used for synchronization between module removal and sysfs infrastructure in a similar way to commit 942e443127e9 ("module: Fix mod->mkobj.kobj potentially freed too early").

Note that we still do not allow the removal for immediate model, that is no consistency model. The module refcount may increase in this case if somebody disables and enables the patch several times. This should not cause any harm.

With this change a call to try_module_get() is moved to __klp_enable_patch from klp_register_patch to make module reference counting symmetric (module_put() is in a patch disable path) and to allow to take a new reference to a disabled module when being enabled.

Finally, we need to be very careful about possible races between klp_unregister_patch(), kobject_put() functions and operations on the related sysfs files.

kobject_put(&patch->kobj) must be called without klp_mutex. Otherwise, it might be blocked by enabled_store() that needs the mutex as well. In addition, enabled_store() must check if the patch was not unregisted in the meantime.

There is no need to do the same for other kobject_put() callsites at the moment. Their sysfs operations neither take the lock nor they access any data that might be freed in the meantime.

There was an attempt to use kobjects the right way and prevent these races by design. But it made the patch definition more complicated and opened another can of worms. See
https://lkml.kernel.org/r/[email protected]

[Thanks to Petr Mladek for improving the commit message.]

Signed-off-by Miroslav Benes <[email protected]>

Signed-off-by Josh Poimboeuf <[email protected]>

Reviewed-by Petr Mladek <[email protected]>

---
v5.1: improve error handling in enable path -- call module_put() in
klp_cancel_transition()

Here's v5 of the consistency model, targeted for 4.12. Only a few minor changes this time.

v5:
- return -EINVAL in __save_stack_trace_reliable()
- only call show_stack() once
- add save_stack_trace_tsk_reliable() define for !CONFIG_STACKTRACE
- update kernel version and date in ABI doc
- make suggested improvements to livepatch.txt
- update barrier comments
- remove klp_try_complete_transition() call from klp_start_transition()
- move end of klp_try_complete_transition() into klp_complete_transition()
- fix __klp_enable_patch() error path
- check for transition in klp_module_going()

 arch/Kconfig                                     |   6 +
 arch/powerpc/include/asm/thread_info.h           |   4 +-
 arch/powerpc/kernel/signal.c                     |   4 +
 arch/s390/include/asm/thread_info.h              |  24 +-
 arch/s390/kernel/entry.S                         |  31 +-
 arch/x86/Kconfig                                 |   1 +
 arch/x86/entry/common.c                          |   9 +-
 arch/x86/include/asm/thread_info.h               |  13 +-
 arch/x86/include/asm/unwind.h                    |   6 +
 arch/x86/kernel/stacktrace.c                     |  96 +++-
 arch/x86/kernel/unwind_frame.c                   |   2 +

Here's v5 of the consistency model