livepatch: hybrid consistency model

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for x86_64. 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 placed in the _TIF_ALLWORK_MASK macro, which results in exit_to_usermode_loop() calling klp_update_patch_state() when it's set.

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

Dusting the cobwebs off the consistency model again.  This is based on linux-next/master.

v1 was posted on 2015-02-09:

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

v2 was posted on 2016-04-28:

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

The biggest issue from v2 was finding a decent way to detect preemption and page faults on the stack of a sleeping task.

Quoted 5 lines · Josh Poimboeuf

Dusting the cobwebs off the consistency model again.  This is based on linux-next/master.

v1 was posted on 2015-02-09:

Quoted 8 lines

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

v2 was posted on 2016-04-28:

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

The biggest issue from v2 was finding a decent way to detect preemption and page faults on the stack of a sleeping task.

Could you please elaborate on this? Preemption of a sleeping task and faults as in the future (time) preemption and faults?

Quoted 10 lines · Balbir Singh

Quoted 9 lines · Josh Poimboeuf

Dusting the cobwebs off the consistency model again. This is based on linux-next/master.

v1 was posted on 2015-02-09:

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

v2 was posted on 2016-04-28:

Quoted 7 lines

Quoted 4 lines

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

The biggest issue from v2 was finding a decent way to detect preemption and page faults on the stack of a sleeping task.

Could you please elaborate on this? Preemption of a sleeping task and faults as in the future (time) preemption and faults?

The normal way for a task to go to sleep is to call schedule(). objtool ensures the stack trace is reliable in that case, by making sure that all functions save the frame pointer on the stack before calling out to another function.

But a task can also go to sleep in a few other ways. One way is by preemption, where an interrupt handler interrupts the task and calls preempt_schedule_irq().

both cases, there's no guarantee that the interrupted function saved the frame pointer on the stack beforehand. So the stack trace might be unreliable. Fortunately, interrupts and exceptions leave evidence behind on the stack. So when walking the stack of a sleeping task, we can detect when an IRQ or exception occurred, and consider such a stack unreliable.

Quoted 24 lines · Josh Poimboeuf

Quoted 10 lines · Balbir Singh

Quoted 9 lines · Josh Poimboeuf

Dusting the cobwebs off the consistency model again. This is based on linux-next/master.

v1 was posted on 2015-02-09:

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

v2 was posted on 2016-04-28:

Quoted 7 lines

Quoted 4 lines

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

The biggest issue from v2 was finding a decent way to detect preemption and page faults on the stack of a sleeping task.

Could you please elaborate on this? Preemption of a sleeping task and faults as in the future (time) preemption and faults?

The normal way for a task to go to sleep is to call schedule(). objtool ensures the stack trace is reliable in that case, by making sure that all functions save the frame pointer on the stack before calling out to another function.

Quoted 3 lines

But a task can also go to sleep in a few other ways. One way is by preemption, where an interrupt handler interrupts the task and calls preempt_schedule_irq().

It's preempted, not sleeping. It's on_rq but not on_cpu.

Another way is by a page fault exception. In

Quoted 7 lines

both cases, there's no guarantee that the interrupted function saved the frame pointer on the stack beforehand. So the stack trace might be unreliable. Fortunately, interrupts and exceptions leave evidence behind on the stack. So when walking the stack of a sleeping task, we can detect when an IRQ or exception occurred, and consider such a stack unreliable.

Thanks for the explanation. I presume a whole lot of this is arch specific code? I'll look at the patches as well

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.

Scenarios which indicate that a stack trace may be unreliable:

- running task

- interrupt stack

- preemption

- corrupted stack data
- stack grows the wrong way

- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

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

arch/x86/kernel/stacktrace.c +9 -0

index 0653788..3e0cf5e 100644
@@ -74,6 +74,64 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
 
+#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
+static int __save_stack_trace_reliable(struct stack_trace *trace,
+				       struct task_struct *task)
+{
+	struct unwind_state state;
+	struct pt_regs *regs;
+	unsigned long addr;
+
+	for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state);

arch/x86/kernel/stacktrace.c +12 -0

+
+			/*
+			 * This frame contains the (user mode) pt_regs at the
+			 * end of the stack.  Finish the unwind.
+			 */
+			unwind_next_frame(&state);
+			break;
+		}
+
+		addr = unwind_get_return_address(&state);
+		if (!addr || save_stack_address(trace, addr, false))
+			return -1;

The _TIF_ALLWORK_MASK macro automatically includes the least-significant 16 bits of the thread_info flags, which is less than obvious and tends to create confusion and surprises when reading or modifying the code.

Define the flags explicitly.

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

---
 arch/x86/include/asm/thread_info.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

arch/x86/include/asm/thread_info.h +0 -0

index ad6f5eb0..1fe6043 100644

arch/x86/include/asm/thread_info.h +0 -2

- * - other flags in MSW
- * Warning: layout of LSW is hardcoded in entry.S
  */
 #define TIF_SYSCALL_TRACE	0	/* syscall trace active */
 #define TIF_NOTIFY_RESUME	1	/* callback before returning to user */

Create temporary stubs for klp_patch_pending() and klp_update_patch_state() so we can add TIF_PATCH_PENDING to different architectures in separate patches without breaking build bisectability.

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

---
 include/linux/livepatch.h | 7 ++++++-
 kernel/livepatch/core.c   | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

include/linux/livepatch.h +0 -0

index 9072f04..60558d8 100644

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for x86_64. 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 placed in the _TIF_ALLWORK_MASK macro, which results in exit_to_usermode_loop() calling klp_update_patch_state() when it's set.

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

Once we have a consistency model, patches and their objects will be enabled and disabled at different times. For example, when a patch is disabled, its loaded objects' funcs can remain registered with ftrace indefinitely until the unpatching operation is complete and they're no longer in use.

It's less confusing if we give them different names: patches can be enabled or disabled; objects (and their funcs) can be patched or unpatched:

- Enabled means that a patch is logically enabled (but not necessarily fully applied).

- Patched means that an object's funcs are registered with ftrace and added to the klp_ops func stack.

Also, since these states are binary, represent them with booleans instead of ints.

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

klp_patch_object()'s callers already ensure that the object is loaded, so its call to klp_is_object_loaded() is unnecessary.

This will also make it possible to move the patching code into a separate file.

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

Move functions related to the actual patching of functions and objects into a new patch.c file.

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

The sysfs enabled value is a boolean, so kstrtobool() is a better fit for parsing the input string since it does the range checking for us.

Suggested-by Petr Mladek <[email protected]>

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

---
 kernel/livepatch/core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

kernel/livepatch/core.c +0 -0

index 6a137e1..8ca8a0e 100644

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

Scenarios which indicate that a stack trace may be unreliable:

- running task

It seems that this has to be enforced by save_stack_trace_tsk_reliable() caller. It should be mentioned in the function description.

Quoted 1 line

- interrupt stack

I guess that it is detected by saved regs on the stack. And it covers also dynamic changes like kprobes. Do I get it correctly, please?

What about ftrace? Is ftrace without regs safe and detected?

Quoted 1 line

- preemption

I wonder if some very active kthreads might almost always be preempted using irq in preemptive kernel. Then they block the conversion with the non-reliable stacks. Have you noticed such problems, please?

Quoted 2 lines

- corrupted stack data
- stack grows the wrong way

This is detected in unwind_next_frame() and passed via state->error. Am I right?

Quoted 31 lines

- stack walk doesn't reach the bottom
- user didn't provide a large enough entries array

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

arch/x86/kernel/stacktrace.c +15 -0

index 0653788..3e0cf5e 100644
@@ -74,6 +74,64 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
 
+#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
+static int __save_stack_trace_reliable(struct stack_trace *trace,
+				       struct task_struct *task)
+{
+	struct unwind_state state;
+	struct pt_regs *regs;
+	unsigned long addr;
+
+	for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state);
+	     unwind_next_frame(&state)) {
+
+		regs = unwind_get_entry_regs(&state);
+		if (regs) {
+			/*
+			 * Preemption and page faults on the stack can make

arch/x86/kernel/stacktrace.c void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) · line 95

Quoted 4 lines

arch/x86/kernel/stacktrace.c +4 -0

+			 * frame pointers unreliable.
+			 */
+			if (!user_mode(regs))
+				return -1;

By other words, it we find regs on the stack, it almost always mean a non-reliable stack. The only exception is when we are in the userspace mode. Do I get it correctly, please?

Quoted 12 lines

arch/x86/kernel/stacktrace.c +12 -0

+
+			/*
+			 * This frame contains the (user mode) pt_regs at the
+			 * end of the stack.  Finish the unwind.
+			 */
+			unwind_next_frame(&state);
+			break;
+		}
+
+		addr = unwind_get_return_address(&state);
+		if (!addr || save_stack_address(trace, addr, false))
+			return -1;

arch/x86/kernel/stacktrace.c void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) · line 117

Quoted 10 lines

arch/x86/kernel/stacktrace.c +10 -0

+	}
+
+	if (!unwind_done(&state) || unwind_error(&state))
+		return -1;
+
+	if (trace->nr_entries < trace->max_entries)
+		trace->entries[trace->nr_entries++] = ULONG_MAX;
+
+	return 0;
+}

Great work! I am surprised that it looks so straightforward.

I still have to think and investigate it more. But it looks very promissing.

Best Regards,
Petr

Quoted 16 lines · Josh Poimboeuf

The _TIF_ALLWORK_MASK macro automatically includes the least-significant 16 bits of the thread_info flags, which is less than obvious and tends to create confusion and surprises when reading or modifying the code.

Define the flags explicitly.

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

---
 arch/x86/include/asm/thread_info.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

arch/x86/include/asm/thread_info.h +0 -0

index ad6f5eb0..1fe6043 100644

arch/x86/include/asm/thread_info.h struct thread_info { · line 76

Quoted 4 lines

arch/x86/include/asm/thread_info.h +0 -1

@@ -73,9 +73,6 @@ struct thread_info {
  * thread information flags
  * - these are process state flags that various assembly files
  *   may need to access
- * - pending work-to-be-done flags are in LSW

Yup, this is not true because also some flags from the most significant bits are in the _TIF_ALLWORK_MASK.

Quoted 7 lines

arch/x86/include/asm/thread_info.h +0 -2

- * - other flags in MSW
- * Warning: layout of LSW is hardcoded in entry.S
  */
 #define TIF_SYSCALL_TRACE	0	/* syscall trace active */
 #define TIF_NOTIFY_RESUME	1	/* callback before returning to user */

@@ -133,8 +130,10 @@ struct thread_info {
 

arch/x86/include/asm/thread_info.h struct thread_info { · line 136

Quoted 8 lines

arch/x86/include/asm/thread_info.h +4 -2

 /* work to do on any return to user space */
 #define _TIF_ALLWORK_MASK						\
-	((0x0000FFFF & ~_TIF_SECCOMP) | _TIF_SYSCALL_TRACEPOINT |	\
-	_TIF_NOHZ)
+	(_TIF_SYSCALL_TRACE | _TIF_NOTIFY_RESUME | _TIF_SIGPENDING |	\
+	 _TIF_SINGLESTEP | _TIF_NEED_RESCHED | _TIF_SYSCALL_EMU |	\
+	 _TIF_SYSCALL_AUDIT | _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE |	\
+	 _TIF_SYSCALL_TRACEPOINT | _TIF_NOHZ)

All flags are sorted by the number except for
_TIF_SINGLESTEP and _TIF_NEED_RESCHED ;-)

The patch does not change the existing behavior. The same existing flags are listed.

Reviewed-by Petr Mladek <[email protected]>

Best Regards,
Petr

Quoted 18 lines · Josh Poimboeuf

Create temporary stubs for klp_patch_pending() and klp_update_patch_state() so we can add TIF_PATCH_PENDING to different architectures in separate patches without breaking build bisectability.

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

---
 include/linux/livepatch.h | 7 ++++++-
 kernel/livepatch/core.c   | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

include/linux/livepatch.h +0 -0

index 9072f04..60558d8 100644
@@ -123,10 +123,15 @@ void arch_klp_init_object_loaded(struct klp_patch *patch,
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
 

include/linux/livepatch.h void arch_klp_init_object_loaded(struct klp_patch *patch, · line 126

Quoted 1 line

include/linux/livepatch.h +1 -0

+static inline bool klp_patch_pending(struct task_struct *task) { return false; }

I was curious about this. It is implemented correctly in the 13th patch and it is never used until 13th patch.

include/linux/livepatch.h void arch_klp_init_object_loaded(struct klp_patch *patch, · line 127

Quoted 2 lines

include/linux/livepatch.h +1 -0

+void klp_update_patch_state(struct task_struct *task);

It seems that the stub for this function is enough.

Well, the extra function is just a cosmetic problem. If it could be fixed, it would be great. But the patch makes sense:

Reviewed-by Petr Mladek <[email protected]>

Best Regards,
Petr

Quoted 20 lines · Josh Poimboeuf

The sysfs enabled value is a boolean, so kstrtobool() is a better fit for parsing the input string since it does the range checking for us.

Suggested-by Petr Mladek <[email protected]>

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

---
 kernel/livepatch/core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

kernel/livepatch/core.c +1 -1

index 6a137e1..8ca8a0e 100644
@@ -408,26 +408,23 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 {
 	struct klp_patch *patch;
 	int ret;
-	unsigned long val;
+	bool enabled;
 

kernel/livepatch/core.c static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, · line 415

Quoted 4 lines

kernel/livepatch/core.c +1 -1

-	ret = kstrtoul(buf, 10, &val);
+	ret = kstrtobool(buf, &enabled);
 	if (ret)
 		return -EINVAL;

I would return "ret" here. It is -EINVAL as well but... ;-)

Anyway, feel free to use:

Reviewed-by Petr Mladek <[email protected]>

Best Regards,
Petr

For the consistency model we'll need to know the sizes of the old and new functions to determine if they're on the stacks of any tasks.

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

The _TIF_ALLWORK_MASK macro automatically includes the least-significant 16 bits of the thread_info flags, which is less than obvious and tends to create confusion and surprises when reading or modifying the code.

Define the flags explicitly.

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

---
 arch/x86/include/asm/thread_info.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

arch/x86/include/asm/thread_info.h +4 -5

index ad6f5eb0..1fe6043 100644
@@ -73,9 +73,6 @@ struct thread_info {
  * thread information flags
  * - these are process state flags that various assembly files
  *   may need to access
- * - pending work-to-be-done flags are in LSW
- * - other flags in MSW
- * Warning: layout of LSW is hardcoded in entry.S
  */
 #define TIF_SYSCALL_TRACE	0	/* syscall trace active */
 #define TIF_NOTIFY_RESUME	1	/* callback before returning to user */

@@ -133,8 +130,10 @@ struct thread_info {
 
 /* work to do on any return to user space */
 #define _TIF_ALLWORK_MASK						\
-	((0x0000FFFF & ~_TIF_SECCOMP) | _TIF_SYSCALL_TRACEPOINT |	\
-	_TIF_NOHZ)
+	(_TIF_SYSCALL_TRACE | _TIF_NOTIFY_RESUME | _TIF_SIGPENDING |	\
+	 _TIF_SINGLESTEP | _TIF_NEED_RESCHED | _TIF_SYSCALL_EMU |	\
+	 _TIF_SYSCALL_AUDIT | _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE |	\
+	 _TIF_SYSCALL_TRACEPOINT | _TIF_NOHZ)
 
 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW							\

Signature · 1 line

2.7.4

arch/x86/Kconfig config X86 · line 158

Quoted 9 lines · Josh Poimboeuf

arch/x86/Kconfig +1 -0

index 215612c..b4a6663 100644
@@ -155,6 +155,7 @@ config X86
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_REGS_AND_STACK_ACCESS_API
+	select HAVE_RELIABLE_STACKTRACE		if X86_64 && FRAME_POINTER && STACK_VALIDATION

Tests to measure possible performance penalty of frame pointers were done by Mel Gorman. The outcome was quite clear. There IS a measurable impact. The percentage depends on the workflow but I think it is safe to say that FP usually takes 5-10 percents.

If my understanding is correct there is no single culprit. Register pressure is definitely not a problem. We ran simple benchmarks while taking a register away from GCC (RBP or a common one). The impact is a combination of more cacheline pressure, more memory accesses and the fact that the kernel contains a lot of small functions.

Thus, I think that DWARF should be the way to go here.

Other than that the patch looks good to me.

Quoted 23 lines · Miroslav Benes

arch/x86/Kconfig config X86 · line 158

Quoted 9 lines · Josh Poimboeuf

arch/x86/Kconfig +1 -0

index 215612c..b4a6663 100644
@@ -155,6 +155,7 @@ config X86
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_REGS_AND_STACK_ACCESS_API
+	select HAVE_RELIABLE_STACKTRACE		if X86_64 && FRAME_POINTER && STACK_VALIDATION

Tests to measure possible performance penalty of frame pointers were done by Mel Gorman. The outcome was quite clear. There IS a measurable impact. The percentage depends on the workflow but I think it is safe to say that FP usually takes 5-10 percents.

If my understanding is correct there is no single culprit. Register pressure is definitely not a problem. We ran simple benchmarks while taking a register away from GCC (RBP or a common one). The impact is a combination of more cacheline pressure, more memory accesses and the fact that the kernel contains a lot of small functions.

Quoted 3 lines

Thus, I think that DWARF should be the way to go here.

Other than that the patch looks good to me.

I agree that DWARF is generally a good idea, and I'm working toward it. However there's still quite a bit of work to get there.

For this consistency model to work with DWARF on x86, we would need:

1) a reliable x86 DWARF unwinder with Linus's blessing
2) objtool DWARF support (I'm working on this at the moment)
3) probably some kind of runtime NMI stack checking feature to complement objtool, along with a lot of burn time to ensure there are no issues, particularly in entry code
4) port save_stack_trace_tsk_reliable() to work with DWARF

DWARF will be nice to have, but it's definitely not required before merging this consistency model.

Also I doubt we'll ever be able to drop frame pointer support completely. Some embedded systems may not want the overhead of the DWARF metadata.

Quoted 23 lines · Miroslav Benes

arch/x86/Kconfig config X86 · line 158

Quoted 9 lines · Josh Poimboeuf

arch/x86/Kconfig +1 -0

index 215612c..b4a6663 100644
@@ -155,6 +155,7 @@ config X86
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_REGS_AND_STACK_ACCESS_API
+	select HAVE_RELIABLE_STACKTRACE		if X86_64 && FRAME_POINTER && STACK_VALIDATION

Tests to measure possible performance penalty of frame pointers were done by Mel Gorman. The outcome was quite clear. There IS a measurable impact. The percentage depends on the workflow but I think it is safe to say that FP usually takes 5-10 percents.

If my understanding is correct there is no single culprit. Register pressure is definitely not a problem. We ran simple benchmarks while taking a register away from GCC (RBP or a common one). The impact is a combination of more cacheline pressure, more memory accesses and the fact that the kernel contains a lot of small functions.

Quoted 3 lines

Thus, I think that DWARF should be the way to go here.

Other than that the patch looks good to me.

I agree that DWARF is generally a good idea, and I'm working toward it. However there's still quite a bit of work to get there.

For this consistency model to work with DWARF on x86, we would need:

1) a reliable x86 DWARF unwinder with Linus's blessing
2) objtool DWARF support (I'm working on this at the moment)
3) probably some kind of runtime NMI stack checking feature to complement objtool, along with a lot of burn time to ensure there are no issues, particularly in entry code

4) port save_stack_trace_tsk_reliable() to work with DWARF

include/linux/livepatch.h +2 -0

index 1a5a93c..8e06fe5 100644
@@ -28,18 +28,40 @@
 
 #include <asm/livepatch.h>
 
+/* task patch states */
+#define KLP_UNDEFINED	-1

include/linux/livepatch.h +3 -0

  * @old_addr:	the address of the function being patched
  * @kobj:	kobject for sysfs resources
  * @stack_node:	list node for klp_ops func_stack list
  * @old_size:	size of the old function
  * @new_size:	size of the new function
  * @patched:	the func has been added to the klp_ops list
+ * @transition:	the func is currently being applied or reverted
+ *

@@ -86,6 +110,7 @@ struct klp_object {
  * struct klp_patch - patch structure for live patching
  * @mod:	reference to the live patch module
  * @objs:	object entries for kernel objects to be patched
+ * @immediate:  patch all funcs immediately, bypassing safety mechanisms
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
  * @enabled:	the patch is enabled (but operation may be incomplete)

kernel/livepatch/core.c +1 -0

+	} else if (enabled) {
 		ret = __klp_enable_patch(patch);
 		if (ret)
 			goto err;

kernel/livepatch/patch.c +2 -0

index 5efa262..e79ebb5 100644
@@ -29,6 +29,7 @@
 #include <linux/bug.h>
 #include <linux/printk.h>
 #include "patch.h"
+#include "transition.h"
 
 static LIST_HEAD(klp_ops);
 

@@ -54,15 +55,53 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 {
 	struct klp_ops *ops;
 	struct klp_func *func;
+	int patch_state;
 

kernel/livepatch/patch.c +16 -0

+	/*
+	 * Enforce the order of the ops->func_stack and func->transition reads.
+	 * The corresponding write barrier is in __klp_enable_patch().
+	 */
+	smp_rmb();
+
+	if (unlikely(func->transition)) {
+
+		/*
+		 * Enforce the order of the func->transition and
+		 * current->patch_state reads.  Otherwise we could read an
+		 * out-of-date task state and pick the wrong function.  The
+		 * corresponding write barriers are in klp_init_transition()
+		 * and __klp_disable_patch().
+		 */
+		smp_rmb();

kernel/livepatch/patch.c +18 -0

+			 */
+			func = list_entry_rcu(func->stack_node.next,
+					      struct klp_func, stack_node);
+
+			if (&func->stack_node == &ops->func_stack)
+				goto unlock;
+		}
+	}
+
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	rcu_read_unlock();

@@ -211,3 +250,12 @@ int klp_patch_object(struct klp_object *obj)
 
 	return 0;
 }
+
+void klp_unpatch_objects(struct klp_patch *patch)
+{
+	struct klp_object *obj;
+
+	klp_for_each_object(patch, obj)
+		if (obj->patched)
+			klp_unpatch_object(obj);
+}

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

@@ -0,0 +1,479 @@
+/*
+ * transition.c - Kernel Live Patching transition functions
+ *
+ * Copyright (C) 2015-2016 Josh Poimboeuf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License

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

+#define MAX_STACK_ENTRIES 100
+
+struct klp_patch *klp_transition_patch;
+
+static int klp_target_state = KLP_UNDEFINED;
+
+/* called from copy_process() during fork */
+void klp_copy_process(struct task_struct *child)
+{
+	child->patch_state = current->patch_state;
+

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

+void klp_update_patch_state(struct task_struct *task)
+{
+	/*
+	 * The synchronize_rcu() call in klp_try_complete_transition() ensures
+	 * this critical section completes before the global patch transition
+	 * is considered complete so we don't have spurious patch_state updates
+	 * afterwards.
+	 */
+	rcu_read_lock();
+
+	/*
+	 * This test_and_clear_tsk_thread_flag() call also serves as a read
+	 * barrier to enforce the order of the TIF_PATCH_PENDING and
+	 * klp_target_state reads.  The corresponding write barriers are in
+	 * __klp_disable_patch() and klp_reverse_transition().
+	 */
+	if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
+		task->patch_state = READ_ONCE(klp_target_state);
+
+	rcu_read_unlock();
+}
+
+/*
+ * Initialize the global target patch state and all tasks to the initial patch
+ * state, and initialize all function transition states to true in preparation
+ * for patching or unpatching.
+ */
+void klp_init_transition(struct klp_patch *patch, int state)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+	struct klp_object *obj;
+	struct klp_func *func;
+	int initial_state = !state;
+
+	WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
+
+	klp_transition_patch = patch;
+
+	/*
+	 * Set the global target patch state which tasks will switch to.  This
+	 * has no effect until the TIF_PATCH_PENDING flags get set later.
+	 */
+	klp_target_state = state;
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (patch->immediate)
+		return;
+
+	/*
+	 * Initialize all tasks to the initial patch state to prepare them for
+	 * switching to the target state.
+	 */
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task) {
+		WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
+		task->patch_state = initial_state;
+	}
+	read_unlock(&tasklist_lock);
+

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

+	/*
+	 * Enforce the order of the task->patch_state initializations and the
+	 * func->transition updates to ensure that, in the enable path,
+	 * klp_ftrace_handler() doesn't see a func in transition with a
+	 * task->patch_state of KLP_UNDEFINED.
+	 */
+	smp_wmb();
+
+	/*
+	 * Set the func transition states so klp_ftrace_handler() will know to
+	 * switch to the transition logic.
+	 *
+	 * When patching, the funcs aren't yet in the func_stack and will be
+	 * made visible to the ftrace handler shortly by the calls to
+	 * klp_patch_object().
+	 *
+	 * When unpatching, the funcs are already in the func_stack and so are
+	 * already visible to the ftrace handler.
+	 */
+	klp_for_each_object(patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = true;
+}
+
+/*
+ * Start the transition to the specified target patch state so tasks can begin
+ * switching to it.
+ */
+void klp_start_transition(void)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
+
+	pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
+		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (klp_transition_patch->immediate)

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

+}
+
+/*
+ * The transition to the target patch state is complete.  Clean up the data
+ * structures.
+ */
+void klp_complete_transition(void)
+{
+	struct klp_object *obj;

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

+		task->patch_state = KLP_UNDEFINED;
+	}
+	put_online_cpus();
+
+done:
+	klp_target_state = KLP_UNDEFINED;
+	klp_transition_patch = NULL;
+}

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

+
+/*
+ * Try to switch all remaining tasks to the target patch state by walking the
+ * stacks of sleeping tasks and looking for any to-be-patched or
+ * to-be-unpatched functions.  If such functions are found, the task can't be
+ * switched yet.
+ *
+ * If any tasks are still stuck in the initial patch state, schedule a retry.
+ */
+bool klp_try_complete_transition(void)
+{
+	unsigned int cpu;
+	struct task_struct *g, *task;
+	bool complete = true;
+
+	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (klp_transition_patch->immediate)
+		goto success;
+
+	/*
+	 * Try to switch the tasks to the target patch state by walking their
+	 * stacks and looking for any to-be-patched or to-be-unpatched
+	 * functions.  If such functions are found on a stack, or if the stack
+	 * is deemed unreliable, the task can't be switched yet.
+	 *
+	 * Usually this will transition most (or all) of the tasks on a system
+	 * unless the patch includes changes to a very common function.
+	 */
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		if (!klp_try_switch_task(task))
+			complete = false;
+	read_unlock(&tasklist_lock);
+
+	/*
+	 * Ditto for the idle "swapper" tasks.
+	 */
+	get_online_cpus();
+	for_each_online_cpu(cpu)
+		if (!klp_try_switch_task(idle_task(cpu)))
+			complete = false;
+	put_online_cpus();
+
+	/*
+	 * Some tasks weren't able to be switched over.  Try again later and/or
+	 * wait for other methods like syscall barrier switching.
+	 */
+	if (!complete)
+		return false;
+
+success:
+
+	/*
+	 * When unpatching, all tasks have transitioned to KLP_UNPATCHED so we
+	 * can now remove the new functions from the func_stack.
+	 */
+	if (klp_target_state == KLP_UNPATCHED)

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

+	synchronize_rcu();
+
+	pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
+		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
+	/* we're done, now cleanup the data structures */
+	klp_complete_transition();
+
+	return true;
+}
+
+/*
+ * This function can be called in the middle of an existing transition to
+ * reverse the direction of the target patch state.  This can be done to
+ * effectively cancel an existing enable or disable operation if there are any
+ * tasks which are stuck in the initial patch state.
+ */
+void klp_reverse_transition(void)
+{

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

+
+	klp_start_transition();
+}
+

samples/livepatch/livepatch-sample.c +2 -0

index e34f871..bb61c65 100644
@@ -17,6 +17,8 @@
  * along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/livepatch.h>

Quoted 3 lines · Josh Poimboeuf

3) probably some kind of runtime NMI stack checking feature to complement objtool, along with a lot of burn time to ensure there are no issues, particularly in entry code

Could you please provide more details about this NMI stack checking? What is it supposed to protect that objtool could not? Will it run regularly or will it be just a random check?

Expose the per-task patch state value so users can determine which tasks are holding up completion of a patching operation.

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

Add the TIF_PATCH_PENDING thread flag to enable the new livepatch per-task consistency model for x86_64. 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 placed in the _TIF_ALLWORK_MASK macro, which results in exit_to_usermode_loop() calling klp_update_patch_state() when it's set.

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

From: Miroslav Benes <[email protected]>

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.

Also all kobject_put(&patch->kobj) calls are moved outside of klp_mutex lock protection to prevent a deadlock situation when klp_unregister_patch is called and sysfs directories are removed. is no need to do the same for other kobject_put() callsites as we currently do not have their sysfs counterparts.

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

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

From: Jiri Slaby <[email protected]>

Group the TIF thread flag bits by their inclusion in the _TIF_WORK and _TIF_TRACE macros.

Signed-off-by Jiri Slaby <[email protected]>

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

Documentation/livepatch/livepatch.txt

Quoted 13 lines · Josh Poimboeuf

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.

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

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

---

Documentation/livepatch/livepatch.txt +0 -0

index 6c43f6e..f87e742 100644

I like the description.

Just a note that we will also need to review the section about limitations. But I am not sure that we want to do it in this patch. It might open a long discussion on its own.

Quoted 8 lines

include/linux/livepatch.h +0 -0

index 1a5a93c..8e06fe5 100644
@@ -28,18 +28,40 @@
 
 #include <asm/livepatch.h>
 

include/linux/livepatch.h line 42

Quoted 12 lines

include/linux/livepatch.h +6 -0

+/* task patch states */
+#define KLP_UNDEFINED	-1
+#define KLP_UNPATCHED	 0
+#define KLP_PATCHED	 1
+
 /**
  * struct klp_func - function structure for live patching
  * @old_name:	name of the function to be patched
  * @new_func:	pointer to the patched function code
  * @old_sympos: a hint indicating which symbol position the old function
  *		can be found (optional)
+ * @immediate:  patch the func immediately, bypassing backtrace safety checks

There are more checks possible. I would use the same description as for klp_object.

Quoted 16 lines

include/linux/livepatch.h +3 -0

  * @old_addr:	the address of the function being patched
  * @kobj:	kobject for sysfs resources
  * @stack_node:	list node for klp_ops func_stack list
  * @old_size:	size of the old function
  * @new_size:	size of the new function
  * @patched:	the func has been added to the klp_ops list
+ * @transition:	the func is currently being applied or reverted
+ *

@@ -86,6 +110,7 @@ struct klp_object {
  * struct klp_patch - patch structure for live patching
  * @mod:	reference to the live patch module
  * @objs:	object entries for kernel objects to be patched
+ * @immediate:  patch all funcs immediately, bypassing safety mechanisms
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
  * @enabled:	the patch is enabled (but operation may be incomplete)

[...]

kernel/livepatch/core.c static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr, · line 482

Quoted 12 lines

kernel/livepatch/core.c +3 -1

index fc160c6..22c0c01 100644
@@ -424,7 +477,10 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 		goto err;
 	}
 
-	if (enabled) {
+	if (patch == klp_transition_patch) {
+		klp_reverse_transition();
+		mod_delayed_work(system_wq, &klp_transition_work, 0);

I would put this mod_delayed_work() into klp_reverse_transition(). Also I would put that schedule_delayed_work() into klp_try_complete_transition().

If I did not miss anything, it will allow to move the klp_transition_work code to transition.c where it logically belongs.

Quoted 4 lines

kernel/livepatch/core.c +1 -0

+	} else if (enabled) {
 		ret = __klp_enable_patch(patch);
 		if (ret)
 			goto err;

[...]

Quoted 18 lines

kernel/livepatch/patch.c +2 -0

index 5efa262..e79ebb5 100644
@@ -29,6 +29,7 @@
 #include <linux/bug.h>
 #include <linux/printk.h>
 #include "patch.h"
+#include "transition.h"
 
 static LIST_HEAD(klp_ops);
 

@@ -54,15 +55,53 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 {
 	struct klp_ops *ops;
 	struct klp_func *func;
+	int patch_state;
 

kernel/livepatch/patch.c static void notrace klp_ftrace_handler(unsigned long ip, · line 68

Quoted 10 lines

kernel/livepatch/patch.c +3 -1

 	ops = container_of(fops, struct klp_ops, fops);
 
 	rcu_read_lock();
+
 	func = list_first_or_null_rcu(&ops->func_stack, struct klp_func,
 				      stack_node);
-	if (WARN_ON_ONCE(!func))
+
+	if (!func)
 		goto unlock;

Why do you removed the WARN_ON_ONCE(), please?

We still add the function on the stack before registering the ftrace handler. Also we unregister the ftrace handler before removing the the last entry from the stack.

AFAIK, unregister_ftrace_function() calls rcu_synchronize()' to make sure that no-one is inside the handler once finished. Mirek knows more about it.

If this is not true, we have a problem. For example, we call kfree(ops) after unregister_ftrace_function();

BTW: I thought that this change was really needed because of klp_try_complete_transition(). But I think that the WARN could and should stay after all. See below.

Quoted 20 lines

kernel/livepatch/patch.c +20 -0

+	/*
+	 * Enforce the order of the ops->func_stack and func->transition reads.
+	 * The corresponding write barrier is in __klp_enable_patch().
+	 */
+	smp_rmb();
+
+	if (unlikely(func->transition)) {
+
+		/*
+		 * Enforce the order of the func->transition and
+		 * current->patch_state reads.  Otherwise we could read an
+		 * out-of-date task state and pick the wrong function.  The
+		 * corresponding write barriers are in klp_init_transition()
+		 * and __klp_disable_patch().
+		 */
+		smp_rmb();
+
+		patch_state = current->patch_state;
+
+		WARN_ON_ONCE(patch_state == KLP_UNDEFINED);

kernel/livepatch/patch.c static void notrace klp_ftrace_handler(unsigned long ip, · line 95

Quoted 6 lines

kernel/livepatch/patch.c +6 -0

+
+		if (patch_state == KLP_UNPATCHED) {
+			/*
+			 * Use the previously patched version of the function.
+			 * If no previous patches exist, use the original
+			 * function.

s/use the original/continue with the original/ ?

Quoted 48 lines

kernel/livepatch/patch.c +18 -0

+			 */
+			func = list_entry_rcu(func->stack_node.next,
+					      struct klp_func, stack_node);
+
+			if (&func->stack_node == &ops->func_stack)
+				goto unlock;
+		}
+	}
+
 	klp_arch_set_pc(regs, (unsigned long)func->new_func);
 unlock:
 	rcu_read_unlock();

@@ -211,3 +250,12 @@ int klp_patch_object(struct klp_object *obj)
 
 	return 0;
 }
+
+void klp_unpatch_objects(struct klp_patch *patch)
+{
+	struct klp_object *obj;
+
+	klp_for_each_object(patch, obj)
+		if (obj->patched)
+			klp_unpatch_object(obj);
+}

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

@@ -0,0 +1,479 @@
+/*
+ * transition.c - Kernel Live Patching transition functions
+ *
+ * Copyright (C) 2015-2016 Josh Poimboeuf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

kernel/livepatch/transition.c line 26

Quoted 6 lines

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

+
+#include <linux/cpu.h>
+#include <linux/stacktrace.h>
+#include "patch.h"
+#include "transition.h"
+#include "../sched/sched.h"

Is this acceptable for the scheduler guys?

Quoted 11 lines

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

+#define MAX_STACK_ENTRIES 100
+
+struct klp_patch *klp_transition_patch;
+
+static int klp_target_state = KLP_UNDEFINED;
+
+/* called from copy_process() during fork */
+void klp_copy_process(struct task_struct *child)
+{
+	child->patch_state = current->patch_state;
+

kernel/livepatch/transition.c line 48

Quoted 10 lines

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

+	/* TIF_PATCH_PENDING gets copied in setup_thread_stack() */
+}
+
+/*
+ * klp_update_patch_state() - change the patched state of a task
+ * @task:	The task to change
+ *
+ * Switches the patched state of the task to the set of functions in the target
+ * patch state.
+ */

Please, add here some warning. Something like:

* This function must never be called in parallel with
* klp_ftrace_handler(). Otherwise, the handler might do random
* decisions and break the consistency.
*
* By other words, call this function only by the @task itself
* or make sure that it is not running.

Quoted 70 lines

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

+void klp_update_patch_state(struct task_struct *task)
+{
+	/*
+	 * The synchronize_rcu() call in klp_try_complete_transition() ensures
+	 * this critical section completes before the global patch transition
+	 * is considered complete so we don't have spurious patch_state updates
+	 * afterwards.
+	 */
+	rcu_read_lock();
+
+	/*
+	 * This test_and_clear_tsk_thread_flag() call also serves as a read
+	 * barrier to enforce the order of the TIF_PATCH_PENDING and
+	 * klp_target_state reads.  The corresponding write barriers are in
+	 * __klp_disable_patch() and klp_reverse_transition().
+	 */
+	if (test_and_clear_tsk_thread_flag(task, TIF_PATCH_PENDING))
+		task->patch_state = READ_ONCE(klp_target_state);
+
+	rcu_read_unlock();
+}
+
+/*
+ * Initialize the global target patch state and all tasks to the initial patch
+ * state, and initialize all function transition states to true in preparation
+ * for patching or unpatching.
+ */
+void klp_init_transition(struct klp_patch *patch, int state)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+	struct klp_object *obj;
+	struct klp_func *func;
+	int initial_state = !state;
+
+	WARN_ON_ONCE(klp_target_state != KLP_UNDEFINED);
+
+	klp_transition_patch = patch;
+
+	/*
+	 * Set the global target patch state which tasks will switch to.  This
+	 * has no effect until the TIF_PATCH_PENDING flags get set later.
+	 */
+	klp_target_state = state;
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (patch->immediate)
+		return;
+
+	/*
+	 * Initialize all tasks to the initial patch state to prepare them for
+	 * switching to the target state.
+	 */
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task) {
+		WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);
+		task->patch_state = initial_state;
+	}
+	read_unlock(&tasklist_lock);
+
+	/*
+	 * Ditto for the idle "swapper" tasks.
+	 */
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		task = idle_task(cpu);
+		WARN_ON_ONCE(task->patch_state != KLP_UNDEFINED);

kernel/livepatch/transition.c line 121

Quoted 3 lines

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

+		task->patch_state = initial_state;
+	}
+	put_online_cpus();

We allow to add/remove CPUs here. I am afraid that we will also need to add a cpu coming/going handler that will set the task->patch_state the right way. We must not set the klp_target_state until all ftrace handlers are ready.

Quoted 43 lines

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

+	/*
+	 * Enforce the order of the task->patch_state initializations and the
+	 * func->transition updates to ensure that, in the enable path,
+	 * klp_ftrace_handler() doesn't see a func in transition with a
+	 * task->patch_state of KLP_UNDEFINED.
+	 */
+	smp_wmb();
+
+	/*
+	 * Set the func transition states so klp_ftrace_handler() will know to
+	 * switch to the transition logic.
+	 *
+	 * When patching, the funcs aren't yet in the func_stack and will be
+	 * made visible to the ftrace handler shortly by the calls to
+	 * klp_patch_object().
+	 *
+	 * When unpatching, the funcs are already in the func_stack and so are
+	 * already visible to the ftrace handler.
+	 */
+	klp_for_each_object(patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = true;
+}
+
+/*
+ * Start the transition to the specified target patch state so tasks can begin
+ * switching to it.
+ */
+void klp_start_transition(void)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
+
+	pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
+		  klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (klp_transition_patch->immediate)

kernel/livepatch/transition.c line 175

Quoted 10 lines

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

+		return;
+
+	/*
+	 * Mark all normal tasks as needing a patch state update.  As they pass
+	 * through the syscall barrier they'll switch over to the target state
+	 * (unless we switch them in klp_try_complete_transition() first).
+	 */
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		set_tsk_thread_flag(task, TIF_PATCH_PENDING);

This is called also from klp_reverse_transition(). We should set it only when the task need migration. Also we should clear it when the task is in the right state already.

It is not only optimization. It actually solves a race between klp_complete_transition() and klp_update_patch_state(), see below.

Quoted 5 lines

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

+	read_unlock(&tasklist_lock);
+
+	/*
+	 * Ditto for the idle "swapper" tasks, though they never cross the
+	 * syscall barrier.  Instead they switch over in cpu_idle_loop().

kernel/livepatch/transition.c line 185

Quoted 5 lines

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

+	 */
+	get_online_cpus();
+	for_each_online_cpu(cpu)
+		set_tsk_thread_flag(idle_task(cpu), TIF_PATCH_PENDING);
+	put_online_cpus();

Also this stage need to be somehow handled by CPU coming/going handlers.

Quoted 9 lines

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

+}
+
+/*
+ * The transition to the target patch state is complete.  Clean up the data
+ * structures.
+ */
+void klp_complete_transition(void)
+{
+	struct klp_object *obj;

kernel/livepatch/transition.c line 204

Quoted 10 lines

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

+	struct klp_func *func;
+	struct task_struct *g, *task;
+	unsigned int cpu;
+
+	if (klp_transition_patch->immediate)
+		goto done;
+
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = false;

We should call rcu_synchronize() here. Otherwise, there might be a race, see below:

CPU1 CPU2

klp_ftrace_handler()
if (unlikely(func->transition))

	// still true

					klp_complete_transition()
					  func->transition = false;
					  task->patch_state =
					      KLP_UNDEFINED;

     patch_state = current->patch_state;

     WARN_ON(patch_state == KLP_UNDEFINED);

BANG!: We print the warning.

Note that that smp_wmb() is enough in klp_init_transition() but it is not enough here. We need to wait longer once someone might be inside the if (true) code.

kernel/livepatch/transition.c line 216

Quoted 11 lines

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

+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task) {
+		clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
+		task->patch_state = KLP_UNDEFINED;
+	}
+	read_unlock(&tasklist_lock);
+
+	get_online_cpus();
+	for_each_online_cpu(cpu) {
+		task = idle_task(cpu);
+		clear_tsk_thread_flag(task, TIF_PATCH_PENDING);

If TIF_PATCH_PENDING flag is set here it means that klp_update_patch_state() might get triggered and it might put wrong value into task->patch_state.

We must make sure that all task have this cleared before calling this function. This is another reason why klp_init_transition() should set the flag only when transition is needed.