Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1364815 > unrolled thread

[RFC PATCH v1.9 00/14] livepatch: hybrid consistency model

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
    [RFC PATCH v1.9 03/14] x86/asm/head: standardize the bottom of the stack for idle tasks Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
    [RFC PATCH v1.9 09/14] livepatch: remove unnecessary object loaded check Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
    [RFC PATCH v1.9 11/14] livepatch: store function sizes Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
    [RFC PATCH v1.9 13/14] livepatch: add /proc/<pid>/patch_status Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
      Re: [RFC PATCH v1.9 13/14] livepatch: add /proc/<pid>/patch_status Jiri Slaby <jslaby@suse.cz> - 2016-03-31 11:40 +0200
        Re: [RFC PATCH v1.9 13/14] livepatch: add /proc/<pid>/patch_status Jiri Slaby <jslaby@suse.cz> - 2016-03-31 11:50 +0200
    [RFC PATCH v1.9 02/14] x86/asm/head: use a common function for starting CPUs Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
    [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting reliable stack traces Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
      Re: [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting  reliable stack traces Miroslav Benes <mbenes@suse.cz> - 2016-03-31 15:10 +0200
    [RFC PATCH v1.9 12/14] livepatch: create per-task consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
      Re: [RFC PATCH v1.9 12/14] livepatch: create per-task consistency  model Miroslav Benes <mbenes@suse.cz> - 2016-03-31 15:20 +0200
    [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched states Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
    [RFC PATCH v1.9 04/14] x86: move _stext marker before head code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-03-25 20:40 +0100
    Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model Miroslav Benes <mbenes@suse.cz> - 2016-03-31 15:00 +0200
    Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model Petr Mladek <pmladek@suse.com> - 2016-04-01 15:40 +0200
      Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model Petr Mladek <pmladek@suse.com> - 2016-04-01 17:40 +0200
    Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model Miroslav Benes <mbenes@suse.cz> - 2016-04-01 15:40 +0200

#1364815 — [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model

These patches are still a work in progress, but Jiri asked that I share
them before I go on vacation next week.  Based on origin/master because
it has CONFIG_STACK_VALIDATION.

This has two consistency models: the immediate model (like in today's
code) and the new kpatch/kgraft hybrid model.

The default is the hybrid model: 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 pretty
flexible, yet the code is still quite simple.

Patches are applied on a per-task basis, when the task is deemed safe to
switch over.  It uses a tiered approach to determine when a task is safe
and can be switched.

The first wave of attack is stack checking of sleeping tasks.  If no
affected functions are on the stack of a given task, the task is
switched.  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
(CONFIG_RELIABLE_STACKTRACE and CONFIG_STACK_VALIDATION).

The next line of attack, if needed, is syscall/IRQ switching.  A task is
switched when it returns from a system call or a user space IRQ.  This
approach is less than ideal because it usually requires signaling tasks
to get them to switch.  It also doesn't work for kthreads.  But it's
still useful in some cases when user tasks get stuck sleeping on an
affected function.

For architectures which don't have reliable stacks, users have two
options:

a) use the hybrid fallback option of using only the syscall/IRQ
   switching (which is the default);

b) or they can use the immediate model (which is the model we already
   have today) by setting the patch->immediate flag.

There's also a func->immediate flag which allows users 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.

Still have a lot of TODOs, some of them are listed here.  If you see
something objectionable, it might be a good idea to make sure it's not
already on the TODO list :-)

TODO:
- actually test it
- don't use TIP_KLP_NEED_UPDATE in arch-independent code
- figure out new API to keep the use of task_rq_lock() in the sched code
- cleaner way to detect preemption on the stack
- allow patch modules to be removed.  still needs more discussion and
  thought.  maybe something like Miroslav's patch would be good:
  https://lkml.kernel.org/r/alpine.LNX.2.00.1512150857510.24899@pobox.suse.cz
- come up with a better name than universe?  KLP_STATE_PREV/NEXT?
  KLP_UNPATCHED/PATCHED?  there were some objections to the name in v1.
- update documentation for sysfs, proc, livepatch
- need atomic accesses or READ_ONCE/WRITE_ONCE anywhere?
- ability to force a patch to the goal universe
- try ftrace handler switching idea from v1 cover letter
- split up the patches better
- cc all the right people

v1.9 changes:
- revive from the dead and rebased
- reliable stacks!
- add support for immediate consistency model
- add a ton of comments
- fix up memory barriers
- remove "allow patch modules to be removed" patch for now, it still 
  needs more discussion and thought - it can be done with something
- "proc/pid/universe" -> "proc/pid/patch_status"
- remove WARN_ON_ONCE from !func condition in ftrace handler -- can
  happen because of RCU
- keep klp_mutex private by putting the work_fn in core.c
- convert states from int to boolean
- remove obsolete '@state' comments
- several header file and include improvements suggested by Jiri S
- change kallsyms_lookup_size_offset() errors from EINVAL -> ENOENT
- change proc file permissions S_IRUGO -> USR
- use klp_for_each_object/func helpers

v1 was here:
https://lkml.kernel.org/r/cover.1423499826.git.jpoimboe@redhat.com

Josh Poimboeuf (14):
  x86/asm/head: cleanup initial stack variable
  x86/asm/head: use a common function for starting CPUs
  x86/asm/head: standardize the bottom of the stack for idle tasks
  x86: move _stext marker before head code
  sched: horrible way to detect whether a task has been preempted
  x86: add error handling to dump_trace()
  x86/stacktrace: add function for detecting reliable stack traces
  livepatch: separate enabled and patched states
  livepatch: remove unnecessary object loaded check
  livepatch: move patching functions into patch.c
  livepatch: store function sizes
  livepatch: create per-task consistency model
  livepatch: add /proc/<pid>/patch_status
  livepatch: update task universe when exiting kernel

 arch/Kconfig                       |   6 +
 arch/x86/Kconfig                   |   1 +
 arch/x86/entry/common.c            |   6 +-
 arch/x86/include/asm/realmode.h    |   2 +-
 arch/x86/include/asm/smp.h         |   3 -
 arch/x86/include/asm/stacktrace.h  |  36 +--
 arch/x86/include/asm/thread_info.h |   2 +
 arch/x86/kernel/acpi/sleep.c       |   2 +-
 arch/x86/kernel/dumpstack.c        |  67 ++++--
 arch/x86/kernel/dumpstack_32.c     |  22 +-
 arch/x86/kernel/dumpstack_64.c     |  53 +++--
 arch/x86/kernel/head_32.S          |   8 +-
 arch/x86/kernel/head_64.S          |  35 ++-
 arch/x86/kernel/smpboot.c          |   2 +-
 arch/x86/kernel/stacktrace.c       |  32 +++
 arch/x86/kernel/vmlinux.lds.S      |   2 +-
 fs/proc/base.c                     |  12 +
 include/linux/livepatch.h          |  49 ++++-
 include/linux/sched.h              |  14 +-
 include/linux/stacktrace.h         |  20 +-
 kernel/fork.c                      |   2 +
 kernel/livepatch/Makefile          |   2 +-
 kernel/livepatch/core.c            | 316 +++++++++------------------
 kernel/livepatch/patch.c           | 226 +++++++++++++++++++
 kernel/livepatch/patch.h           |  33 +++
 kernel/livepatch/transition.c      | 435 +++++++++++++++++++++++++++++++++++++
 kernel/livepatch/transition.h      |  20 ++
 kernel/sched/core.c                |  28 ++-
 kernel/sched/idle.c                |   4 +
 kernel/stacktrace.c                |   4 +-
 lib/Kconfig.debug                  |   6 +
 31 files changed, 1135 insertions(+), 315 deletions(-)
 create mode 100644 kernel/livepatch/patch.c
 create mode 100644 kernel/livepatch/patch.h
 create mode 100644 kernel/livepatch/transition.c
 create mode 100644 kernel/livepatch/transition.h

-- 
2.4.3

[toc] | [next] | [standalone]

#1364816 — [RFC PATCH v1.9 03/14] x86/asm/head: standardize the bottom of the stack for idle tasks

Thanks to all the recent x86 entry code refactoring, most tasks' kernel
stacks start at the same offset right above their saved pt_regs,
regardless of which syscall was used to enter the kernel.  That creates
a useful convention which makes it straightforward to identify the
"bottom" of the stack, which can be useful for stack walking code which
needs to verify the stack is sane.

However there are still a few types of tasks which don't yet follow that
convention:

1) CPU idle tasks, aka the "swapper" tasks

2) freshly forked tasks which haven't run yet and have TIF_FORK set

Make the idle tasks conform to the new stack bottom convention by
starting their stack at a sizeof(pt_regs) offset from the end of the
stack page.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/head_64.S | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index f2deae7..255fe54 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -20,6 +20,7 @@
 #include <asm/processor-flags.h>
 #include <asm/percpu.h>
 #include <asm/nops.h>
+#include "../entry/calling.h"
 
 #ifdef CONFIG_PARAVIRT
 #include <asm/asm-offsets.h>
@@ -287,8 +288,9 @@ ENTRY(start_cpu)
 	 *	REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
 	 *		address given in m16:64.
 	 */
-	movq	initial_code(%rip),%rax
-	pushq	$0		# fake return address to stop unwinder
+	call	1f		# put return address on stack for unwinder
+1:	xorq	%rbp, %rbp	# clear frame pointer
+	movq	initial_code(%rip), %rax
 	pushq	$__KERNEL_CS	# set correct cs
 	pushq	%rax		# target address in negative space
 	lretq
@@ -316,7 +318,7 @@ ENDPROC(start_cpu0)
 	GLOBAL(initial_gs)
 	.quad	INIT_PER_CPU_VAR(irq_stack_union)
 	GLOBAL(initial_stack)
-	.quad  init_thread_union+THREAD_SIZE-8
+	.quad  init_thread_union + THREAD_SIZE - SIZEOF_PTREGS
 	__FINITDATA
 
 bad_address:
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1364818 — [RFC PATCH v1.9 09/14] livepatch: remove unnecessary object loaded check

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 <jpoimboe@redhat.com>
---
 kernel/livepatch/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index be1e106..1f70500 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -402,9 +402,6 @@ static int klp_patch_object(struct klp_object *obj)
 	if (WARN_ON(obj->patched))
 		return -EINVAL;
 
-	if (WARN_ON(!klp_is_object_loaded(obj)))
-		return -EINVAL;
-
 	klp_for_each_func(obj, func) {
 		ret = klp_patch_func(func);
 		if (ret) {
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1364819 — [RFC PATCH v1.9 11/14] livepatch: store function sizes

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

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 include/linux/livepatch.h |  3 +++
 kernel/livepatch/core.c   | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 6d45dc7..dd5db74 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -37,6 +37,8 @@
  * @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
  */
 struct klp_func {
@@ -56,6 +58,7 @@ struct klp_func {
 	unsigned long old_addr;
 	struct kobject kobj;
 	struct list_head stack_node;
+	unsigned long old_size, new_size;
 	bool patched;
 };
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index beaf263..b0fd31d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -540,6 +540,22 @@ static int klp_init_object_loaded(struct klp_patch *patch,
 					     &func->old_addr);
 		if (ret)
 			return ret;
+
+		ret = kallsyms_lookup_size_offset(func->old_addr,
+						  &func->old_size, NULL);
+		if (!ret) {
+			pr_err("kallsyms size lookup failed for '%s'\n",
+			       func->old_name);
+			return -ENOENT;
+		}
+
+		ret = kallsyms_lookup_size_offset((unsigned long)func->new_func,
+						  &func->new_size, NULL);
+		if (!ret) {
+			pr_err("kallsyms size lookup failed for '%s' replacement\n",
+			       func->old_name);
+			return -ENOENT;
+		}
 	}
 
 	return 0;
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1364820 — [RFC PATCH v1.9 13/14] livepatch: add /proc/<pid>/patch_status

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

Call it "patch_status" rather than "universe": it's at least more
descriptive for the average user.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 fs/proc/base.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index b1755b2..f90998b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2801,6 +2801,15 @@ static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
 	return err;
 }
 
+#ifdef CONFIG_LIVEPATCH
+static int proc_pid_patch_status(struct seq_file *m, struct pid_namespace *ns,
+				 struct pid *pid, struct task_struct *task)
+{
+	seq_printf(m, "%d\n", task->klp_universe);
+	return 0;
+}
+#endif /* CONFIG_LIVEPATCH */
+
 /*
  * Thread groups
  */
@@ -2900,6 +2909,9 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("timers",	  S_IRUGO, proc_timers_operations),
 #endif
 	REG("timerslack_ns", S_IRUGO|S_IWUGO, proc_pid_set_timerslack_ns_operations),
+#ifdef CONFIG_LIVEPATCH
+	ONE("patch_status",  S_IRUSR, proc_pid_patch_status),
+#endif
 };
 
 static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1368101 — Re: [RFC PATCH v1.9 13/14] livepatch: add /proc/<pid>/patch_status

On 03/25/2016, 08:35 PM, Josh Poimboeuf wrote:
> Expose the per-task klp_universe value so users can determine which
> tasks are holding up completion of a patching operation.
> 
> Call it "patch_status" rather than "universe": it's at least more
> descriptive for the average user.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  fs/proc/base.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index b1755b2..f90998b 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2801,6 +2801,15 @@ static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
>  	return err;
>  }
>  
> +#ifdef CONFIG_LIVEPATCH
> +static int proc_pid_patch_status(struct seq_file *m, struct pid_namespace *ns,
> +				 struct pid *pid, struct task_struct *task)
> +{
> +	seq_printf(m, "%d\n", task->klp_universe);
> +	return 0;
> +}
> +#endif /* CONFIG_LIVEPATCH */
> +
>  /*
>   * Thread groups
>   */
> @@ -2900,6 +2909,9 @@ static const struct pid_entry tgid_base_stuff[] = {
>  	REG("timers",	  S_IRUGO, proc_timers_operations),
>  #endif
>  	REG("timerslack_ns", S_IRUGO|S_IWUGO, proc_pid_set_timerslack_ns_operations),
> +#ifdef CONFIG_LIVEPATCH
> +	ONE("patch_status",  S_IRUSR, proc_pid_patch_status),
> +#endif

I think we want to know the state about all tasks, not only group
leaders. This should go to tid_base_stuff.

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1368113 — Re: [RFC PATCH v1.9 13/14] livepatch: add /proc/<pid>/patch_status

On 03/31/2016, 11:33 AM, Jiri Slaby wrote:
> On 03/25/2016, 08:35 PM, Josh Poimboeuf wrote:
>> Expose the per-task klp_universe value so users can determine which
>> tasks are holding up completion of a patching operation.
>>
>> Call it "patch_status" rather than "universe": it's at least more
>> descriptive for the average user.
>>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> ---
>>  fs/proc/base.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index b1755b2..f90998b 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2801,6 +2801,15 @@ static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
>>  	return err;
>>  }
>>  
>> +#ifdef CONFIG_LIVEPATCH
>> +static int proc_pid_patch_status(struct seq_file *m, struct pid_namespace *ns,
>> +				 struct pid *pid, struct task_struct *task)
>> +{
>> +	seq_printf(m, "%d\n", task->klp_universe);
>> +	return 0;
>> +}
>> +#endif /* CONFIG_LIVEPATCH */
>> +
>>  /*
>>   * Thread groups
>>   */
>> @@ -2900,6 +2909,9 @@ static const struct pid_entry tgid_base_stuff[] = {
>>  	REG("timers",	  S_IRUGO, proc_timers_operations),
>>  #endif
>>  	REG("timerslack_ns", S_IRUGO|S_IWUGO, proc_pid_set_timerslack_ns_operations),
>> +#ifdef CONFIG_LIVEPATCH
>> +	ONE("patch_status",  S_IRUSR, proc_pid_patch_status),
>> +#endif
> 
> I think we want to know the state about all tasks, not only group
> leaders. This should go to tid_base_stuff.

Or loop over all tasks here by for_each_thread(task, t) and check them all.

> thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1364821 — [RFC PATCH v1.9 02/14] x86/asm/head: use a common function for starting CPUs

There are two different pieces of code for starting a CPU: start_cpu0()
and the end of secondary_startup_64().  They're identical except for the
stack setup.  Combine the common parts into a shared start_cpu()
function.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/head_64.S | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 278270e..f2deae7 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -255,13 +255,15 @@ ENTRY(secondary_startup_64)
 	movl	$MSR_GS_BASE,%ecx
 	movl	initial_gs(%rip),%eax
 	movl	initial_gs+4(%rip),%edx
-	wrmsr	
+	wrmsr
 
 	/* rsi is pointer to real mode structure with interesting info.
 	   pass it to C */
 	movq	%rsi, %rdi
-	
-	/* Finally jump to run C code and to be on real kernel address
+
+ENTRY(start_cpu)
+	/*
+	 * Jump to run C code and to be on a real kernel address.
 	 * Since we are running on identity-mapped space we have to jump
 	 * to the full 64bit address, this is only possible as indirect
 	 * jump.  In addition we need to ensure %cs is set so we make this
@@ -290,6 +292,7 @@ ENTRY(secondary_startup_64)
 	pushq	$__KERNEL_CS	# set correct cs
 	pushq	%rax		# target address in negative space
 	lretq
+ENDPROC(start_cpu)
 
 #include "verify_cpu.S"
 
@@ -297,15 +300,11 @@ ENTRY(secondary_startup_64)
 /*
  * Boot CPU0 entry point. It's called from play_dead(). Everything has been set
  * up already except stack. We just set up stack here. Then call
- * start_secondary().
+ * start_secondary() via start_cpu().
  */
 ENTRY(start_cpu0)
-	movq initial_stack(%rip),%rsp
-	movq	initial_code(%rip),%rax
-	pushq	$0		# fake return address to stop unwinder
-	pushq	$__KERNEL_CS	# set correct cs
-	pushq	%rax		# target address in negative space
-	lretq
+	movq	initial_stack(%rip), %rsp
+	jmp	start_cpu
 ENDPROC(start_cpu0)
 #endif
 
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1364822 — [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting reliable stack traces

For live patching and possibly other use cases, a stack trace is only
useful if you 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 strace may be unreliable:

- interrupt stacks
- preemption
- corrupted stack data
- newly forked tasks
- running tasks
- the user didn't provide a large enough entries array

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

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/Kconfig                 |  6 ++++++
 arch/x86/Kconfig             |  1 +
 arch/x86/kernel/dumpstack.c  | 36 ++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/stacktrace.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/stacktrace.h   | 20 ++++++++++++++++----
 kernel/stacktrace.c          |  4 ++--
 lib/Kconfig.debug            |  6 ++++++
 7 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 81869a5..68b95f1 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -589,6 +589,12 @@ config HAVE_STACK_VALIDATION
 	  Architecture supports the 'objtool check' host tool command, which
 	  performs compile-time stack metadata validation.
 
+config HAVE_RELIABLE_STACKTRACE
+	bool
+	help
+	  Architecure has a save_stack_trace_tsk_reliable() function which only
+	  returns a stack trace if it can guarantee the trace is reliable.
+
 #
 # ABI hall of shame
 #
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2dc18605..76274b8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -138,6 +138,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
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UID16			if X86_32 || IA32_EMULATION
 	select HAVE_UNSTABLE_SCHED_CLOCK
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 3b10518..9c68bfc 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -145,6 +145,42 @@ int print_context_stack_bp(struct thread_info *tinfo,
 }
 EXPORT_SYMBOL_GPL(print_context_stack_bp);
 
+int print_context_stack_reliable(struct thread_info *tinfo,
+				 unsigned long *stack, unsigned long *bp,
+				 const struct stacktrace_ops *ops,
+				 void *data, unsigned long *end, int *graph)
+{
+	struct stack_frame *frame = (struct stack_frame *)*bp;
+	struct stack_frame *last_frame = frame;
+	unsigned long *ret_addr = &frame->return_address;
+
+	if (test_ti_thread_flag(tinfo, TIF_FORK))
+		return -EINVAL;
+
+	while (valid_stack_ptr(tinfo, ret_addr, sizeof(*ret_addr), end)) {
+		unsigned long addr = *ret_addr;
+
+		if (frame <= last_frame || !__kernel_text_address(addr) ||
+		    in_preempt_schedule_irq(addr))
+			return -EINVAL;
+
+		if (ops->address(data, addr, 1))
+			return -EINVAL;
+
+		print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
+
+		last_frame = frame;
+		frame = frame->next_frame;
+		ret_addr = &frame->return_address;
+	}
+
+	if (last_frame + 1 != (void *)task_pt_regs(tinfo->task))
+		return -EINVAL;
+
+	*bp = (unsigned long)frame;
+	return 0;
+}
+
 static int print_trace_stack(void *data, char *name)
 {
 	printk("%s <%s> ", (char *)data, name);
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 9ee98ee..61078eb 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -14,6 +14,11 @@ static int save_stack_stack(void *data, char *name)
 	return 0;
 }
 
+static int save_stack_stack_reliable(void *data, char *name)
+{
+	return -EINVAL;
+}
+
 static int
 __save_stack_address(void *data, unsigned long addr, bool reliable, bool nosched)
 {
@@ -59,6 +64,12 @@ static const struct stacktrace_ops save_stack_ops_nosched = {
 	.walk_stack	= print_context_stack,
 };
 
+static const struct stacktrace_ops save_stack_ops_reliable = {
+	.stack		= save_stack_stack_reliable,
+	.address	= save_stack_address,
+	.walk_stack	= print_context_stack_reliable,
+};
+
 /*
  * Save stack-backtrace addresses into a stack_trace buffer.
  */
@@ -148,3 +159,24 @@ void save_stack_trace_user(struct stack_trace *trace)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
 
+#ifdef CONFIG_RELIABLE_STACKTRACE
+/*
+ * Returns 0 if the stack trace is deemed reliable.  The caller must ensure
+ * that the task is either sleeping or is the current task.
+ */
+int save_stack_trace_tsk_reliable(struct task_struct *tsk,
+				  struct stack_trace *trace)
+{
+	int ret;
+
+	ret = dump_trace(tsk, NULL, NULL, 0, &save_stack_ops_reliable, trace);
+	if (ret)
+		return ret;
+
+	if (trace->nr_entries == trace->max_entries)
+		return -EINVAL;
+
+	trace->entries[trace->nr_entries++] = ULONG_MAX;
+	return 0;
+}
+#endif /* CONFIG_RELIABLE_STACKTRACE */
diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
index 0a34489..527e4cc 100644
--- a/include/linux/stacktrace.h
+++ b/include/linux/stacktrace.h
@@ -2,17 +2,18 @@
 #define __LINUX_STACKTRACE_H
 
 #include <linux/types.h>
+#include <linux/errno.h>
 
 struct task_struct;
 struct pt_regs;
 
-#ifdef CONFIG_STACKTRACE
 struct stack_trace {
 	unsigned int nr_entries, max_entries;
 	unsigned long *entries;
 	int skip;	/* input argument: How many entries to skip */
 };
 
+#ifdef CONFIG_STACKTRACE
 extern void save_stack_trace(struct stack_trace *trace);
 extern void save_stack_trace_regs(struct pt_regs *regs,
 				  struct stack_trace *trace);
@@ -29,12 +30,23 @@ extern void save_stack_trace_user(struct stack_trace *trace);
 # define save_stack_trace_user(trace)              do { } while (0)
 #endif
 
-#else
+#else /* !CONFIG_STACKTRACE */
 # define save_stack_trace(trace)			do { } while (0)
 # define save_stack_trace_tsk(tsk, trace)		do { } while (0)
 # define save_stack_trace_user(trace)			do { } while (0)
 # define print_stack_trace(trace, spaces)		do { } while (0)
 # define snprint_stack_trace(buf, size, trace, spaces)	do { } while (0)
-#endif
+#endif /* CONFIG_STACKTRACE */
 
-#endif
+#ifdef CONFIG_RELIABLE_STACKTRACE
+extern int save_stack_trace_tsk_reliable(struct task_struct *tsk,
+					 struct stack_trace *trace);
+#else
+static inline int save_stack_trace_tsk_reliable(struct task_struct *tsk,
+						struct stack_trace *trace)
+{
+	return -ENOSYS;
+}
+#endif /* CONFIG_RELIABLE_STACKTRACE */
+
+#endif /* __LINUX_STACKTRACE_H */
diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index b6e4c16..f35bc5d 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -58,8 +58,8 @@ int snprint_stack_trace(char *buf, size_t size,
 EXPORT_SYMBOL_GPL(snprint_stack_trace);
 
 /*
- * Architectures that do not implement save_stack_trace_tsk or
- * save_stack_trace_regs get this weak alias and a once-per-bootup warning
+ * Architectures that do not implement save_stack_trace_*()
+ * get this weak alias and a once-per-bootup warning
  * (whenever this facility is utilized - for example by procfs):
  */
 __weak void
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 1e9a607..1edf69c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1159,6 +1159,12 @@ config STACKTRACE
 	  It is also used by various kernel debugging features that require
 	  stack trace generation.
 
+config RELIABLE_STACKTRACE
+	def_bool y
+	depends on HAVE_RELIABLE_STACKTRACE
+	depends on STACKTRACE
+	depends on STACK_VALIDATION
+
 config DEBUG_KOBJECT
 	bool "kobject debugging"
 	depends on DEBUG_KERNEL
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1368287 — Re: [RFC PATCH v1.9 07/14] x86/stacktrace: add function for detecting reliable stack traces

On Fri, 25 Mar 2016, Josh Poimboeuf wrote:

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2dc18605..76274b8 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -138,6 +138,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

I understand we have to rely on frame pointer for now. Do you plan to 
switch to dwarf unwinder one day in the future? IOW is there a plan to 
implement dwarf stuff generation in objtool and then to have a dwarf-based 
stack unwinder upstream and to use it for live patching? 

We have FRAME_POINTER unset in SLES for performance reasons (there was 
some 5 percent slowdown measured in the past. However we should redo the 
experiments.) and one day we'd really like to switch to upstream from 
kgraft :). So I'm just asking.

Miroslav

[toc] | [prev] | [next] | [standalone]

#1364823 — [RFC PATCH v1.9 12/14] livepatch: create per-task consistency model

Add 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 prototypes and/or data semantics.

When a patch is enabled, livepatch enters into a transition state where
tasks are converging from the old universe to the new universe.  If a
given task isn't using any of the patched functions, it's switched to
the new universe.  Once all the tasks have been converged to the new
universe, patching is complete.

The same sequence occurs when a patch is disabled, except the tasks
converge from the new universe to the old universe.

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 the
transition state indefinitely, if any of the tasks are stuck in the
previous universe.

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

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 include/linux/livepatch.h     |  27 +++
 include/linux/sched.h         |   3 +
 kernel/fork.c                 |   2 +
 kernel/livepatch/Makefile     |   2 +-
 kernel/livepatch/core.c       |  99 +++++++---
 kernel/livepatch/patch.c      |  43 ++++-
 kernel/livepatch/patch.h      |   1 +
 kernel/livepatch/transition.c | 406 ++++++++++++++++++++++++++++++++++++++++++
 kernel/livepatch/transition.h |  20 +++
 kernel/sched/core.c           |   2 +
 10 files changed, 582 insertions(+), 23 deletions(-)
 create mode 100644 kernel/livepatch/transition.c
 create mode 100644 kernel/livepatch/transition.h

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index dd5db74..4d2e26d 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -34,12 +34,14 @@
  * @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
  * @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
  */
 struct klp_func {
 	/* external */
@@ -53,6 +55,7 @@ struct klp_func {
 	 * in kallsyms for the given object is used.
 	 */
 	unsigned long old_sympos;
+	bool immediate;
 
 	/* internal */
 	unsigned long old_addr;
@@ -60,6 +63,7 @@ struct klp_func {
 	struct list_head stack_node;
 	unsigned long old_size, new_size;
 	bool patched;
+	bool transition;
 };
 
 /**
@@ -106,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)
@@ -114,6 +119,7 @@ struct klp_patch {
 	/* external */
 	struct module *mod;
 	struct klp_object *objs;
+	bool immediate;
 
 	/* internal */
 	struct list_head list;
@@ -136,11 +142,32 @@ int klp_disable_patch(struct klp_patch *);
 int klp_module_coming(struct module *mod);
 void klp_module_going(struct module *mod);
 
+extern int klp_universe_goal;
+/*
+ * klp_update_task_universe() - change the patched state of a task
+ * @task:	The task to change
+ *
+ * Converts the patched state of the task so that it will switch to the set of
+ * functions in the goal universe.
+ */
+static inline void klp_update_task_universe(struct task_struct *task)
+{
+	/*
+	 * The corresponding write barriers are in klp_init_transition() and
+	 * klp_start_transition().  See the comments there for an explanation.
+	 */
+	smp_rmb();
+
+	task->klp_universe = klp_universe_goal;
+}
+
 #else /* !CONFIG_LIVEPATCH */
 
 static inline int klp_module_coming(struct module *mod) { return 0; }
 static inline void klp_module_going(struct module *mod) { }
 
+static inline void klp_update_task_universe(struct task_struct *task) {}
+
 #endif /* CONFIG_LIVEPATCH */
 
 #endif /* _LINUX_LIVEPATCH_H_ */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 62d0961..c27286f 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1848,6 +1848,9 @@ struct task_struct {
 	unsigned long	task_state_change;
 #endif
 	int pagefault_disabled;
+#ifdef CONFIG_LIVEPATCH
+	int klp_universe;
+#endif
 /* CPU-specific state of this task */
 	struct thread_struct thread;
 /*
diff --git a/kernel/fork.c b/kernel/fork.c
index d277e83..27b181e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -76,6 +76,7 @@
 #include <linux/compiler.h>
 #include <linux/sysctl.h>
 #include <linux/kcov.h>
+#include <linux/livepatch.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -1615,6 +1616,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	total_forks++;
 	spin_unlock(&current->sighand->siglock);
 	syscall_tracepoint_update(p);
+	klp_update_task_universe(p);
 	write_unlock_irq(&tasklist_lock);
 
 	proc_fork_connector(p);
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e136dad..2b8bdb1 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 
-livepatch-objs := core.o patch.o
+livepatch-objs := core.o patch.o transition.o
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index b0fd31d..19afa9b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -27,14 +27,18 @@
 #include <linux/list.h>
 #include <linux/kallsyms.h>
 #include <linux/livepatch.h>
+#include <linux/stacktrace.h>
 #include <asm/cacheflush.h>
 #include "patch.h"
+#include "transition.h"
 
 /*
- * The klp_mutex protects the global lists and state transitions of any
- * structure reachable from them.  References to any structure must be obtained
- * under mutex protection (except in klp_ftrace_handler(), which uses RCU to
- * ensure it gets consistent data).
+ * klp_mutex is a coarse lock which serializes access to klp data.  All
+ * accesses to klp-related variables and structures must have mutex protection,
+ * except within the following functions which carefully avoid the need for it:
+ *
+ * - klp_ftrace_handler()
+ * - klp_update_task_universe()
  */
 static DEFINE_MUTEX(klp_mutex);
 
@@ -42,6 +46,30 @@ static LIST_HEAD(klp_patches);
 
 static struct kobject *klp_root_kobj;
 
+static void klp_work_fn(struct work_struct *work);
+static DECLARE_DELAYED_WORK(klp_work, klp_work_fn);
+
+static void klp_schedule_work(void)
+{
+	if (IS_ENABLED(CONFIG_RELIABLE_STACKTRACE))
+		schedule_delayed_work(&klp_work, round_jiffies_relative(HZ));
+}
+
+/*
+ * This work can be performed periodically to finish patching or unpatching any
+ * "straggler" tasks which failed to transition in klp_enable_patch().
+ */
+static void klp_work_fn(struct work_struct *work)
+{
+	mutex_lock(&klp_mutex);
+
+	if (klp_transition_patch)
+		if (!klp_try_complete_transition())
+			klp_schedule_work();
+
+	mutex_unlock(&klp_mutex);
+}
+
 static bool klp_is_module(struct klp_object *obj)
 {
 	return obj->name;
@@ -80,7 +108,6 @@ static void klp_find_object_module(struct klp_object *obj)
 	mutex_unlock(&module_mutex);
 }
 
-/* klp_mutex must be held by caller */
 static bool klp_is_patch_registered(struct klp_patch *patch)
 {
 	struct klp_patch *mypatch;
@@ -244,19 +271,18 @@ out:
 
 static int __klp_disable_patch(struct klp_patch *patch)
 {
-	struct klp_object *obj;
+	if (klp_transition_patch)
+		return -EBUSY;
 
 	/* enforce stacking: only the last enabled patch can be disabled */
 	if (!list_is_last(&patch->list, &klp_patches) &&
 	    list_next_entry(patch, list)->enabled)
 		return -EBUSY;
 
-	pr_notice("disabling patch '%s'\n", patch->mod->name);
-
-	klp_for_each_object(patch, obj) {
-		if (obj->patched)
-			klp_unpatch_object(obj);
-	}
+	klp_init_transition(patch, KLP_UNIVERSE_NEW);
+	klp_start_transition(KLP_UNIVERSE_OLD);
+	if (!klp_try_complete_transition())
+		klp_schedule_work();
 
 	patch->enabled = false;
 
@@ -300,6 +326,9 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	struct klp_object *obj;
 	int ret;
 
+	if (klp_transition_patch)
+		return -EBUSY;
+
 	if (WARN_ON(patch->enabled))
 		return -EINVAL;
 
@@ -311,24 +340,32 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
 	add_taint(TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
 
-	pr_notice("enabling patch '%s'\n", patch->mod->name);
+	klp_init_transition(patch, KLP_UNIVERSE_OLD);
 
 	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_unpatch_objects(patch);
+			klp_complete_transition();
+
+			return ret;
+		}
 	}
 
+	klp_start_transition(KLP_UNIVERSE_NEW);
+
+	if (!klp_try_complete_transition())
+		klp_schedule_work();
+
 	patch->enabled = true;
 
 	return 0;
-
-unregister:
-	WARN_ON(__klp_disable_patch(patch));
-	return ret;
 }
 
 /**
@@ -365,6 +402,7 @@ EXPORT_SYMBOL_GPL(klp_enable_patch);
  * /sys/kernel/livepatch
  * /sys/kernel/livepatch/<patch>
  * /sys/kernel/livepatch/<patch>/enabled
+ * /sys/kernel/livepatch/<patch>/transition
  * /sys/kernel/livepatch/<patch>/<object>
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
@@ -393,7 +431,9 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 		goto err;
 	}
 
-	if (val) {
+	if (klp_transition_patch == patch) {
+		klp_reverse_transition();
+	} else if (val) {
 		ret = __klp_enable_patch(patch);
 		if (ret)
 			goto err;
@@ -421,9 +461,21 @@ static ssize_t enabled_show(struct kobject *kobj,
 	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
 }
 
+static ssize_t transition_show(struct kobject *kobj,
+			       struct kobj_attribute *attr, char *buf)
+{
+	struct klp_patch *patch;
+
+	patch = container_of(kobj, struct klp_patch, kobj);
+	return snprintf(buf, PAGE_SIZE-1, "%d\n",
+			klp_transition_patch == patch);
+}
+
 static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
+static struct kobj_attribute transition_kobj_attr = __ATTR_RO(transition);
 static struct attribute *klp_patch_attrs[] = {
 	&enabled_kobj_attr.attr,
+	&transition_kobj_attr.attr,
 	NULL
 };
 
@@ -510,6 +562,7 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
 	INIT_LIST_HEAD(&func->stack_node);
 	func->patched = false;
+	func->transition = false;
 
 	/* The format for the sysfs directory is <function,sympos> where sympos
 	 * is the nth occurrence of this symbol in kallsyms for the patched
@@ -738,7 +791,11 @@ int klp_module_coming(struct module *mod)
 				goto err;
 			}
 
-			if (!patch->enabled)
+			/*
+			 * Only patch the module if the patch is enabled or is
+			 * in transition.
+			 */
+			if (!patch->enabled && klp_transition_patch != patch)
 				break;
 
 			pr_notice("applying patch '%s' to loading module '%s'\n",
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
index 92e9ee0..f0fa6b5 100644
--- a/kernel/livepatch/patch.c
+++ b/kernel/livepatch/patch.c
@@ -29,6 +29,7 @@
 #include <linux/bug.h>
 #include <linux/printk.h>
 #include "patch.h"
+#include "transition.h"
 
 static LIST_HEAD(klp_ops);
 
@@ -58,11 +59,42 @@ static void notrace klp_ftrace_handler(unsigned long ip,
 	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;
 
+	/*
+	 * See the comment for the 2nd smp_wmb() in klp_init_transition() for
+	 * an explanation of why this read barrier is needed.
+	 */
+	smp_rmb();
+
+	if (unlikely(func->transition)) {
+
+		/*
+		 * See the comment for the 1st smp_wmb() in
+		 * klp_init_transition() for an explanation of why this read
+		 * barrier is needed.
+		 */
+		smp_rmb();
+
+		if (current->klp_universe == KLP_UNIVERSE_OLD) {
+			/*
+			 * Use the previously patched version of the function.
+			 * If no previous patches exist, use the original
+			 * function.
+			 */
+			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();
@@ -183,3 +215,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);
+}
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
index 2d0cce0..0db2271 100644
--- a/kernel/livepatch/patch.h
+++ b/kernel/livepatch/patch.h
@@ -28,5 +28,6 @@ struct klp_ops *klp_find_ops(unsigned long old_addr);
 
 int klp_patch_object(struct klp_object *obj);
 void klp_unpatch_object(struct klp_object *obj);
+void klp_unpatch_objects(struct klp_patch *patch);
 
 #endif /* _LIVEPATCH_PATCH_H */
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
new file mode 100644
index 0000000..0609d84
--- /dev/null
+++ b/kernel/livepatch/transition.c
@@ -0,0 +1,406 @@
+/*
+ * transition.c - Kernel Live Patching transition functions
+ *
+ * Copyright (C) 2015 Josh Poimboeuf <jpoimboe@redhat.com>
+ *
+ * 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
+
+#include <linux/cpu.h>
+#include <linux/stacktrace.h>
+#include "../sched/sched.h"
+
+#include "patch.h"
+#include "transition.h"
+
+#define MAX_STACK_ENTRIES 100
+
+struct klp_patch *klp_transition_patch;
+
+int klp_universe_goal = KLP_UNIVERSE_UNDEFINED;
+
+/*
+ * The transition to the universe goal is complete.  Clean up the data
+ * structures.
+ */
+void klp_complete_transition(void)
+{
+	struct klp_object *obj;
+	struct klp_func *func;
+
+	if (klp_transition_patch->immediate)
+		goto done;
+
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			func->transition = false;
+
+done:
+	klp_transition_patch = NULL;
+}
+
+/*
+ * Determine whether the given stack trace includes any references to a
+ * to-be-patched or to-be-unpatched function.
+ */
+static int klp_check_stack_func(struct klp_func *func,
+				struct stack_trace *trace)
+{
+	unsigned long func_addr, func_size, address;
+	struct klp_ops *ops;
+	int i;
+
+	if (func->immediate)
+		return 0;
+
+	for (i = 0; i < trace->max_entries; i++) {
+		address = trace->entries[i];
+
+		if (klp_universe_goal == KLP_UNIVERSE_OLD) {
+			 /*
+			  * Check for the to-be-unpatched function
+			  * (the func itself).
+			  */
+			func_addr = (unsigned long)func->new_func;
+			func_size = func->new_size;
+		} else {
+			/*
+			 * Check for the to-be-patched function
+			 * (the previous func).
+			 */
+			ops = klp_find_ops(func->old_addr);
+
+			if (list_is_singular(&ops->func_stack)) {
+				/* original function */
+				func_addr = func->old_addr;
+				func_size = func->old_size;
+			} else {
+				/* previously patched function */
+				struct klp_func *prev;
+
+				prev = list_next_entry(func, stack_node);
+				func_addr = (unsigned long)prev->new_func;
+				func_size = prev->new_size;
+			}
+		}
+
+		if (address >= func_addr && address < func_addr + func_size)
+			return -EAGAIN;
+	}
+
+	return 0;
+}
+
+/*
+ * Determine whether it's safe to transition the task to the new universe by
+ * looking for any to-be-patched or to-be-unpatched functions on its stack.
+ */
+static int klp_check_stack(struct task_struct *task)
+{
+	static unsigned long entries[MAX_STACK_ENTRIES];
+	struct stack_trace trace;
+	struct klp_object *obj;
+	struct klp_func *func;
+	int ret;
+
+	trace.skip = 0;
+	trace.nr_entries = 0;
+	trace.max_entries = MAX_STACK_ENTRIES;
+	trace.entries = entries;
+	ret = save_stack_trace_tsk_reliable(task, &trace);
+	WARN_ON_ONCE(ret == -ENOSYS);
+	if (ret) {
+		pr_debug("%s: pid %d (%s) has an unreliable stack\n",
+			 __func__, task->pid, task->comm);
+		return 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, &trace);
+			if (ret) {
+				pr_debug("%s: pid %d (%s) is sleeping on function %s\n",
+					 __func__, task->pid, task->comm,
+					 func->old_name);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Try to safely switch a task to the universe goal.  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.
+ */
+static bool klp_try_switch_task(struct task_struct *task)
+{
+	struct rq *rq;
+	unsigned long flags;
+	int ret;
+	bool success = false;
+
+	/* check if this task has already switched over */
+	if (task->klp_universe == klp_universe_goal)
+		return true;
+
+	/*
+	 * For arches which don't have reliable stack traces, we have to rely
+	 * on other methods (e.g., switching tasks at the syscall barrier).
+	 */
+	if (!IS_ENABLED(CONFIG_RELIABLE_STACKTRACE))
+		return false;
+
+	/*
+	 * Now try to check the stack for any to-be-patched or to-be-unpatched
+	 * functions.  If all goes well, switch the task to the goal universe.
+	 */
+	rq = task_rq_lock(task, &flags);
+
+	if (task_running(rq, task) && task != current) {
+		pr_debug("%s: pid %d (%s) is running\n", __func__, task->pid,
+			 task->comm);
+		goto done;
+	}
+
+	ret = klp_check_stack(task);
+	if (ret)
+		goto done;
+
+	klp_update_task_universe(task);
+
+	success = true;
+done:
+	task_rq_unlock(rq, task, &flags);
+	return success;
+}
+
+/*
+ * Try to switch all remaining tasks to the goal universe 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 starting universe, schedule a retry.
+ */
+bool klp_try_complete_transition(void)
+{
+	unsigned int cpu;
+	struct task_struct *g, *task;
+	bool complete = true;
+
+	/*
+	 * 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 goal universe 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 the old universe so
+	 * we can now remove the new functions from the func_stack.
+	 */
+	if (klp_universe_goal == KLP_UNIVERSE_OLD) {
+		klp_unpatch_objects(klp_transition_patch);
+
+		/*
+		 * Don't allow any existing instances of ftrace handlers to
+		 * access any obsolete funcs before we reset the func
+		 * transition states to false.  Otherwise the handler may see
+		 * the deleted "new" func, see that it's not in transition, and
+		 * wrongly pick the new version of the function.
+		 */
+		synchronize_rcu();
+	}
+
+	pr_notice("'%s': %s complete\n", klp_transition_patch->mod->name,
+		  klp_universe_goal == KLP_UNIVERSE_NEW ? "patching" :
+							  "unpatching");
+
+	/* we're done, now cleanup the data structures */
+	klp_complete_transition();
+
+	return true;
+}
+
+/*
+ * Start the transition to the specified universe goal so tasks can begin
+ * switching to it.
+ */
+void klp_start_transition(int universe)
+{
+	if (WARN_ON(klp_universe_goal == universe))
+		return;
+
+	pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
+		  universe == KLP_UNIVERSE_NEW ? "patching" : "unpatching");
+
+	/*
+	 * Set the global universe goal which tasks will switch to.
+	 *
+	 * Note that any newly forked tasks after this call will be born in the
+	 * goal universe.  So the transition begins here, even before we start
+	 * switching tasks.
+	 */
+	klp_universe_goal = universe;
+
+	/*
+	 * Enforce the ordering of the universe goal write with later
+	 * task universe writes which are done via
+	 * klp_try_complete_transition().  The corresponding read barrier is in
+	 * klp_update_task_universe().
+	 */
+	smp_wmb();
+}
+
+/*
+ * This function can be called in the middle of an existing transition to
+ * reverse the direction of the universe goal.  This can be done to effectively
+ * cancel an existing enable or disable operation if there are any tasks which
+ * are stuck in the starting universe.
+ */
+void klp_reverse_transition(void)
+{
+	struct klp_patch *patch = klp_transition_patch;
+
+	klp_start_transition(!klp_universe_goal);
+	klp_try_complete_transition();
+
+	patch->enabled = !patch->enabled;
+}
+
+/*
+ * Set the global universe goal and all tasks to the starting universe, and
+ * initialize all function transition states to true in preparation for
+ * patching or unpatching.
+ */
+void klp_init_transition(struct klp_patch *patch, int universe)
+{
+	struct task_struct *g, *task;
+	unsigned int cpu;
+	struct klp_object *obj;
+	struct klp_func *func;
+
+	klp_transition_patch = patch;
+
+	/*
+	 * Initialize the universe goal to the starting universe.
+	 */
+	klp_universe_goal = universe;
+
+	/*
+	 * Ensure that if another CPU forks a task after the below task
+	 * universe writes and calls klp_update_task_universe(), it also sees
+	 * the above write to the universe goal.  Otherwise it may undo the
+	 * task universe writes below and mess up the task's starting universe.
+	 */
+	smp_wmb();
+
+	/*
+	 * If the patch can be applied or reverted immediately, skip the
+	 * per-task transitions.
+	 */
+	if (patch->immediate)
+		return;
+
+	/*
+	 * Initialize the task universes to their starting universe to prepare
+	 * them for switching to the goal universe.
+	 */
+	read_lock(&tasklist_lock);
+	for_each_process_thread(g, task)
+		klp_update_task_universe(task);
+	read_unlock(&tasklist_lock);
+
+	/*
+	 * Ditto for the idle "swapper" tasks.
+	 */
+	get_online_cpus();
+	for_each_online_cpu(cpu)
+		klp_update_task_universe(idle_task(cpu));
+	put_online_cpus();
+
+	/*
+	 * Ensure klp_ftrace_handler() sees the task->klp_universe updates
+	 * before the func->transition updates.  Otherwise it could read an
+	 * out-of-date task universe and pick the wrong function.
+	 */
+	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;
+
+	/*
+	 * For the enable path, ensure klp_ftrace_handler() will see the
+	 * func->transition updates before the funcs become visible to the
+	 * handler.  Otherwise the handler may wrongly pick the new func before
+	 * the task switches to the new universe.
+	 *
+	 * For the disable path, the funcs are already visible to the handler.
+	 * But we still need to ensure the ftrace handler will see the
+	 * func->transition updates before the tasks start switching to the old
+	 * universe.  Otherwise the handler can miss a universe change which
+	 * would result in it wrongly picking the new function.
+	 */
+	smp_wmb();
+}
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
new file mode 100644
index 0000000..98e9930
--- /dev/null
+++ b/kernel/livepatch/transition.h
@@ -0,0 +1,20 @@
+#ifndef _LIVEPATCH_TRANSITION_H
+#define _LIVEPATCH_TRANSITION_H
+
+#include <linux/livepatch.h>
+
+enum {
+	KLP_UNIVERSE_UNDEFINED = -1,
+	KLP_UNIVERSE_OLD,
+	KLP_UNIVERSE_NEW,
+};
+
+extern struct klp_patch *klp_transition_patch;
+
+extern void klp_init_transition(struct klp_patch *patch, int universe);
+extern void klp_start_transition(int universe);
+extern void klp_reverse_transition(void);
+extern bool klp_try_complete_transition(void);
+extern void klp_complete_transition(void);
+
+#endif /* _LIVEPATCH_TRANSITION_H */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index be1ef22..431007b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -74,6 +74,7 @@
 #include <linux/context_tracking.h>
 #include <linux/compiler.h>
 #include <linux/frame.h>
+#include <linux/livepatch.h>
 
 #include <asm/switch_to.h>
 #include <asm/tlb.h>
@@ -5082,6 +5083,7 @@ void init_idle(struct task_struct *idle, int cpu)
 #ifdef CONFIG_SMP
 	sprintf(idle->comm, "%s/%d", INIT_TASK_COMM, cpu);
 #endif
+	klp_update_task_universe(idle);
 }
 
 int cpuset_cpumask_can_shrink(const struct cpumask *cur,
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1368293 — Re: [RFC PATCH v1.9 12/14] livepatch: create per-task consistency model

On Fri, 25 Mar 2016, Josh Poimboeuf wrote:

[...]

> diff --git a/kernel/fork.c b/kernel/fork.c
> index d277e83..27b181e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -76,6 +76,7 @@
>  #include <linux/compiler.h>
>  #include <linux/sysctl.h>
>  #include <linux/kcov.h>
> +#include <linux/livepatch.h>
>  
>  #include <asm/pgtable.h>
>  #include <asm/pgalloc.h>
> @@ -1615,6 +1616,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	total_forks++;
>  	spin_unlock(&current->sighand->siglock);
>  	syscall_tracepoint_update(p);
> +	klp_update_task_universe(p);

Shouldn't we copy transition and TIF from the parent? I deal with a race 
in kgraft and the solution seems to be this code exactly at this place in 
copy_process(). I need to think about it.

[...]

> +static void klp_schedule_work(void)
> +{
> +	if (IS_ENABLED(CONFIG_RELIABLE_STACKTRACE))
> +		schedule_delayed_work(&klp_work, round_jiffies_relative(HZ));
> +}

As mentioned in my cover letter reply I'd allow to schedule delayed work 
even for !CONFIG_RELIABLE_STACKTRACE archs and configurations. There is a 
check in klp_try_switch_task() for CONFIG_RELIABLE_STACKTRACE so the 
change should be trivial. The patching could be successful even without 
reliable stack traces especially in combination with a fake signal and 
a syscall boundary migration.

Miroslav

[toc] | [prev] | [next] | [standalone]

#1364824 — [RFC PATCH v1.9 08/14] livepatch: separate enabled and patched states

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 <jpoimboe@redhat.com>
---
 include/linux/livepatch.h | 17 ++++-------
 kernel/livepatch/core.c   | 72 +++++++++++++++++++++++------------------------
 2 files changed, 42 insertions(+), 47 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index bd830d5..6d45dc7 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -28,11 +28,6 @@
 
 #include <asm/livepatch.h>
 
-enum klp_state {
-	KLP_DISABLED,
-	KLP_ENABLED
-};
-
 /**
  * struct klp_func - function structure for live patching
  * @old_name:	name of the function to be patched
@@ -41,8 +36,8 @@ enum klp_state {
  *		can be found (optional)
  * @old_addr:	the address of the function being patched
  * @kobj:	kobject for sysfs resources
- * @state:	tracks function-level patch application state
  * @stack_node:	list node for klp_ops func_stack list
+ * @patched:	the func has been added to the klp_ops list
  */
 struct klp_func {
 	/* external */
@@ -60,8 +55,8 @@ struct klp_func {
 	/* internal */
 	unsigned long old_addr;
 	struct kobject kobj;
-	enum klp_state state;
 	struct list_head stack_node;
+	bool patched;
 };
 
 /**
@@ -90,7 +85,7 @@ struct klp_reloc {
  * @kobj:	kobject for sysfs resources
  * @mod:	kernel module associated with the patched object
  * 		(NULL for vmlinux)
- * @state:	tracks object-level patch application state
+ * @patched:	the object's funcs have been add to the klp_ops list
  */
 struct klp_object {
 	/* external */
@@ -101,7 +96,7 @@ struct klp_object {
 	/* internal */
 	struct kobject kobj;
 	struct module *mod;
-	enum klp_state state;
+	bool patched;
 };
 
 /**
@@ -110,7 +105,7 @@ struct klp_object {
  * @objs:	object entries for kernel objects to be patched
  * @list:	list node for global list of registered patches
  * @kobj:	kobject for sysfs resources
- * @state:	tracks patch-level application state
+ * @enabled:	the patch is enabled (but operation may be incomplete)
  */
 struct klp_patch {
 	/* external */
@@ -120,7 +115,7 @@ struct klp_patch {
 	/* internal */
 	struct list_head list;
 	struct kobject kobj;
-	enum klp_state state;
+	bool enabled;
 };
 
 #define klp_for_each_object(patch, obj) \
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index d68fbf6..be1e106 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -298,11 +298,11 @@ unlock:
 	rcu_read_unlock();
 }
 
-static void klp_disable_func(struct klp_func *func)
+static void klp_unpatch_func(struct klp_func *func)
 {
 	struct klp_ops *ops;
 
-	if (WARN_ON(func->state != KLP_ENABLED))
+	if (WARN_ON(!func->patched))
 		return;
 	if (WARN_ON(!func->old_addr))
 		return;
@@ -322,10 +322,10 @@ static void klp_disable_func(struct klp_func *func)
 		list_del_rcu(&func->stack_node);
 	}
 
-	func->state = KLP_DISABLED;
+	func->patched = false;
 }
 
-static int klp_enable_func(struct klp_func *func)
+static int klp_patch_func(struct klp_func *func)
 {
 	struct klp_ops *ops;
 	int ret;
@@ -333,7 +333,7 @@ static int klp_enable_func(struct klp_func *func)
 	if (WARN_ON(!func->old_addr))
 		return -EINVAL;
 
-	if (WARN_ON(func->state != KLP_DISABLED))
+	if (WARN_ON(func->patched))
 		return -EINVAL;
 
 	ops = klp_find_ops(func->old_addr);
@@ -372,7 +372,7 @@ static int klp_enable_func(struct klp_func *func)
 		list_add_rcu(&func->stack_node, &ops->func_stack);
 	}
 
-	func->state = KLP_ENABLED;
+	func->patched = true;
 
 	return 0;
 
@@ -383,36 +383,36 @@ err:
 	return ret;
 }
 
-static void klp_disable_object(struct klp_object *obj)
+static void klp_unpatch_object(struct klp_object *obj)
 {
 	struct klp_func *func;
 
 	klp_for_each_func(obj, func)
-		if (func->state == KLP_ENABLED)
-			klp_disable_func(func);
+		if (func->patched)
+			klp_unpatch_func(func);
 
-	obj->state = KLP_DISABLED;
+	obj->patched = false;
 }
 
-static int klp_enable_object(struct klp_object *obj)
+static int klp_patch_object(struct klp_object *obj)
 {
 	struct klp_func *func;
 	int ret;
 
-	if (WARN_ON(obj->state != KLP_DISABLED))
+	if (WARN_ON(obj->patched))
 		return -EINVAL;
 
 	if (WARN_ON(!klp_is_object_loaded(obj)))
 		return -EINVAL;
 
 	klp_for_each_func(obj, func) {
-		ret = klp_enable_func(func);
+		ret = klp_patch_func(func);
 		if (ret) {
-			klp_disable_object(obj);
+			klp_unpatch_object(obj);
 			return ret;
 		}
 	}
-	obj->state = KLP_ENABLED;
+	obj->patched = true;
 
 	return 0;
 }
@@ -423,17 +423,17 @@ static int __klp_disable_patch(struct klp_patch *patch)
 
 	/* enforce stacking: only the last enabled patch can be disabled */
 	if (!list_is_last(&patch->list, &klp_patches) &&
-	    list_next_entry(patch, list)->state == KLP_ENABLED)
+	    list_next_entry(patch, list)->enabled)
 		return -EBUSY;
 
 	pr_notice("disabling patch '%s'\n", patch->mod->name);
 
 	klp_for_each_object(patch, obj) {
-		if (obj->state == KLP_ENABLED)
-			klp_disable_object(obj);
+		if (obj->patched)
+			klp_unpatch_object(obj);
 	}
 
-	patch->state = KLP_DISABLED;
+	patch->enabled = false;
 
 	return 0;
 }
@@ -457,7 +457,7 @@ int klp_disable_patch(struct klp_patch *patch)
 		goto err;
 	}
 
-	if (patch->state == KLP_DISABLED) {
+	if (!patch->enabled) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -475,12 +475,12 @@ static int __klp_enable_patch(struct klp_patch *patch)
 	struct klp_object *obj;
 	int ret;
 
-	if (WARN_ON(patch->state != KLP_DISABLED))
+	if (WARN_ON(patch->enabled))
 		return -EINVAL;
 
 	/* enforce stacking: only the first disabled patch can be enabled */
 	if (patch->list.prev != &klp_patches &&
-	    list_prev_entry(patch, list)->state == KLP_DISABLED)
+	    !list_prev_entry(patch, list)->enabled)
 		return -EBUSY;
 
 	pr_notice_once("tainting kernel with TAINT_LIVEPATCH\n");
@@ -492,12 +492,12 @@ static int __klp_enable_patch(struct klp_patch *patch)
 		if (!klp_is_object_loaded(obj))
 			continue;
 
-		ret = klp_enable_object(obj);
+		ret = klp_patch_object(obj);
 		if (ret)
 			goto unregister;
 	}
 
-	patch->state = KLP_ENABLED;
+	patch->enabled = true;
 
 	return 0;
 
@@ -555,20 +555,20 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (ret)
 		return -EINVAL;
 
-	if (val != KLP_DISABLED && val != KLP_ENABLED)
+	if (val > 1)
 		return -EINVAL;
 
 	patch = container_of(kobj, struct klp_patch, kobj);
 
 	mutex_lock(&klp_mutex);
 
-	if (val == patch->state) {
+	if (patch->enabled == val) {
 		/* already in requested state */
 		ret = -EINVAL;
 		goto err;
 	}
 
-	if (val == KLP_ENABLED) {
+	if (val) {
 		ret = __klp_enable_patch(patch);
 		if (ret)
 			goto err;
@@ -593,7 +593,7 @@ static ssize_t enabled_show(struct kobject *kobj,
 	struct klp_patch *patch;
 
 	patch = container_of(kobj, struct klp_patch, kobj);
-	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->state);
+	return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
 }
 
 static struct kobj_attribute enabled_kobj_attr = __ATTR_RW(enabled);
@@ -684,7 +684,7 @@ static void klp_free_patch(struct klp_patch *patch)
 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
 	INIT_LIST_HEAD(&func->stack_node);
-	func->state = KLP_DISABLED;
+	func->patched = false;
 
 	/* The format for the sysfs directory is <function,sympos> where sympos
 	 * is the nth occurrence of this symbol in kallsyms for the patched
@@ -729,7 +729,7 @@ static int klp_init_object(struct klp_patch *patch, struct klp_object *obj)
 	if (!obj->funcs)
 		return -EINVAL;
 
-	obj->state = KLP_DISABLED;
+	obj->patched = false;
 	obj->mod = NULL;
 
 	klp_find_object_module(obj);
@@ -770,7 +770,7 @@ static int klp_init_patch(struct klp_patch *patch)
 
 	mutex_lock(&klp_mutex);
 
-	patch->state = KLP_DISABLED;
+	patch->enabled = false;
 
 	ret = kobject_init_and_add(&patch->kobj, &klp_ktype_patch,
 				   klp_root_kobj, "%s", patch->mod->name);
@@ -816,7 +816,7 @@ int klp_unregister_patch(struct klp_patch *patch)
 		goto out;
 	}
 
-	if (patch->state == KLP_ENABLED) {
+	if (patch->enabled) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -897,13 +897,13 @@ int klp_module_coming(struct module *mod)
 				goto err;
 			}
 
-			if (patch->state == KLP_DISABLED)
+			if (!patch->enabled)
 				break;
 
 			pr_notice("applying patch '%s' to loading module '%s'\n",
 				  patch->mod->name, obj->mod->name);
 
-			ret = klp_enable_object(obj);
+			ret = klp_patch_object(obj);
 			if (ret) {
 				pr_warn("failed to apply patch '%s' to module '%s' (%d)\n",
 					patch->mod->name, obj->mod->name, ret);
@@ -954,10 +954,10 @@ void klp_module_going(struct module *mod)
 			if (!klp_is_module(obj) || strcmp(obj->name, mod->name))
 				continue;
 
-			if (patch->state != KLP_DISABLED) {
+			if (patch->enabled) {
 				pr_notice("reverting patch '%s' on unloading module '%s'\n",
 					  patch->mod->name, obj->mod->name);
-				klp_disable_object(obj);
+				klp_unpatch_object(obj);
 			}
 
 			klp_free_object_loaded(obj);
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1364825 — [RFC PATCH v1.9 04/14] x86: move _stext marker before head code

When core_kernel_text() is used to determine whether an address on a
task's stack trace is a kernel text address, it incorrectly returns
false for early text addresses for the head code between the _text and
_stext markers.

Head code is text code too, so mark it as such.  This seems to match the
intent of other users of the _stext symbol, and it also seems consistent
with what other architectures are already doing.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/vmlinux.lds.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index d239639..66e6acb 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -91,10 +91,10 @@ SECTIONS
 	/* Text and read-only data */
 	.text :  AT(ADDR(.text) - LOAD_OFFSET) {
 		_text = .;
+		_stext = .;
 		/* bootstrapping code */
 		HEAD_TEXT
 		. = ALIGN(8);
-		_stext = .;
 		TEXT_TEXT
 		SCHED_TEXT
 		LOCK_TEXT
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1368273

Hi,

this is a great work. I'll have to review it properly (especially 13/14, 
probably several times as it is a heavy stuff), but I've gathered some 
notes so there they are.

On Fri, 25 Mar 2016, Josh Poimboeuf wrote:

> These patches are still a work in progress, but Jiri asked that I share
> them before I go on vacation next week.  Based on origin/master because
> it has CONFIG_STACK_VALIDATION.
> 
> This has two consistency models: the immediate model (like in today's
> code) and the new kpatch/kgraft hybrid model.
> 
> The default is the hybrid model: 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 pretty
> flexible, yet the code is still quite simple.
> 
> Patches are applied on a per-task basis, when the task is deemed safe to
> switch over.  It uses a tiered approach to determine when a task is safe
> and can be switched.
> 
> The first wave of attack is stack checking of sleeping tasks.  If no
> affected functions are on the stack of a given task, the task is
> switched.  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
> (CONFIG_RELIABLE_STACKTRACE and CONFIG_STACK_VALIDATION).

I think we could allow periodic checking even for 
!CONFIG_RELIABLE_STACKTRACE situations. The problematic task could migrate 
by itself after some time (meaning it woke up meanwhile and sleeps 
somewhere else now, or it went through a syscall boundary). So we can 
gain something, especially when combined with a fake signal approach. More 
on that below and in my 13/14 mail.

> The next line of attack, if needed, is syscall/IRQ switching.  A task is
> switched when it returns from a system call or a user space IRQ.  This
> approach is less than ideal because it usually requires signaling tasks
> to get them to switch.  It also doesn't work for kthreads.  But it's
> still useful in some cases when user tasks get stuck sleeping on an
> affected function.
> 
> For architectures which don't have reliable stacks, users have two
> options:
> 
> a) use the hybrid fallback option of using only the syscall/IRQ
>    switching (which is the default);
> 
> b) or they can use the immediate model (which is the model we already
>    have today) by setting the patch->immediate flag.
> 
> There's also a func->immediate flag which allows users 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.
> 
> Still have a lot of TODOs, some of them are listed here.  If you see
> something objectionable, it might be a good idea to make sure it's not
> already on the TODO list :-)
> 
> TODO:
> - actually test it
> - don't use TIP_KLP_NEED_UPDATE in arch-independent code
> - figure out new API to keep the use of task_rq_lock() in the sched code

Hm, no idea how to do it so that everyone is satisfied. I still remember 
Peter's protests.

> - cleaner way to detect preemption on the stack
> - allow patch modules to be removed.  still needs more discussion and
>   thought.  maybe something like Miroslav's patch would be good:
>   https://lkml.kernel.org/r/alpine.LNX.2.00.1512150857510.24899@pobox.suse.cz

Yup, that could be part of the patch set. The second option (to rework 
klp_unregister_patch and move kobject_put out of mutex protected parts) is 
maybe a way to go. The mutex_trylock approach works as well, but it is not 
clean and nice enough I guess. However the patch is there :).

Anyway the module removal should be prohibited when one uses immmediate 
flag set to true. Without consistency model we cannot say if it is safe to 
remove the module. Some process could still be in its code.

> - come up with a better name than universe?  KLP_STATE_PREV/NEXT?
>   KLP_UNPATCHED/PATCHED?  there were some objections to the name in v1.
> - update documentation for sysfs, proc, livepatch
> - need atomic accesses or READ_ONCE/WRITE_ONCE anywhere?
> - ability to force a patch to the goal universe

This could be made by a call to klp_update_task_universe for all tasks, 
couldn't it? We have something similar in kgraft.

> - try ftrace handler switching idea from v1 cover letter
> - split up the patches better
> - cc all the right people

I'd add a fake signal facility for sleeping non-migrated tasks. This 
would accelerate a migration to a new universe. We have it in kgraft for 
quite some time and it worked out great. See 
lkml.kernel.org/r/1430739625-4658-9-git-send-email-jslaby@suse.cz which 
went with Jiri's kgraft-on-klp patch set. See also Oleg's reply as it is 
important (I changed kgraft implementation according to that).

Miroslav

[toc] | [prev] | [next] | [standalone]

#1369322

On Fri 2016-03-25 14:34:47, Josh Poimboeuf wrote:
> These patches are still a work in progress, but Jiri asked that I share
> them before I go on vacation next week.  Based on origin/master because
> it has CONFIG_STACK_VALIDATION.

I have to follow Mirek and say that it is a great work.

> There's also a func->immediate flag which allows users 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.

I like the possibility to immediately patch some functions or objects.
Just note that this is not yet completely implemented and it is not
on the TODO list.

We probably should not set func->transition flag when func->immediate
is set or when the related func->object is set. It currently happens
only when patch->immediate is set.

Also we should ignore immediate functions and objects when the stack
is checked.


> Still have a lot of TODOs, some of them are listed here.  If you see
> something objectionable, it might be a good idea to make sure it's not
> already on the TODO list :-)
> 
> TODO:
> - come up with a better name than universe?  KLP_STATE_PREV/NEXT?
>   KLP_UNPATCHED/PATCHED?  there were some objections to the name in v1.

The name "universe" has an advantage if we later allow to
enable/disable more patches in parallel. The integer might hold
an identifier of the last applied patch. I have been playing with
this for kGraft one year ago and it was really challenging.
We should avoid it if possible. It is not really needed
if we are able to complete any transition in a reasonable time.

If we support only one transition at a time, a simple boolean
or even bit should be enough. The most descriptive name would
be klp_transition_patch_applied but it is quite long.

Note that similar information is provided by TIF_KLP_NEED_UPDATE
flag. We use only this bit in kGraft. It saves some space in
task_struct but it brings other challenges. We need to prevent
migration using a global "kgr_immutable" flag until ftrace handlers
for all patched functions are in place. We need to set the flag
back when the ftrace handler is called and the global "kgr_immutable"
flag is set.

> - update documentation for sysfs, proc, livepatch

Also we should publish somewhere the information about TIF_KLP_NEED_UPDATE
flag, e.g. /proc/<pid>/klp_need_update. It is handy to see what
process blocks the transition. We have something similar in
kGraft, see in_progress_show() at
https://git.kernel.org/cgit/linux/kernel/git/jirislaby/kgraft.git/commit/?h=kgraft-4.4&id=1c82fbd7b1fe240f4ed178a6506a93033f6a4bed


I am still shaking my head around the patches.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1369426

On Fri 2016-04-01 15:39:44, Petr Mladek wrote:
> On Fri 2016-03-25 14:34:47, Josh Poimboeuf wrote:
> > These patches are still a work in progress, but Jiri asked that I share
> > them before I go on vacation next week.  Based on origin/master because
> > it has CONFIG_STACK_VALIDATION.
> 
> I have to follow Mirek and say that it is a great work.
> 
> > There's also a func->immediate flag which allows users 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.
> 
> I like the possibility to immediately patch some functions or objects.
> Just note that this is not yet completely implemented and it is not
> on the TODO list.

Correction. Only patch and func can be marked by the "immediate" flag, not
an object. It looks fine.

> We probably should not set func->transition flag when func->immediate
> is set or when the related func->object is set. It currently happens
> only when patch->immediate is set.

This is true but only for func->transition.


> Also we should ignore immediate functions and objects when the stack
> is checked.

This is already done. I have missed this.

I am sorry for confusion. I should have shaken my head even more
before writing.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1369323

> - actually test it

I did slightly and it partially worked and partially it did not.

When I applied sample livepatch module, /proc/cmdline was patched and when 
I called 'cat /proc/cmdline' I got the correct livepatched message. So far 
so good. But the patching itself never finished because of many processes 
with unreliable stacks. It almost looked like every sleeping process was 
reported. I haven't debugged that yet.

Second, I have a simple test case. Kthread which sleeps in to-be-patched 
function foo() for a while and then it sleeps somewhere else and that in a 
loop. After live patch application the kthread is reported to have 
unreliable stack and it is not migrated. The good thing is that also the 
function foo() from the old universe is called and thus the consistency 
model works.

So I guess there is some problem in a stack checking...

Miroslav

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web