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

Groups > linux.kernel > #1580210 > unrolled thread

[PATCH v5 00/15] livepatch: hybrid consistency model

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 00/15] livepatch: hybrid consistency model Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-14 02:50 +0100
    [PATCH v5 11/15] livepatch: use kstrtobool() in enabled_store() Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-14 02:50 +0100
    [PATCH v5 07/15] livepatch/s390: add TIF_PATCH_PENDING thread flag Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-14 02:50 +0100
    [PATCH v5 01/15] stacktrace/x86: add function for detecting reliable stack traces Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-14 02:50 +0100
      Re: [PATCH v5 01/15] stacktrace/x86: add function for detecting  reliable stack traces Miroslav Benes <mbenes@suse.cz> - 2017-02-15 13:20 +0100
        Re: [PATCH v5 01/15] stacktrace/x86: add function for detecting  reliable stack traces Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-15 15:50 +0100
    Re: [PATCH v5 13/15] livepatch: change to a per-task consistency  model Miroslav Benes <mbenes@suse.cz> - 2017-02-16 15:40 +0100
      Re: [PATCH v5 13/15] livepatch: change to a per-task consistency  model Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-16 21:40 +0100
        Re: [PATCH v5 13/15] livepatch: change to a per-task consistency  model Miroslav Benes <mbenes@suse.cz> - 2017-02-17 10:00 +0100
          Re: [PATCH v5 13/15] livepatch: change to a per-task consistency  model Josh Poimboeuf <jpoimboe@redhat.com> - 2017-02-21 22:30 +0100
            Re: [PATCH v5 13/15] livepatch: change to a per-task consistency  model Miroslav Benes <mbenes@suse.cz> - 2017-02-22 10:10 +0100
    Re: [PATCH v5 00/15] livepatch: hybrid consistency model Miroslav Benes <mbenes@suse.cz> - 2017-02-17 10:00 +0100

#1580210 — [PATCH v5 00/15] livepatch: hybrid consistency model

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

I would very much appreciate reviews/acks from the following:

- Michael Ellerman for the powerpc changes in patch 5.

- Heiko Carstens for the s390 changes in patches 6 & 7.

- Peter Zijlstra/Ingo Molnar for the use of task_rq_lock() and the
  modification of do_idle() in patch 13.

Thanks!

Based on linux-next/master (20170213).

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

v4:
- add warnings for "impossible" scenarios in __save_stack_trace_reliable()
- sort _TIF_ALLWORK_MASK flags
- move klp_transition_work to transition.c.  This resulted in the following 
  related changes:
  - klp_mutex is now visible to transition.c
  - klp_start_transition() now calls klp_try_complete_transition()
  - klp_try_complete_transition() now sets up the work
  - rearrange code in transition.c accordingly
- klp_reverse_transition(): clear TIF flags and call synchronize_rcu()
- klp_try_complete_transition(): do synchronize_rcu() only when unpatching
- klp_start_transition(): only set TIF flags when necessary
- klp_complete_transition(): add synchronize_rcu() when patching
- klp_ftrace_handler(): put WARN_ON_ONCE back in and add comment
- use for_each_possible_cpu() to patch offline idle tasks
- add warnings to sample module when setting patch.immediate
- don't use pr_debug() with the task rq lock
- add documentation about porting consistency model to other arches
- move klp_patch_pending() to patch 13
- improve several comments and commit messages

v3:
- rebase on new x86 unwinder
- force !HAVE_RELIABLE_STACKTRACE arches to use patch->immediate for
  now, because we don't have a way to transition kthreads otherwise
- rebase s390 TIF_PATCH_PENDING patch onto latest entry code
- update barrier comments and move barrier from the end of
  klp_init_transition() to its callers
- "klp_work" -> "klp_transition_work"
- "klp_patch_task()" -> "klp_update_patch_state()"
- explicit _TIF_ALLWORK_MASK
- change klp_reverse_transition() to not try to complete transition.
  instead modify the work queue delay to zero.
- get rid of klp_schedule_work() in favor of calling
  schedule_delayed_work() directly with a KLP_TRANSITION_DELAY
- initialize klp_target_state to KLP_UNDEFINED
- move klp_target_state assignment to before patch->immediate check in
  klp_init_transition()
- rcu_read_lock() in klp_update_patch_state(), test the thread flag in
  patch task, synchronize_rcu() in klp_complete_transition()
- use kstrtobool() in enabled_store()
- change task_rq_lock() argument type to struct rq_flags
- add several WARN_ON_ONCE assertions for klp_target_state and
  task->patch_state

v2:
- "universe" -> "patch state"
- rename klp_update_task_universe() -> klp_patch_task()
- add preempt IRQ tracking (TF_PREEMPT_IRQ)
- fix print_context_stack_reliable() bug
- improve print_context_stack_reliable() comments
- klp_ftrace_handler comment fixes
- add "patch_state" proc file to tid_base_stuff
- schedule work even for !RELIABLE_STACKTRACE
- forked child inherits patch state from parent
- add detailed comment to livepatch.h klp_func definition about the
  klp_func patched/transition state transitions
- update exit_to_usermode_loop() comment
- clear all TIF_KLP_NEED_UPDATE flags in klp_complete_transition()
- remove unnecessary function externs
- add livepatch documentation, sysfs documentation, /proc documentation
- /proc/pid/patch_state: -1 means no patch is currently being applied/reverted
- "TIF_KLP_NEED_UPDATE" -> "TIF_PATCH_PENDING"
- support for s390 and powerpc-le
- don't assume stacks with dynamic ftrace trampolines are reliable
- add _TIF_ALLWORK_MASK info to commit log

v1.9:
- 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


Jiri Slaby (1):
  livepatch/s390: reorganize TIF thread flag bits

Josh Poimboeuf (12):
  stacktrace/x86: add function for detecting reliable stack traces
  x86/entry: define _TIF_ALLWORK_MASK flags explicitly
  livepatch: create temporary klp_update_patch_state() stub
  livepatch/x86: add TIF_PATCH_PENDING thread flag
  livepatch/powerpc: add TIF_PATCH_PENDING thread flag
  livepatch: separate enabled and patched states
  livepatch: remove unnecessary object loaded check
  livepatch: move patching functions into patch.c
  livepatch: use kstrtobool() in enabled_store()
  livepatch: store function sizes
  livepatch: change to a per-task consistency model
  livepatch: add /proc/<pid>/patch_state

Miroslav Benes (2):
  livepatch/s390: add TIF_PATCH_PENDING thread flag
  livepatch: allow removal of a disabled patch

 Documentation/ABI/testing/sysfs-kernel-livepatch |   8 +
 Documentation/filesystems/proc.txt               |  18 +
 Documentation/livepatch/livepatch.txt            | 214 +++++++--
 arch/Kconfig                                     |   6 +
 arch/powerpc/include/asm/thread_info.h           |   4 +-
 arch/powerpc/kernel/signal.c                     |   4 +
 arch/s390/include/asm/thread_info.h              |  24 +-
 arch/s390/kernel/entry.S                         |  31 +-
 arch/x86/Kconfig                                 |   1 +
 arch/x86/entry/common.c                          |   9 +-
 arch/x86/include/asm/thread_info.h               |  13 +-
 arch/x86/include/asm/unwind.h                    |   6 +
 arch/x86/kernel/stacktrace.c                     |  96 +++-
 arch/x86/kernel/unwind_frame.c                   |   2 +
 fs/proc/base.c                                   |  15 +
 include/linux/init_task.h                        |   9 +
 include/linux/livepatch.h                        |  68 ++-
 include/linux/sched.h                            |   3 +
 include/linux/stacktrace.h                       |   9 +-
 kernel/fork.c                                    |   3 +
 kernel/livepatch/Makefile                        |   2 +-
 kernel/livepatch/core.c                          | 436 +++++++-----------
 kernel/livepatch/patch.c                         | 272 +++++++++++
 kernel/livepatch/patch.h                         |  33 ++
 kernel/livepatch/transition.c                    | 551 +++++++++++++++++++++++
 kernel/livepatch/transition.h                    |  14 +
 kernel/sched/idle.c                              |   4 +
 kernel/stacktrace.c                              |  12 +-
 samples/livepatch/livepatch-sample.c             |  18 +-
 29 files changed, 1531 insertions(+), 354 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.7.4

[toc] | [next] | [standalone]

#1580211 — [PATCH v5 11/15] livepatch: use kstrtobool() in enabled_store()

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 <pmladek@suse.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Acked-by: Miroslav Benes <mbenes@suse.cz>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/livepatch/core.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6a137e1..83c4949 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -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;
 
-	ret = kstrtoul(buf, 10, &val);
+	ret = kstrtobool(buf, &enabled);
 	if (ret)
-		return -EINVAL;
-
-	if (val > 1)
-		return -EINVAL;
+		return ret;
 
 	patch = container_of(kobj, struct klp_patch, kobj);
 
 	mutex_lock(&klp_mutex);
 
-	if (patch->enabled == val) {
+	if (patch->enabled == enabled) {
 		/* already in requested state */
 		ret = -EINVAL;
 		goto err;
 	}
 
-	if (val) {
+	if (enabled) {
 		ret = __klp_enable_patch(patch);
 		if (ret)
 			goto err;
-- 
2.7.4

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

#1580212 — [PATCH v5 07/15] livepatch/s390: add TIF_PATCH_PENDING thread flag

From: Miroslav Benes <mbenes@suse.cz>

Update a task's patch state when returning from a system call or user
space interrupt, or after handling a signal.

This greatly increases the chances of a patch operation succeeding.  If
a task is I/O bound, it can be patched when returning from a system
call.  If a task is CPU bound, it can be patched when returning from an
interrupt.  If a task is sleeping on a to-be-patched function, the user
can send SIGSTOP and SIGCONT to force it to switch.

Since there are two ways the syscall can be restarted on return from a
signal handling process, it is important to clear the flag before
do_signal() is called. Otherwise we could miss the migration if we used
SIGSTOP/SIGCONT procedure or fake signal to migrate patching blocking
tasks. If we place our hook to sysc_work label in entry before
TIF_SIGPENDING is evaluated we kill two birds with one stone. The task
is correctly migrated in all return paths from a syscall.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/s390/include/asm/thread_info.h |  2 ++
 arch/s390/kernel/entry.S            | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
index 4977668..646845e 100644
--- a/arch/s390/include/asm/thread_info.h
+++ b/arch/s390/include/asm/thread_info.h
@@ -56,6 +56,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
 #define TIF_SIGPENDING		1	/* signal pending */
 #define TIF_NEED_RESCHED	2	/* rescheduling necessary */
 #define TIF_UPROBE		3	/* breakpointed or single-stepping */
+#define TIF_PATCH_PENDING	4	/* pending live patching update */
 
 #define TIF_31BIT		16	/* 32bit process */
 #define TIF_MEMDIE		17	/* is terminating due to OOM killer */
@@ -74,6 +75,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
 #define _TIF_SIGPENDING		_BITUL(TIF_SIGPENDING)
 #define _TIF_NEED_RESCHED	_BITUL(TIF_NEED_RESCHED)
 #define _TIF_UPROBE		_BITUL(TIF_UPROBE)
+#define _TIF_PATCH_PENDING	_BITUL(TIF_PATCH_PENDING)
 
 #define _TIF_31BIT		_BITUL(TIF_31BIT)
 #define _TIF_SINGLE_STEP	_BITUL(TIF_SINGLE_STEP)
diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
index 34ab7e8..9a15eac 100644
--- a/arch/s390/kernel/entry.S
+++ b/arch/s390/kernel/entry.S
@@ -47,7 +47,7 @@ STACK_SIZE  = 1 << STACK_SHIFT
 STACK_INIT = STACK_SIZE - STACK_FRAME_OVERHEAD - __PT_SIZE
 
 _TIF_WORK	= (_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | _TIF_NEED_RESCHED | \
-		   _TIF_UPROBE)
+		   _TIF_UPROBE | _TIF_PATCH_PENDING)
 _TIF_TRACE	= (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | _TIF_SECCOMP | \
 		   _TIF_SYSCALL_TRACEPOINT)
 _CIF_WORK	= (_CIF_MCCK_PENDING | _CIF_ASCE | _CIF_FPU)
@@ -333,6 +333,11 @@ ENTRY(system_call)
 #endif
 	TSTMSK	__PT_FLAGS(%r11),_PIF_PER_TRAP
 	jo	.Lsysc_singlestep
+#ifdef CONFIG_LIVEPATCH
+	TSTMSK	__TI_flags(%r12),_TIF_PATCH_PENDING
+	jo	.Lsysc_patch_pending	# handle live patching just before
+					# signals and possible syscall restart
+#endif
 	TSTMSK	__TI_flags(%r12),_TIF_SIGPENDING
 	jo	.Lsysc_sigpending
 	TSTMSK	__TI_flags(%r12),_TIF_NOTIFY_RESUME
@@ -405,6 +410,16 @@ ENTRY(system_call)
 #endif
 
 #
+# _TIF_PATCH_PENDING is set, call klp_update_patch_state
+#
+#ifdef CONFIG_LIVEPATCH
+.Lsysc_patch_pending:
+	lg	%r2,__LC_CURRENT	# pass pointer to task struct
+	larl	%r14,.Lsysc_return
+	jg	klp_update_patch_state
+#endif
+
+#
 # _PIF_PER_TRAP is set, call do_per_trap
 #
 .Lsysc_singlestep:
@@ -654,6 +669,10 @@ ENTRY(io_int_handler)
 	jo	.Lio_mcck_pending
 	TSTMSK	__TI_flags(%r12),_TIF_NEED_RESCHED
 	jo	.Lio_reschedule
+#ifdef CONFIG_LIVEPATCH
+	TSTMSK	__TI_flags(%r12),_TIF_PATCH_PENDING
+	jo	.Lio_patch_pending
+#endif
 	TSTMSK	__TI_flags(%r12),_TIF_SIGPENDING
 	jo	.Lio_sigpending
 	TSTMSK	__TI_flags(%r12),_TIF_NOTIFY_RESUME
@@ -700,6 +719,16 @@ ENTRY(io_int_handler)
 	j	.Lio_return
 
 #
+# _TIF_PATCH_PENDING is set, call klp_update_patch_state
+#
+#ifdef CONFIG_LIVEPATCH
+.Lio_patch_pending:
+	lg	%r2,__LC_CURRENT	# pass pointer to task struct
+	larl	%r14,.Lio_return
+	jg	klp_update_patch_state
+#endif
+
+#
 # _TIF_SIGPENDING or is set, call do_signal
 #
 .Lio_sigpending:
-- 
2.7.4

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

#1580213 — [PATCH v5 01/15] stacktrace/x86: add function for detecting reliable stack traces

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

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

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

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

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

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

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

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/Kconfig                   |  6 +++
 arch/x86/Kconfig               |  1 +
 arch/x86/include/asm/unwind.h  |  6 +++
 arch/x86/kernel/stacktrace.c   | 96 +++++++++++++++++++++++++++++++++++++++++-
 arch/x86/kernel/unwind_frame.c |  2 +
 include/linux/stacktrace.h     |  9 ++--
 kernel/stacktrace.c            | 12 +++++-
 7 files changed, 126 insertions(+), 6 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 80f3e5e..478b939 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -749,6 +749,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
+	  Architecture has a save_stack_trace_tsk_reliable() function which
+	  only returns a stack trace if it can guarantee the trace is reliable.
+
 config HAVE_ARCH_HASH
 	bool
 	default n
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ea82a7b..e79fbf8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -160,6 +160,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
 	select HAVE_STACK_VALIDATION		if X86_64
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UNSTABLE_SCHED_CLOCK
diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
index 6fa75b1..137e9cce 100644
--- a/arch/x86/include/asm/unwind.h
+++ b/arch/x86/include/asm/unwind.h
@@ -11,6 +11,7 @@ struct unwind_state {
 	unsigned long stack_mask;
 	struct task_struct *task;
 	int graph_idx;
+	bool error;
 #ifdef CONFIG_FRAME_POINTER
 	unsigned long *bp, *orig_sp;
 	struct pt_regs *regs;
@@ -40,6 +41,11 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
 	__unwind_start(state, task, regs, first_frame);
 }
 
+static inline bool unwind_error(struct unwind_state *state)
+{
+	return state->error;
+}
+
 #ifdef CONFIG_FRAME_POINTER
 
 static inline
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 0653788..c5490d9 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -74,6 +74,101 @@ 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
+
+#define STACKTRACE_DUMP_ONCE(task) ({				\
+	static bool __section(.data.unlikely) __dumped;		\
+								\
+	if (!__dumped) {					\
+		__dumped = true;				\
+		WARN_ON(1);					\
+		show_stack(task, NULL);				\
+	}							\
+})
+
+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) {
+			/*
+			 * Kernel mode registers on the stack indicate an
+			 * in-kernel interrupt or exception (e.g., preemption
+			 * or a page fault), which can make frame pointers
+			 * unreliable.
+			 */
+			if (!user_mode(regs))
+				return -EINVAL;
+
+			/*
+			 * The last frame contains the user mode syscall
+			 * pt_regs.  Skip it and finish the unwind.
+			 */
+			unwind_next_frame(&state);
+			if (!unwind_done(&state)) {
+				STACKTRACE_DUMP_ONCE(task);
+				return -EINVAL;
+			}
+			break;
+		}
+
+		addr = unwind_get_return_address(&state);
+
+		/*
+		 * A NULL or invalid return address probably means there's some
+		 * generated code which __kernel_text_address() doesn't know
+		 * about.
+		 */
+		if (!addr) {
+			STACKTRACE_DUMP_ONCE(task);
+			return -EINVAL;
+		}
+
+		if (save_stack_address(trace, addr, false))
+			return -EINVAL;
+	}
+
+	/* Check for stack corruption */
+	if (unwind_error(&state)) {
+		STACKTRACE_DUMP_ONCE(task);
+		return -EINVAL;
+	}
+
+	if (trace->nr_entries < trace->max_entries)
+		trace->entries[trace->nr_entries++] = ULONG_MAX;
+
+	return 0;
+}
+
+/*
+ * This function returns an error if it detects any unreliable features of the
+ * stack.  Otherwise it guarantees that the stack trace is reliable.
+ *
+ * If the task is not 'current', the caller *must* ensure the task is inactive.
+ */
+int save_stack_trace_tsk_reliable(struct task_struct *tsk,
+				  struct stack_trace *trace)
+{
+	int ret;
+
+	if (!try_get_task_stack(tsk))
+		return -EINVAL;
+
+	ret = __save_stack_trace_reliable(trace, tsk);
+
+	put_task_stack(tsk);
+
+	return ret;
+}
+#endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */
+
 /* Userspace stacktrace - based on kernel/trace/trace_sysprof.c */
 
 struct stack_frame_user {
@@ -136,4 +231,3 @@ void save_stack_trace_user(struct stack_trace *trace)
 	if (trace->nr_entries < trace->max_entries)
 		trace->entries[trace->nr_entries++] = ULONG_MAX;
 }
-
diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 23d1556..b15563d 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -223,6 +223,8 @@ bool unwind_next_frame(struct unwind_state *state)
 	return true;
 
 bad_address:
+	state->error = true;
+
 	/*
 	 * When unwinding a non-current task, the task might actually be
 	 * running on another CPU, in which case it could be modifying its
diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
index 0a34489..4205f71 100644
--- a/include/linux/stacktrace.h
+++ b/include/linux/stacktrace.h
@@ -18,6 +18,8 @@ extern void save_stack_trace_regs(struct pt_regs *regs,
 				  struct stack_trace *trace);
 extern void save_stack_trace_tsk(struct task_struct *tsk,
 				struct stack_trace *trace);
+extern int save_stack_trace_tsk_reliable(struct task_struct *tsk,
+					 struct stack_trace *trace);
 
 extern void print_stack_trace(struct stack_trace *trace, int spaces);
 extern int snprint_stack_trace(char *buf, size_t size,
@@ -29,12 +31,13 @@ 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
+# define save_stack_trace_tsk_reliable(tsk, trace)	({ -ENOSYS; })
+#endif /* CONFIG_STACKTRACE */
 
-#endif
+#endif /* __LINUX_STACKTRACE_H */
diff --git a/kernel/stacktrace.c b/kernel/stacktrace.c
index 9c15a91..f8edee9 100644
--- a/kernel/stacktrace.c
+++ b/kernel/stacktrace.c
@@ -54,8 +54,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 these weak aliases and once-per-bootup warnings
  * (whenever this facility is utilized - for example by procfs):
  */
 __weak void
@@ -69,3 +69,11 @@ save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 {
 	WARN_ONCE(1, KERN_INFO "save_stack_trace_regs() not implemented yet.\n");
 }
+
+__weak int
+save_stack_trace_tsk_reliable(struct task_struct *tsk,
+			      struct stack_trace *trace)
+{
+	WARN_ONCE(1, KERN_INFO "save_stack_tsk_reliable() not implemented yet.\n");
+	return -ENOSYS;
+}
-- 
2.7.4

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

#1581270 — Re: [PATCH v5 01/15] stacktrace/x86: add function for detecting reliable stack traces

On Mon, 13 Feb 2017, Josh Poimboeuf wrote:

> For live patching and possibly other use cases, a stack trace is only
> useful if it can be assured that it's completely reliable.  Add a new
> save_stack_trace_tsk_reliable() function to achieve that.
> 
> Note that if the target task isn't the current task, and the target task
> is allowed to run, then it could be writing the stack while the unwinder
> is reading it, resulting in possible corruption.  So the caller of
> save_stack_trace_tsk_reliable() must ensure that the task is either
> 'current' or inactive.
> 
> save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection
> of pt_regs on the stack.  If the pt_regs are not user-mode registers
> from a syscall, then they indicate an in-kernel interrupt or exception
> (e.g. preemption or a page fault), in which case the stack is considered
> unreliable due to the nature of frame pointers.
> 
> It also relies on the x86 unwinder's detection of other issues, such as:
> 
> - corrupted stack data
> - stack grows the wrong way
> - stack walk doesn't reach the bottom
> - user didn't provide a large enough entries array
> 
> Such issues are reported by checking unwind_error() and !unwind_done().
> 
> Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can
> determine at build time whether the function is implemented.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

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

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

stays.

Regards,
Miroslav

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

#1581375 — Re: [PATCH v5 01/15] stacktrace/x86: add function for detecting reliable stack traces

On Wed, Feb 15, 2017 at 01:18:40PM +0100, Miroslav Benes wrote:
> On Mon, 13 Feb 2017, Josh Poimboeuf wrote:
> 
> > For live patching and possibly other use cases, a stack trace is only
> > useful if it can be assured that it's completely reliable.  Add a new
> > save_stack_trace_tsk_reliable() function to achieve that.
> > 
> > Note that if the target task isn't the current task, and the target task
> > is allowed to run, then it could be writing the stack while the unwinder
> > is reading it, resulting in possible corruption.  So the caller of
> > save_stack_trace_tsk_reliable() must ensure that the task is either
> > 'current' or inactive.
> > 
> > save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection
> > of pt_regs on the stack.  If the pt_regs are not user-mode registers
> > from a syscall, then they indicate an in-kernel interrupt or exception
> > (e.g. preemption or a page fault), in which case the stack is considered
> > unreliable due to the nature of frame pointers.
> > 
> > It also relies on the x86 unwinder's detection of other issues, such as:
> > 
> > - corrupted stack data
> > - stack grows the wrong way
> > - stack walk doesn't reach the bottom
> > - user didn't provide a large enough entries array
> > 
> > Such issues are reported by checking unwind_error() and !unwind_done().
> > 
> > Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can
> > determine at build time whether the function is implemented.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> I do not see any difference from 4.1 version, so my
> 
> Reviewed-by: Miroslav Benes <mbenes@suse.cz>

Sorry, forgot to add your Reviewed-by from last time.  The patch is
indeed the same.  Thanks!

-- 
Josh

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

#1582616 — Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model

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

[...]

> +static void klp_complete_transition(void)
> +{
> +	struct klp_object *obj;
> +	struct klp_func *func;
> +	struct task_struct *g, *task;
> +	unsigned int cpu;
> +
> +	if (klp_target_state == KLP_UNPATCHED) {
> +		/*
> +		 * All tasks have transitioned to KLP_UNPATCHED so we can now
> +		 * remove the new functions from the func_stack.
> +		 */
> +		klp_unpatch_objects(klp_transition_patch);
> +
> +		/*
> +		 * Make sure klp_ftrace_handler() can no longer see functions
> +		 * from this patch on the ops->func_stack.  Otherwise, after
> +		 * func->transition gets cleared, the handler may choose a
> +		 * removed function.
> +		 */
> +		synchronize_rcu();
> +	}
> +
> +	if (klp_transition_patch->immediate)
> +		goto done;
> +
> +	klp_for_each_object(klp_transition_patch, obj)
> +		klp_for_each_func(obj, func)
> +			func->transition = false;
> +
> +	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> +	if (klp_target_state == KLP_PATCHED)
> +		synchronize_rcu();
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process_thread(g, task) {
> +		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> +		task->patch_state = KLP_UNDEFINED;
> +	}
> +	read_unlock(&tasklist_lock);
> +
> +	for_each_possible_cpu(cpu) {
> +		task = idle_task(cpu);
> +		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> +		task->patch_state = KLP_UNDEFINED;
> +	}
> +
> +done:
> +	klp_target_state = KLP_UNDEFINED;
> +	klp_transition_patch = NULL;
> +}
> +
> +/*
> + * This is called in the error path, to cancel a transition before it has
> + * started, i.e. klp_init_transition() has been called but
> + * klp_start_transition() hasn't.  If the transition *has* been started,
> + * klp_reverse_transition() should be used instead.
> + */
> +void klp_cancel_transition(void)
> +{
> +	klp_target_state = !klp_target_state;
> +	klp_complete_transition();
> +}

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

Pity I have not spotted this earlier.

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

What do you think?

Miroslav

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

#1582928 — Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model

On Thu, Feb 16, 2017 at 03:33:26PM +0100, Miroslav Benes wrote:
> 
> > @@ -347,22 +356,36 @@ static int __klp_enable_patch(struct klp_patch *patch)
> >  
> >  	pr_notice("enabling patch '%s'\n", patch->mod->name);
> >  
> > +	klp_init_transition(patch, KLP_PATCHED);
> > +
> > +	/*
> > +	 * Enforce the order of the func->transition writes in
> > +	 * klp_init_transition() and the ops->func_stack writes in
> > +	 * klp_patch_object(), so that klp_ftrace_handler() will see the
> > +	 * func->transition updates before the handler is registered and the
> > +	 * new funcs become visible to the handler.
> > +	 */
> > +	smp_wmb();
> > +
> >  	klp_for_each_object(patch, obj) {
> >  		if (!klp_is_object_loaded(obj))
> >  			continue;
> >  
> >  		ret = klp_patch_object(obj);
> > -		if (ret)
> > -			goto unregister;
> > +		if (ret) {
> > +			pr_warn("failed to enable patch '%s'\n",
> > +				patch->mod->name);
> > +
> > +			klp_cancel_transition();
> > +			return ret;
> > +		}
> 
> [...]
> 
> > +static void klp_complete_transition(void)
> > +{
> > +	struct klp_object *obj;
> > +	struct klp_func *func;
> > +	struct task_struct *g, *task;
> > +	unsigned int cpu;
> > +
> > +	if (klp_target_state == KLP_UNPATCHED) {
> > +		/*
> > +		 * All tasks have transitioned to KLP_UNPATCHED so we can now
> > +		 * remove the new functions from the func_stack.
> > +		 */
> > +		klp_unpatch_objects(klp_transition_patch);
> > +
> > +		/*
> > +		 * Make sure klp_ftrace_handler() can no longer see functions
> > +		 * from this patch on the ops->func_stack.  Otherwise, after
> > +		 * func->transition gets cleared, the handler may choose a
> > +		 * removed function.
> > +		 */
> > +		synchronize_rcu();
> > +	}
> > +
> > +	if (klp_transition_patch->immediate)
> > +		goto done;
> > +
> > +	klp_for_each_object(klp_transition_patch, obj)
> > +		klp_for_each_func(obj, func)
> > +			func->transition = false;
> > +
> > +	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> > +	if (klp_target_state == KLP_PATCHED)
> > +		synchronize_rcu();
> > +
> > +	read_lock(&tasklist_lock);
> > +	for_each_process_thread(g, task) {
> > +		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> > +		task->patch_state = KLP_UNDEFINED;
> > +	}
> > +	read_unlock(&tasklist_lock);
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		task = idle_task(cpu);
> > +		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> > +		task->patch_state = KLP_UNDEFINED;
> > +	}
> > +
> > +done:
> > +	klp_target_state = KLP_UNDEFINED;
> > +	klp_transition_patch = NULL;
> > +}
> > +
> > +/*
> > + * This is called in the error path, to cancel a transition before it has
> > + * started, i.e. klp_init_transition() has been called but
> > + * klp_start_transition() hasn't.  If the transition *has* been started,
> > + * klp_reverse_transition() should be used instead.
> > + */
> > +void klp_cancel_transition(void)
> > +{
> > +	klp_target_state = !klp_target_state;
> > +	klp_complete_transition();
> > +}
> 
> If we fail to enable patch in __klp_enable_patch(), we call 
> klp_cancel_transition() and get to klp_complete_transition(). If the patch 
> has immediate set to true, the module would not be allowed to go (the 
> changes are in the last patch unfortunately, but my remark is closely 
> related to klp_cancel_transition() and __klp_enable_patch()). This could 
> annoy a user if it was due to a trivial reason. So we could call  
> module_put() in this case. It should be safe as no task could be in a new 
> function thanks to klp_ftrace_handler() logic.
> 
> Pity I have not spotted this earlier.
> 
> Putting module_put(patch->mod) right after klp_cancel_transition() call in 
> __klp_enable_patch() would be the simplest fix (as a part of 15/15 patch). 
> Maybe with a comment that it is safe to do it there.
> 
> What do you think?

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

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

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


diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index e96346e..bd1c1fd 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -121,8 +121,28 @@ static void klp_complete_transition(void)
  */
 void klp_cancel_transition(void)
 {
+	bool immediate_func = false;
+
 	klp_target_state = !klp_target_state;
 	klp_complete_transition();
+
+	if (klp_target_state == KLP_PATCHED)
+		return;
+
+	/*
+	 * In the enable error path, even immediate patches can be safely
+	 * removed because the transition hasn't been started yet.
+	 *
+	 * klp_complete_transition() doesn't have a module_put() for immediate
+	 * patches, so do it here.
+	 */
+	klp_for_each_object(klp_transition_patch, obj)
+		klp_for_each_func(obj, func)
+			if (func->immediate)
+				immediate_func = true;
+
+	if (klp_transition_patch->immediate || immediate_func)
+		module_put(klp_transition_patch->mod);
 }
 
 /*

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

#1583224 — Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model

On Thu, 16 Feb 2017, Josh Poimboeuf wrote:

> On Thu, Feb 16, 2017 at 03:33:26PM +0100, Miroslav Benes wrote:
> > 
> > > @@ -347,22 +356,36 @@ static int __klp_enable_patch(struct klp_patch *patch)
> > >  
> > >  	pr_notice("enabling patch '%s'\n", patch->mod->name);
> > >  
> > > +	klp_init_transition(patch, KLP_PATCHED);
> > > +
> > > +	/*
> > > +	 * Enforce the order of the func->transition writes in
> > > +	 * klp_init_transition() and the ops->func_stack writes in
> > > +	 * klp_patch_object(), so that klp_ftrace_handler() will see the
> > > +	 * func->transition updates before the handler is registered and the
> > > +	 * new funcs become visible to the handler.
> > > +	 */
> > > +	smp_wmb();
> > > +
> > >  	klp_for_each_object(patch, obj) {
> > >  		if (!klp_is_object_loaded(obj))
> > >  			continue;
> > >  
> > >  		ret = klp_patch_object(obj);
> > > -		if (ret)
> > > -			goto unregister;
> > > +		if (ret) {
> > > +			pr_warn("failed to enable patch '%s'\n",
> > > +				patch->mod->name);
> > > +
> > > +			klp_cancel_transition();
> > > +			return ret;
> > > +		}
> > 
> > [...]
> > 
> > > +static void klp_complete_transition(void)
> > > +{
> > > +	struct klp_object *obj;
> > > +	struct klp_func *func;
> > > +	struct task_struct *g, *task;
> > > +	unsigned int cpu;
> > > +
> > > +	if (klp_target_state == KLP_UNPATCHED) {
> > > +		/*
> > > +		 * All tasks have transitioned to KLP_UNPATCHED so we can now
> > > +		 * remove the new functions from the func_stack.
> > > +		 */
> > > +		klp_unpatch_objects(klp_transition_patch);
> > > +
> > > +		/*
> > > +		 * Make sure klp_ftrace_handler() can no longer see functions
> > > +		 * from this patch on the ops->func_stack.  Otherwise, after
> > > +		 * func->transition gets cleared, the handler may choose a
> > > +		 * removed function.
> > > +		 */
> > > +		synchronize_rcu();
> > > +	}
> > > +
> > > +	if (klp_transition_patch->immediate)
> > > +		goto done;
> > > +
> > > +	klp_for_each_object(klp_transition_patch, obj)
> > > +		klp_for_each_func(obj, func)
> > > +			func->transition = false;
> > > +
> > > +	/* Prevent klp_ftrace_handler() from seeing KLP_UNDEFINED state */
> > > +	if (klp_target_state == KLP_PATCHED)
> > > +		synchronize_rcu();
> > > +
> > > +	read_lock(&tasklist_lock);
> > > +	for_each_process_thread(g, task) {
> > > +		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> > > +		task->patch_state = KLP_UNDEFINED;
> > > +	}
> > > +	read_unlock(&tasklist_lock);
> > > +
> > > +	for_each_possible_cpu(cpu) {
> > > +		task = idle_task(cpu);
> > > +		WARN_ON_ONCE(test_tsk_thread_flag(task, TIF_PATCH_PENDING));
> > > +		task->patch_state = KLP_UNDEFINED;
> > > +	}
> > > +
> > > +done:
> > > +	klp_target_state = KLP_UNDEFINED;
> > > +	klp_transition_patch = NULL;
> > > +}
> > > +
> > > +/*
> > > + * This is called in the error path, to cancel a transition before it has
> > > + * started, i.e. klp_init_transition() has been called but
> > > + * klp_start_transition() hasn't.  If the transition *has* been started,
> > > + * klp_reverse_transition() should be used instead.
> > > + */
> > > +void klp_cancel_transition(void)
> > > +{
> > > +	klp_target_state = !klp_target_state;
> > > +	klp_complete_transition();
> > > +}
> > 
> > If we fail to enable patch in __klp_enable_patch(), we call 
> > klp_cancel_transition() and get to klp_complete_transition(). If the patch 
> > has immediate set to true, the module would not be allowed to go (the 
> > changes are in the last patch unfortunately, but my remark is closely 
> > related to klp_cancel_transition() and __klp_enable_patch()). This could 
> > annoy a user if it was due to a trivial reason. So we could call  
> > module_put() in this case. It should be safe as no task could be in a new 
> > function thanks to klp_ftrace_handler() logic.
> > 
> > Pity I have not spotted this earlier.
> > 
> > Putting module_put(patch->mod) right after klp_cancel_transition() call in 
> > __klp_enable_patch() would be the simplest fix (as a part of 15/15 patch). 
> > Maybe with a comment that it is safe to do it there.
> > 
> > What do you think?
> 
> Good catch.  I agree that 15/15 should have something like that.
> 
> Also, the module_put() will be needed for non-immediate patches which
> have a func->immediate set.

Right. This further complicates it.
 
> What do you think about the following?  I tried to put the logic in
> klp_complete_transition(), so the module_put()'s would be in one place.
> But it was too messy, so I put it in klp_cancel_transition() instead.
>
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index e96346e..bd1c1fd 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -121,8 +121,28 @@ static void klp_complete_transition(void)
>   */
>  void klp_cancel_transition(void)
>  {
> +	bool immediate_func = false;
> +
>  	klp_target_state = !klp_target_state;
>  	klp_complete_transition();
> +
> +	if (klp_target_state == KLP_PATCHED)
> +		return;

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

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

> +
> +	/*
> +	 * In the enable error path, even immediate patches can be safely
> +	 * removed because the transition hasn't been started yet.
> +	 *
> +	 * klp_complete_transition() doesn't have a module_put() for immediate
> +	 * patches, so do it here.
> +	 */
> +	klp_for_each_object(klp_transition_patch, obj)
> +		klp_for_each_func(obj, func)
> +			if (func->immediate)
> +				immediate_func = true;
> +
> +	if (klp_transition_patch->immediate || immediate_func)
> +		module_put(klp_transition_patch->mod);

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

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

Miroslav

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

#1585742 — Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model

On Fri, Feb 17, 2017 at 09:51:29AM +0100, Miroslav Benes wrote:
> On Thu, 16 Feb 2017, Josh Poimboeuf wrote:
> > What do you think about the following?  I tried to put the logic in
> > klp_complete_transition(), so the module_put()'s would be in one place.
> > But it was too messy, so I put it in klp_cancel_transition() instead.
> >
> > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > index e96346e..bd1c1fd 100644
> > --- a/kernel/livepatch/transition.c
> > +++ b/kernel/livepatch/transition.c
> > @@ -121,8 +121,28 @@ static void klp_complete_transition(void)
> >   */
> >  void klp_cancel_transition(void)
> >  {
> > +	bool immediate_func = false;
> > +
> >  	klp_target_state = !klp_target_state;
> >  	klp_complete_transition();
> > +
> > +	if (klp_target_state == KLP_PATCHED)
> > +		return;
> 
> This is not needed, I think. We call klp_cancel_transition() in 
> __klp_enable_patch() only. klp_target_state is set to KLP_PATCHED there 
> (through klp_init_transition()) and negated here. We know it must be 
> KLP_UNPATCHED.

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

> Moreover, due to klp_complete_transition() klp_target_state is always 
> KLP_UNDEFINED after it.
> 
> > +
> > +	/*
> > +	 * In the enable error path, even immediate patches can be safely
> > +	 * removed because the transition hasn't been started yet.
> > +	 *
> > +	 * klp_complete_transition() doesn't have a module_put() for immediate
> > +	 * patches, so do it here.
> > +	 */
> > +	klp_for_each_object(klp_transition_patch, obj)
> > +		klp_for_each_func(obj, func)
> > +			if (func->immediate)
> > +				immediate_func = true;
> > +
> > +	if (klp_transition_patch->immediate || immediate_func)
> > +		module_put(klp_transition_patch->mod);
> 
> Almost correct. The only problem is that klp_transition_patch is NULL at 
> this point. klp_complete_transition() does that and it should stay there 
> in my opinion to keep it simple.
> 
> So we could either move all this to __klp_enable_patch(), where patch 
> variable is defined, or we could store klp_transition_patch to a local 
> variable here in klp_cancel_transition() before klp_complete_transition() 
> is called. That should be safe. I like the latter more, because it keeps 
> the code in klp_cancel_transition() where it belongs.

Good points.  Here's another try:

diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index e96346e..a23c63c 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -121,8 +121,31 @@ static void klp_complete_transition(void)
  */
 void klp_cancel_transition(void)
 {
-	klp_target_state = !klp_target_state;
+	struct klp_patch *patch = klp_transition_patch;
+	struct klp_object *obj;
+	struct klp_func *func;
+	bool immediate_func = false;
+
+	if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
+		return;
+
+	klp_target_state = KLP_UNPATCHED;
 	klp_complete_transition();
+
+	/*
+	 * In the enable error path, even immediate patches can be safely
+	 * removed because the transition hasn't been started yet.
+	 *
+	 * klp_complete_transition() doesn't have a module_put() for immediate
+	 * patches, so do it here.
+	 */
+	klp_for_each_object(patch, obj)
+		klp_for_each_func(obj, func)
+			if (func->immediate)
+				immediate_func = true;
+
+	if (patch->immediate || immediate_func)
+		module_put(patch->mod);
 }
 
 /*

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

#1586004 — Re: [PATCH v5 13/15] livepatch: change to a per-task consistency model

On Tue, 21 Feb 2017, Josh Poimboeuf wrote:

> On Fri, Feb 17, 2017 at 09:51:29AM +0100, Miroslav Benes wrote:
> > On Thu, 16 Feb 2017, Josh Poimboeuf wrote:
> > > What do you think about the following?  I tried to put the logic in
> > > klp_complete_transition(), so the module_put()'s would be in one place.
> > > But it was too messy, so I put it in klp_cancel_transition() instead.
> > >
> > > diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> > > index e96346e..bd1c1fd 100644
> > > --- a/kernel/livepatch/transition.c
> > > +++ b/kernel/livepatch/transition.c
> > > @@ -121,8 +121,28 @@ static void klp_complete_transition(void)
> > >   */
> > >  void klp_cancel_transition(void)
> > >  {
> > > +	bool immediate_func = false;
> > > +
> > >  	klp_target_state = !klp_target_state;
> > >  	klp_complete_transition();
> > > +
> > > +	if (klp_target_state == KLP_PATCHED)
> > > +		return;
> > 
> > This is not needed, I think. We call klp_cancel_transition() in 
> > __klp_enable_patch() only. klp_target_state is set to KLP_PATCHED there 
> > (through klp_init_transition()) and negated here. We know it must be 
> > KLP_UNPATCHED.
> 
> Yeah, I was trying to hedge against the possibility of future code
> calling this function in the disable path.  But that probably won't
> happen and it would probably be cleaner to just add a warning if
> klp_target_state isn't KLP_PATCHED.
> 
> > Moreover, due to klp_complete_transition() klp_target_state is always 
> > KLP_UNDEFINED after it.
> > 
> > > +
> > > +	/*
> > > +	 * In the enable error path, even immediate patches can be safely
> > > +	 * removed because the transition hasn't been started yet.
> > > +	 *
> > > +	 * klp_complete_transition() doesn't have a module_put() for immediate
> > > +	 * patches, so do it here.
> > > +	 */
> > > +	klp_for_each_object(klp_transition_patch, obj)
> > > +		klp_for_each_func(obj, func)
> > > +			if (func->immediate)
> > > +				immediate_func = true;
> > > +
> > > +	if (klp_transition_patch->immediate || immediate_func)
> > > +		module_put(klp_transition_patch->mod);
> > 
> > Almost correct. The only problem is that klp_transition_patch is NULL at 
> > this point. klp_complete_transition() does that and it should stay there 
> > in my opinion to keep it simple.
> > 
> > So we could either move all this to __klp_enable_patch(), where patch 
> > variable is defined, or we could store klp_transition_patch to a local 
> > variable here in klp_cancel_transition() before klp_complete_transition() 
> > is called. That should be safe. I like the latter more, because it keeps 
> > the code in klp_cancel_transition() where it belongs.
> 
> Good points.  Here's another try:
> 
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index e96346e..a23c63c 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -121,8 +121,31 @@ static void klp_complete_transition(void)
>   */
>  void klp_cancel_transition(void)
>  {
> -	klp_target_state = !klp_target_state;
> +	struct klp_patch *patch = klp_transition_patch;
> +	struct klp_object *obj;
> +	struct klp_func *func;
> +	bool immediate_func = false;
> +
> +	if (WARN_ON_ONCE(klp_target_state != KLP_PATCHED))
> +		return;
> +
> +	klp_target_state = KLP_UNPATCHED;
>  	klp_complete_transition();
> +
> +	/*
> +	 * In the enable error path, even immediate patches can be safely
> +	 * removed because the transition hasn't been started yet.
> +	 *
> +	 * klp_complete_transition() doesn't have a module_put() for immediate
> +	 * patches, so do it here.
> +	 */
> +	klp_for_each_object(patch, obj)
> +		klp_for_each_func(obj, func)
> +			if (func->immediate)
> +				immediate_func = true;
> +
> +	if (patch->immediate || immediate_func)
> +		module_put(patch->mod);
>  }

Looks ok.

Thanks,
Miroslav

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

#1583223

On Mon, 13 Feb 2017, Josh Poimboeuf wrote:

> Here's v5 of the consistency model, targeted for 4.12.  Only a few minor
> changes this time.
> 
> v5:
> - return -EINVAL in __save_stack_trace_reliable()
> - only call show_stack() once
> - add save_stack_trace_tsk_reliable() define for !CONFIG_STACKTRACE
> - update kernel version and date in ABI doc
> - make suggested improvements to livepatch.txt
> - update barrier comments
> - remove klp_try_complete_transition() call from klp_start_transition()
> - move end of klp_try_complete_transition() into klp_complete_transition()
> - fix __klp_enable_patch() error path
> - check for transition in klp_module_going()

All the changes since v4 look good to me (except for module_put() in 
__klp_enable_patch() mentioned elsewhere).

Thanks,
Miroslav

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web