This is v2 of the livepatch hybrid consistency model, based on
linux-next/master.
v1 of this patch set was posted over a year ago:
https://lkml.kernel.org/r/[email protected]
The biggest complaint at that time was that stack traces are unreliable.
Since CONFIG_STACK_VALIDATION was merged, that issue has been addressed.
I've also tried to address all other outstanding complaints and issues.
Ingo and Peter, note that I'm using task_rq_lock() in patch 17/18 to
make sure a task stays asleep while its stack gets checked. I'm not
sure if there's a better way to achieve that goal -- any suggestions
there would be greatly appreciated.
Patches 1-7 create a mechanism for detecting whether a given stack trace
can be deemed reliable.
Patches 8-18 add the consistency model. See patch 17/18 for more
details about the consistency model itself.
Remaining TODOs:
- how to patch kthreads without RELIABLE_STACKTRACE?
- safe patch module removal
- fake signal facility
- allow user to force a task to the patched state
- enable the patching of kthreads which are sleeping on affected
functions, via the livepatch ftrace handler
- WARN on certain stack error conditions
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 (16):
x86/asm/head: clean up 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: add task flag for preempt IRQ tracking
x86: dump_trace() error handling
stacktrace/x86: function for detecting reliable stack traces
livepatch: temporary stubs for klp_patch_pending() and
klp_patch_task()
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: store function sizes
livepatch: change to a per-task consistency model
livepatch: add /proc/<pid>/patch_state
Miroslav Benes (1):
livepatch/s390: add TIF_PATCH_PENDING thread flag
Documentation/ABI/testing/sysfs-kernel-livepatch | 8 +
Documentation/filesystems/proc.txt | 18 +
Documentation/livepatch/livepatch.txt | 132 ++++++-
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/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 | 108 +++++-
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 | 34 +-
arch/x86/kernel/smpboot.c | 2 +-
arch/x86/kernel/stacktrace.c | 24 ++
arch/x86/kernel/vmlinux.lds.S | 2 +-
fs/proc/base.c | 15 +
include/linux/init_task.h | 9 +
include/linux/kernel.h | 1 +
include/linux/livepatch.h | 57 ++-
include/linux/sched.h | 4 +
include/linux/stacktrace.h | 20 +-
kernel/extable.c | 2 +-
kernel/fork.c | 5 +-
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 342 +++++-----------
kernel/livepatch/patch.c | 254 ++++++++++++
kernel/livepatch/patch.h | 33 ++
kernel/livepatch/transition.c | 474 +++++++++++++++++++++++
kernel/livepatch/transition.h | 14 +
kernel/sched/core.c | 4 +
kernel/sched/idle.c | 4 +
kernel/stacktrace.c | 4 +-
lib/Kconfig.debug | 6 +
41 files changed, 1413 insertions(+), 372 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.11
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 <[email protected]>
---
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 792c3bb..6dbd2c0 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -264,13 +264,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
@@ -299,6 +301,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"
@@ -306,15 +309,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.11
Create temporary stubs for klp_patch_pending() and klp_patch_task() so
we can add TIF_PATCH_PENDING to different architectures in separate
patches without breaking build bisectability.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
include/linux/livepatch.h | 7 ++++++-
kernel/livepatch/core.c | 3 +++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0933ca4..a8c6c9c 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -118,10 +118,15 @@ int klp_disable_patch(struct klp_patch *);
int klp_module_coming(struct module *mod);
void klp_module_going(struct module *mod);
+static inline bool klp_patch_pending(struct task_struct *task) { return false; }
+void klp_patch_task(struct task_struct *task);
+
#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_module_going(struct module *mod) {}
+static inline bool klp_patch_pending(struct task_struct *task) { return false; }
+static inline void klp_patch_task(struct task_struct *task) {}
#endif /* CONFIG_LIVEPATCH */
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a19f195..6ea6880 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -64,6 +64,9 @@ static LIST_HEAD(klp_ops);
static struct kobject *klp_root_kobj;
+/* TODO: temporary stub */
+void klp_patch_task(struct task_struct *task) {}
+
static struct klp_ops *klp_find_ops(unsigned long old_addr)
{
struct klp_ops *ops;
--
2.4.11
Add the TIF_PATCH_PENDING thread flag to enable the new livepatch
per-task consistency model for powerpc. The bit getting set indicates
the thread has a pending patch which needs to be applied when the thread
exits the kernel.
The bit is included in the _TIF_USER_WORK_MASK macro so that
do_notify_resume() and klp_patch_task() get called when the bit is set.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/powerpc/include/asm/thread_info.h | 4 +++-
arch/powerpc/kernel/signal.c | 4 ++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index 8febc3f..df262ca 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -88,6 +88,7 @@ static inline struct thread_info *current_thread_info(void)
TIF_NEED_RESCHED */
#define TIF_32BIT 4 /* 32 bit binary */
#define TIF_RESTORE_TM 5 /* need to restore TM FP/VEC/VSX */
+#define TIF_PATCH_PENDING 6 /* pending live patching update */
#define TIF_SYSCALL_AUDIT 7 /* syscall auditing active */
#define TIF_SINGLESTEP 8 /* singlestepping active */
#define TIF_NOHZ 9 /* in adaptive nohz mode */
@@ -111,6 +112,7 @@ static inline struct thread_info *current_thread_info(void)
#define _TIF_POLLING_NRFLAG (1<<TIF_POLLING_NRFLAG)
#define _TIF_32BIT (1<<TIF_32BIT)
#define _TIF_RESTORE_TM (1<<TIF_RESTORE_TM)
+#define _TIF_PATCH_PENDING (1<<TIF_PATCH_PENDING)
#define _TIF_SYSCALL_AUDIT (1<<TIF_SYSCALL_AUDIT)
#define _TIF_SINGLESTEP (1<<TIF_SINGLESTEP)
#define _TIF_SECCOMP (1<<TIF_SECCOMP)
@@ -127,7 +129,7 @@ static inline struct thread_info *current_thread_info(void)
#define _TIF_USER_WORK_MASK (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
_TIF_NOTIFY_RESUME | _TIF_UPROBE | \
- _TIF_RESTORE_TM)
+ _TIF_RESTORE_TM | _TIF_PATCH_PENDING)
#define _TIF_PERSYSCALL_MASK (_TIF_RESTOREALL|_TIF_NOERROR)
/* Bits in local_flags */
diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index cb64d6f..844497b 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -14,6 +14,7 @@
#include <linux/uprobes.h>
#include <linux/key.h>
#include <linux/context_tracking.h>
+#include <linux/livepatch.h>
#include <asm/hw_breakpoint.h>
#include <asm/uaccess.h>
#include <asm/unistd.h>
@@ -159,6 +160,9 @@ void do_notify_resume(struct pt_regs *regs, unsigned long thread_info_flags)
tracehook_notify_resume(regs);
}
+ if (thread_info_flags & _TIF_PATCH_PENDING)
+ klp_patch_task(current);
+
user_enter();
}
--
2.4.11
klp_patch_object()'s callers already ensure that the object is loaded,
so its call to klp_is_object_loaded() is unnecessary.
This will also make it possible to move the patching code into a
separate file.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
kernel/livepatch/core.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 2b59230..2ad7892 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -469,9 +469,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.11
From: Miroslav Benes <[email protected]>
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 <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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 8642c1d..b69d538 100644
--- a/arch/s390/include/asm/thread_info.h
+++ b/arch/s390/include/asm/thread_info.h
@@ -75,6 +75,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#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 */
@@ -93,6 +94,7 @@ void arch_release_task_struct(struct task_struct *tsk);
#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 2d47f9c..5db5959 100644
--- a/arch/s390/kernel/entry.S
+++ b/arch/s390/kernel/entry.S
@@ -46,7 +46,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)
@@ -329,6 +329,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
@@ -404,6 +409,16 @@ ENTRY(system_call)
#endif
#
+# _TIF_PATCH_PENDING is set, call klp_patch_task
+#
+#ifdef CONFIG_LIVEPATCH
+.Lsysc_patch_pending:
+ lg %r2,__TI_task(%r12)
+ larl %r14,.Lsysc_return
+ jg klp_patch_task
+#endif
+
+#
# _PIF_PER_TRAP is set, call do_per_trap
#
.Lsysc_singlestep:
@@ -652,6 +667,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
@@ -698,6 +717,16 @@ ENTRY(io_int_handler)
j .Lio_return
#
+# _TIF_PATCH_PENDING is set, call klp_patch_task
+#
+#ifdef CONFIG_LIVEPATCH
+.Lio_patch_pending:
+ lg %r2,__TI_task(%r12)
+ larl %r14,.Lio_return
+ jg klp_patch_task
+#endif
+
+#
# _TIF_SIGPENDING or is set, call do_signal
#
.Lio_sigpending:
--
2.4.11
For the consistency model we'll need to know the sizes of the old and
new functions to determine if they're on the stacks of any tasks.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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 9ba26c5..c38c694 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 f28504d..aa3dbdf 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -577,6 +577,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.11
Once we have a consistency model, patches and their objects will be
enabled and disabled at different times. For example, when a patch is
disabled, its loaded objects' funcs can remain registered with ftrace
indefinitely until the unpatching operation is complete and they're no
longer in use.
It's less confusing if we give them different names: patches can be
enabled or disabled; objects (and their funcs) can be patched or
unpatched:
- Enabled means that a patch is logically enabled (but not necessarily
fully applied).
- Patched means that an object's funcs are registered with ftrace and
added to the klp_ops func stack.
Also, since these states are binary, represent them with booleans
instead of ints.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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 a8c6c9c..9ba26c5 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;
};
/**
@@ -71,7 +66,7 @@ struct klp_func {
* @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 added to the klp_ops list
*/
struct klp_object {
/* external */
@@ -81,7 +76,7 @@ struct klp_object {
/* internal */
struct kobject kobj;
struct module *mod;
- enum klp_state state;
+ bool patched;
};
/**
@@ -90,7 +85,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 */
@@ -100,7 +95,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 6ea6880..2b59230 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -350,11 +350,11 @@ static unsigned long klp_get_ftrace_location(unsigned long faddr)
}
#endif
-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;
@@ -380,10 +380,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;
@@ -391,7 +391,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);
@@ -439,7 +439,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;
@@ -450,36 +450,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;
}
@@ -490,17 +490,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;
}
@@ -524,7 +524,7 @@ int klp_disable_patch(struct klp_patch *patch)
goto err;
}
- if (patch->state == KLP_DISABLED) {
+ if (!patch->enabled) {
ret = -EINVAL;
goto err;
}
@@ -542,12 +542,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");
@@ -559,12 +559,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;
@@ -622,20 +622,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;
@@ -660,7 +660,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);
@@ -751,7 +751,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
@@ -794,7 +794,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);
@@ -835,7 +835,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);
@@ -881,7 +881,7 @@ int klp_unregister_patch(struct klp_patch *patch)
goto out;
}
- if (patch->state == KLP_ENABLED) {
+ if (patch->enabled) {
ret = -EBUSY;
goto out;
}
@@ -968,13 +968,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);
@@ -1025,10 +1025,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.11
Expose the per-task patch state value so users can determine which tasks
are holding up completion of a patching operation.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
Documentation/filesystems/proc.txt | 18 ++++++++++++++++++
fs/proc/base.c | 15 +++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index e8d0075..0b09495 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -44,6 +44,7 @@ Table of Contents
3.8 /proc/<pid>/fdinfo/<fd> - Information about opened file
3.9 /proc/<pid>/map_files - Information about memory mapped files
3.10 /proc/<pid>/timerslack_ns - Task timerslack value
+ 3.11 /proc/<pid>/patch_state - Livepatch patch operation state
4 Configuring procfs
4.1 Mount options
@@ -1880,6 +1881,23 @@ Valid values are from 0 - ULLONG_MAX
An application setting the value must have PTRACE_MODE_ATTACH_FSCREDS level
permissions on the task specified to change its timerslack_ns value.
+3.11 /proc/<pid>/patch_state - Livepatch patch operation state
+-----------------------------------------------------------------
+When CONFIG_LIVEPATCH is enabled, this file displays the value of the
+patch state for the task.
+
+A value of '-1' indicates that no patch is in transition.
+
+A value of '0' indicates that a patch is in transition and the task is
+unpatched. If the patch is being enabled, then the task hasn't been
+patched yet. If the patch is being disabled, then the task has already
+been unpatched.
+
+A value of '1' indicates that a patch is in transition and the task is
+patched. If the patch is being enabled, then the task has already been
+patched. If the patch is being disabled, then the task hasn't been
+unpatched yet.
+
------------------------------------------------------------------------------
Configuring procfs
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2868cdf..c485450 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_state(struct seq_file *m, struct pid_namespace *ns,
+ struct pid *pid, struct task_struct *task)
+{
+ seq_printf(m, "%d\n", task->patch_state);
+ 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_state", S_IRUSR, proc_pid_patch_state),
+#endif
};
static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
@@ -3282,6 +3294,9 @@ static const struct pid_entry tid_base_stuff[] = {
REG("projid_map", S_IRUGO|S_IWUSR, proc_projid_map_operations),
REG("setgroups", S_IRUGO|S_IWUSR, proc_setgroups_operations),
#endif
+#ifdef CONFIG_LIVEPATCH
+ ONE("patch_state", S_IRUSR, proc_pid_patch_state),
+#endif
};
static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)
--
2.4.11
Change livepatch to use a basic per-task consistency model. This is the
foundation which will eventually enable us to patch those ~10% of
security patches which change function or data semantics. This is the
biggest remaining piece needed to make livepatch more generally useful.
This code stems from the design proposal made by Vojtech [1] in November
2014. It's a hybrid of kGraft and kpatch: it uses kGraft's per-task
consistency and syscall barrier switching combined with kpatch's stack
trace switching. There are also a number of fallback options which make
it quite flexible.
Patches are applied on a per-task basis, when the task is deemed safe to
switch over. When a patch is enabled, livepatch enters into a
transition state where tasks are converging to the patched state.
Usually this transition state can complete in a few seconds. The same
sequence occurs when a patch is disabled, except the tasks converge from
the patched state to the unpatched state.
An interrupt handler inherits the patched state of the task it
interrupts. The same is true for forked tasks: the child inherits the
patched state of the parent.
Livepatch uses several complementary approaches to determine when it's
safe to patch tasks:
1. The first and most effective approach is stack checking of sleeping
tasks. If no affected functions are on the stack of a given task,
the task is patched. In most cases this will patch most or all of
the tasks on the first try. Otherwise it'll keep trying
periodically. This option is only available if the architecture has
reliable stacks (CONFIG_RELIABLE_STACKTRACE and
CONFIG_STACK_VALIDATION).
2. The second approach, if needed, is kernel exit switching. A
task is switched when it returns to user space from a system call, a
user space IRQ, or a signal. It's useful in the following cases:
a) Patching I/O-bound user tasks which are sleeping on an affected
function. In this case you have to send SIGSTOP and SIGCONT to
force it to exit the kernel and be patched.
b) Patching CPU-bound user tasks. If the task is highly CPU-bound
then it will get patched the next time it gets interrupted by an
IRQ.
c) Applying patches for architectures which don't yet have
CONFIG_RELIABLE_STACKTRACE. In this case you'll have to signal
most of the tasks on the system. However this isn't a complete
solution, because there's currently no way to patch kthreads
without CONFIG_RELIABLE_STACKTRACE.
Note: since idle "swapper" tasks don't ever exit the kernel, they
instead have a kpatch_patch_task() call in the idle loop which allows
them to patched before the CPU enters the idle state.
3. A third approach (not yet implemented) is planned for the case where
a kthread is sleeping on an affected function. In that case we could
kick the kthread with a signal and then try to patch the task from
the to-be-patched function's livepatch ftrace handler when it
re-enters the function. This will require
CONFIG_RELIABLE_STACKTRACE.
All the above approaches may be skipped by setting the 'immediate' flag
in the 'klp_patch' struct, which will patch all tasks immediately. This
can be useful if the patch doesn't change any function or data
semantics. Note that, even with this flag set, it's possible that some
tasks may still be running with an old version of the function, until
that function returns.
There's also an 'immediate' flag in the 'klp_func' struct which allows
you to specify that certain functions in the patch can be applied
without per-task consistency. This might be useful if you want to patch
a common function like schedule(), and the function change doesn't need
consistency but the rest of the patch does.
For architectures which don't have CONFIG_RELIABLE_STACKTRACE, there
are two options:
a) the user can set the patch->immediate flag which causes all tasks to
be patched immediately. This option should be used with care, only
when the patch doesn't change any function or data semantics; or
b) use the kernel exit switching approach (this is the default).
Note the patching will never complete because there's no currently no
way to patch kthreads without CONFIG_RELIABLE_STACKTRACE.
The /sys/kernel/livepatch/<patch>/transition file shows whether a patch
is in transition. Only a single patch (the topmost patch on the stack)
can be in transition at a given time. A patch can remain in transition
indefinitely, if any of the tasks are stuck in the initial patch state.
A transition can be reversed and effectively canceled by writing the
opposite value to the /sys/kernel/livepatch/<patch>/enabled file while
the transition is in progress. Then all the tasks will attempt to
converge back to the original patch state.
[1] https://lkml.kernel.org/r/[email protected]
Signed-off-by: Josh Poimboeuf <[email protected]>
---
Documentation/ABI/testing/sysfs-kernel-livepatch | 8 +
Documentation/livepatch/livepatch.txt | 132 ++++++-
include/linux/init_task.h | 9 +
include/linux/livepatch.h | 34 +-
include/linux/sched.h | 3 +
kernel/fork.c | 3 +
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 98 +++--
kernel/livepatch/patch.c | 43 +-
kernel/livepatch/patch.h | 1 +
kernel/livepatch/transition.c | 474 +++++++++++++++++++++++
kernel/livepatch/transition.h | 14 +
kernel/sched/idle.c | 4 +
13 files changed, 781 insertions(+), 44 deletions(-)
create mode 100644 kernel/livepatch/transition.c
create mode 100644 kernel/livepatch/transition.h
diff --git a/Documentation/ABI/testing/sysfs-kernel-livepatch b/Documentation/ABI/testing/sysfs-kernel-livepatch
index da87f43..24ca6df 100644
--- a/Documentation/ABI/testing/sysfs-kernel-livepatch
+++ b/Documentation/ABI/testing/sysfs-kernel-livepatch
@@ -25,6 +25,14 @@ Description:
code is currently applied. Writing 0 will disable the patch
while writing 1 will re-enable the patch.
+What: /sys/kernel/livepatch/<patch>/transition
+Date: May 2016
+KernelVersion: 4.7.0
+Contact: [email protected]
+Description:
+ An attribute which indicates whether the patch is currently in
+ transition.
+
What: /sys/kernel/livepatch/<patch>/<object>
Date: Nov 2014
KernelVersion: 3.19.0
diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 6c43f6e..bee86d0 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -72,7 +72,8 @@ example, they add a NULL pointer or a boundary check, fix a race by adding
a missing memory barrier, or add some locking around a critical section.
Most of these changes are self contained and the function presents itself
the same way to the rest of the system. In this case, the functions might
-be updated independently one by one.
+be updated independently one by one. (This can be done by setting the
+'immediate' flag in the klp_patch struct.)
But there are more complex fixes. For example, a patch might change
ordering of locking in multiple functions at the same time. Or a patch
@@ -86,20 +87,103 @@ or no data are stored in the modified structures at the moment.
The theory about how to apply functions a safe way is rather complex.
The aim is to define a so-called consistency model. It attempts to define
conditions when the new implementation could be used so that the system
-stays consistent. The theory is not yet finished. See the discussion at
-http://thread.gmane.org/gmane.linux.kernel/1823033/focus=1828189
-
-The current consistency model is very simple. It guarantees that either
-the old or the new function is called. But various functions get redirected
-one by one without any synchronization.
-
-In other words, the current implementation _never_ modifies the behavior
-in the middle of the call. It is because it does _not_ rewrite the entire
-function in the memory. Instead, the function gets redirected at the
-very beginning. But this redirection is used immediately even when
-some other functions from the same patch have not been redirected yet.
-
-See also the section "Limitations" below.
+stays consistent.
+
+Livepatch has a consistency model which is a hybrid of kGraft and
+kpatch: it uses kGraft's per-task consistency and syscall barrier
+switching combined with kpatch's stack trace switching. There are also
+a number of fallback options which make it quite flexible.
+
+Patches are applied on a per-task basis, when the task is deemed safe to
+switch over. When a patch is enabled, livepatch enters into a
+transition state where tasks are converging to the patched state.
+Usually this transition state can complete in a few seconds. The same
+sequence occurs when a patch is disabled, except the tasks converge from
+the patched state to the unpatched state.
+
+An interrupt handler inherits the patched state of the task it
+interrupts. The same is true for forked tasks: the child inherits the
+patched state of the parent.
+
+Livepatch uses several complementary approaches to determine when it's
+safe to patch tasks:
+
+1. The first and most effective approach is stack checking of sleeping
+ tasks. If no affected functions are on the stack of a given task,
+ the task is patched. In most cases this will patch most or all of
+ the tasks on the first try. Otherwise it'll keep trying
+ periodically. This option is only available if the architecture has
+ reliable stacks (CONFIG_RELIABLE_STACKTRACE and
+ CONFIG_STACK_VALIDATION).
+
+2. The second approach, if needed, is kernel exit switching. A
+ task is switched when it returns to user space from a system call, a
+ user space IRQ, or a signal. It's useful in the following cases:
+
+ a) Patching I/O-bound user tasks which are sleeping on an affected
+ function. In this case you have to send SIGSTOP and SIGCONT to
+ force it to exit the kernel and be patched.
+ b) Patching CPU-bound user tasks. If the task is highly CPU-bound
+ then it will get patched the next time it gets interrupted by an
+ IRQ.
+ c) Applying patches for architectures which don't yet have
+ CONFIG_RELIABLE_STACKTRACE. In this case you'll have to signal
+ most of the tasks on the system. However this isn't a complete
+ solution, because there's currently no way to patch kthreads
+ without CONFIG_RELIABLE_STACKTRACE.
+
+ Note: since idle "swapper" tasks don't ever exit the kernel, they
+ instead have a kpatch_patch_task() call in the idle loop which allows
+ them to patched before the CPU enters the idle state.
+
+3. A third approach (not yet implemented) is planned for the case where
+ a kthread is sleeping on an affected function. In that case we could
+ kick the kthread with a signal and then try to patch the task from
+ the to-be-patched function's livepatch ftrace handler when it
+ re-enters the function. This will require
+ CONFIG_RELIABLE_STACKTRACE.
+
+All the above approaches may be skipped by setting the 'immediate' flag
+in the 'klp_patch' struct, which will patch all tasks immediately. This
+can be useful if the patch doesn't change any function or data
+semantics. Note that, even with this flag set, it's possible that some
+tasks may still be running with an old version of the function, until
+that function returns.
+
+There's also an 'immediate' flag in the 'klp_func' struct which allows
+you to specify that certain functions in the patch can be applied
+without per-task consistency. This might be useful if you want to patch
+a common function like schedule(), and the function change doesn't need
+consistency but the rest of the patch does.
+
+For architectures which don't have CONFIG_RELIABLE_STACKTRACE, there
+are two options:
+
+a) the user can set the patch->immediate flag which causes all tasks to
+ be patched immediately. This option should be used with care, only
+ when the patch doesn't change any function or data semantics; or
+
+b) use the kernel exit switching approach (this is the default).
+ Note the patching will never complete because there's no currently no
+ way to patch kthreads without CONFIG_RELIABLE_STACKTRACE.
+
+The /sys/kernel/livepatch/<patch>/transition file shows whether a patch
+is in transition. Only a single patch (the topmost patch on the stack)
+can be in transition at a given time. A patch can remain in transition
+indefinitely, if any of the tasks are stuck in the initial patch state.
+
+A transition can be reversed and effectively canceled by writing the
+opposite value to the /sys/kernel/livepatch/<patch>/enabled file while
+the transition is in progress. Then all the tasks will attempt to
+converge back to the original patch state.
+
+There's also a /proc/<pid>/patch_state file which can be used to
+determine which tasks are blocking completion of a patching operation.
+If a patch is in transition, this file shows 0 to indicate the task is
+unpatched and 1 to indicate it's patched. Otherwise, if no patch is in
+transition, it shows -1. Any tasks which are blocking the transition
+can be signaled with SIGSTOP and SIGCONT to force them to change their
+patched state.
4. Livepatch module
@@ -239,9 +323,15 @@ Registered patches might be enabled either by calling klp_enable_patch() or
by writing '1' to /sys/kernel/livepatch/<name>/enabled. The system will
start using the new implementation of the patched functions at this stage.
-In particular, if an original function is patched for the first time, a
-function specific struct klp_ops is created and an universal ftrace handler
-is registered.
+When a patch is enabled, livepatch enters into a transition state where
+tasks are converging to the patched state. This is indicated by a value
+of '1' in /sys/kernel/livepatch/<name>/transition. Once all tasks have
+been patched, the 'transition' value changes to '0'. For more
+information about this process, see the "Consistency model" section.
+
+If an original function is patched for the first time, a function
+specific struct klp_ops is created and an universal ftrace handler is
+registered.
Functions might be patched multiple times. The ftrace handler is registered
only once for the given function. Further patches just add an entry to the
@@ -261,6 +351,12 @@ by writing '0' to /sys/kernel/livepatch/<name>/enabled. At this stage
either the code from the previously enabled patch or even the original
code gets used.
+When a patch is disabled, livepatch enters into a transition state where
+tasks are converging to the unpatched state. This is indicated by a
+value of '1' in /sys/kernel/livepatch/<name>/transition. Once all tasks
+have been unpatched, the 'transition' value changes to '0'. For more
+information about this process, see the "Consistency model" section.
+
Here all the functions (struct klp_func) associated with the to-be-disabled
patch are removed from the corresponding struct klp_ops. The ftrace handler
is unregistered and the struct klp_ops is freed when the func_stack list
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index f2cb8d4..12199ef 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -14,6 +14,7 @@
#include <linux/rbtree.h>
#include <net/net_namespace.h>
#include <linux/sched/rt.h>
+#include <linux/livepatch.h>
#ifdef CONFIG_SMP
# define INIT_PUSHABLE_TASKS(tsk) \
@@ -183,6 +184,13 @@ extern struct task_group root_task_group;
# define INIT_KASAN(tsk)
#endif
+#ifdef CONFIG_LIVEPATCH
+#define INIT_LIVEPATCH(tsk) \
+ .patch_state = KLP_UNDEFINED,
+#else
+#define INIT_LIVEPATCH(tsk)
+#endif
+
/*
* INIT_TASK is used to set up the first task table, touch at
* your own risk!. Base=0, limit=0x1fffff (=2MB)
@@ -260,6 +268,7 @@ extern struct task_group root_task_group;
INIT_VTIME(tsk) \
INIT_NUMA_BALANCING(tsk) \
INIT_KASAN(tsk) \
+ INIT_LIVEPATCH(tsk) \
}
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index c38c694..6ec50ff 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -28,18 +28,40 @@
#include <asm/livepatch.h>
+/* task patch states */
+#define KLP_UNDEFINED -1
+#define KLP_UNPATCHED 0
+#define KLP_PATCHED 1
+
/**
* struct klp_func - function structure for live patching
* @old_name: name of the function to be patched
* @new_func: pointer to the patched function code
* @old_sympos: a hint indicating which symbol position the old function
* can be found (optional)
+ * @immediate: patch the func immediately, bypassing backtrace safety checks
* @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
+ *
+ * The patched and transition variables define the func's patching state. When
+ * patching, a func is always in one of the following states:
+ *
+ * patched=0 transition=0: unpatched
+ * patched=0 transition=1: unpatched, temporary starting state
+ * patched=1 transition=1: patched, may be visible to some tasks
+ * patched=1 transition=0: patched, visible to all tasks
+ *
+ * And when unpatching, it goes in the reverse order:
+ *
+ * patched=1 transition=0: patched, visible to all tasks
+ * patched=1 transition=1: patched, may be visible to some tasks
+ * patched=0 transition=1: unpatched, temporary ending state
+ * patched=0 transition=0: unpatched
*/
struct klp_func {
/* external */
@@ -53,6 +75,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 +83,7 @@ struct klp_func {
struct list_head stack_node;
unsigned long old_size, new_size;
bool patched;
+ bool transition;
};
/**
@@ -86,6 +110,7 @@ struct klp_object {
* struct klp_patch - patch structure for live patching
* @mod: reference to the live patch module
* @objs: object entries for kernel objects to be patched
+ * @immediate: patch all funcs immediately, bypassing safety mechanisms
* @list: list node for global list of registered patches
* @kobj: kobject for sysfs resources
* @enabled: the patch is enabled (but operation may be incomplete)
@@ -94,6 +119,7 @@ struct klp_patch {
/* external */
struct module *mod;
struct klp_object *objs;
+ bool immediate;
/* internal */
struct list_head list;
@@ -116,15 +142,21 @@ int klp_disable_patch(struct klp_patch *);
int klp_module_coming(struct module *mod);
void klp_module_going(struct module *mod);
-static inline bool klp_patch_pending(struct task_struct *task) { return false; }
+void klp_copy_process(struct task_struct *child);
void klp_patch_task(struct task_struct *task);
+static inline bool klp_patch_pending(struct task_struct *task)
+{
+ return test_tsk_thread_flag(task, TIF_PATCH_PENDING);
+}
+
#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 bool klp_patch_pending(struct task_struct *task) { return false; }
static inline void klp_patch_task(struct task_struct *task) {}
+static inline void klp_copy_process(struct task_struct *child) {}
#endif /* CONFIG_LIVEPATCH */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index fb364a0..7fc8b49 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1860,6 +1860,9 @@ struct task_struct {
#ifdef CONFIG_MMU
struct task_struct *oom_reaper_list;
#endif
+#ifdef CONFIG_LIVEPATCH
+ int patch_state;
+#endif
/* CPU-specific state of this task */
struct thread_struct thread;
/*
diff --git a/kernel/fork.c b/kernel/fork.c
index d2fe04a..a12e3b0 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>
@@ -1586,6 +1587,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
p->parent_exec_id = current->self_exec_id;
}
+ klp_copy_process(p);
+
spin_lock(¤t->sighand->siglock);
/*
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 aa3dbdf..0be352f 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -31,12 +31,15 @@
#include <linux/moduleloader.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_patch_task()
*/
static DEFINE_MUTEX(klp_mutex);
@@ -44,8 +47,28 @@ static LIST_HEAD(klp_patches);
static struct kobject *klp_root_kobj;
-/* TODO: temporary stub */
-void klp_patch_task(struct task_struct *task) {}
+static void klp_work_fn(struct work_struct *work);
+static DECLARE_DELAYED_WORK(klp_work, klp_work_fn);
+
+static void klp_schedule_work(void)
+{
+ 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)
{
@@ -85,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;
@@ -283,19 +305,18 @@ static int klp_write_object_relocations(struct module *pmod,
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_UNPATCHED);
+ klp_start_transition();
+ if (!klp_try_complete_transition())
+ klp_schedule_work();
patch->enabled = false;
@@ -339,6 +360,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;
@@ -350,24 +374,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_PATCHED);
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();
+
+ if (!klp_try_complete_transition())
+ klp_schedule_work();
+
patch->enabled = true;
return 0;
-
-unregister:
- WARN_ON(__klp_disable_patch(patch));
- return ret;
}
/**
@@ -404,6 +436,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>
*/
@@ -432,7 +465,9 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
goto err;
}
- if (val) {
+ if (patch == klp_transition_patch) {
+ klp_reverse_transition();
+ } else if (val) {
ret = __klp_enable_patch(patch);
if (ret)
goto err;
@@ -460,9 +495,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",
+ patch == klp_transition_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
};
@@ -549,6 +596,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
@@ -781,7 +829,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 && patch != klp_transition_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 782fbb5..b3b8639 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->patch_state == KLP_UNPATCHED) {
+ /*
+ * 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();
@@ -211,3 +243,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..92819bb
--- /dev/null
+++ b/kernel/livepatch/transition.c
@@ -0,0 +1,474 @@
+/*
+ * transition.c - Kernel Live Patching transition functions
+ *
+ * Copyright (C) 2015 Josh Poimboeuf <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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;
+
+static int klp_target_state;
+
+/* called from copy_process() during fork */
+void klp_copy_process(struct task_struct *child)
+{
+ child->patch_state = current->patch_state;
+
+ /* TIF_PATCH_PENDING gets copied in setup_thread_stack() */
+}
+
+/*
+ * klp_patch_task() - change the patched state of a task
+ * @task: The task to change
+ *
+ * Switches the patched state of the task to the set of functions in the target
+ * patch state.
+ */
+void klp_patch_task(struct task_struct *task)
+{
+ clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
+
+ /*
+ * The corresponding write barriers are in klp_init_transition() and
+ * klp_reverse_transition(). See the comments there for an explanation.
+ */
+ smp_rmb();
+
+ task->patch_state = klp_target_state;
+}
+
+/*
+ * Initialize the global target patch state and all tasks to the initial patch
+ * state, and initialize all function transition states to true in preparation
+ * for patching or unpatching.
+ */
+void klp_init_transition(struct klp_patch *patch, int state)
+{
+ struct task_struct *g, *task;
+ unsigned int cpu;
+ struct klp_object *obj;
+ struct klp_func *func;
+ int initial_state = !state;
+
+ klp_transition_patch = patch;
+
+ /*
+ * If the patch can be applied or reverted immediately, skip the
+ * per-task transitions.
+ */
+ if (patch->immediate)
+ return;
+
+ /*
+ * Initialize all tasks to the initial patch state to prepare them for
+ * switching to the target state.
+ */
+ read_lock(&tasklist_lock);
+ for_each_process_thread(g, task)
+ task->patch_state = initial_state;
+ read_unlock(&tasklist_lock);
+
+ /*
+ * Ditto for the idle "swapper" tasks.
+ */
+ get_online_cpus();
+ for_each_online_cpu(cpu)
+ idle_task(cpu)->patch_state = initial_state;
+ put_online_cpus();
+
+ /*
+ * Ensure klp_ftrace_handler() sees the task->patch_state updates
+ * before the func->transition updates. Otherwise it could read an
+ * out-of-date task state 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;
+
+ /*
+ * Set the global target patch state which tasks will switch to. This
+ * has no effect until the TIF_PATCH_PENDING flags get set later.
+ */
+ klp_target_state = state;
+
+ /*
+ * 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 patched state.
+ *
+ * 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
+ * unpatched state. Otherwise the handler can miss a task patch state
+ * change which would result in it wrongly picking the new function.
+ *
+ * This barrier also ensures that if another CPU goes through the
+ * syscall barrier, sees the TIF_PATCH_PENDING writes in
+ * klp_start_transition(), and calls klp_patch_task(), it also sees the
+ * above write to the target state. Otherwise it can put the task in
+ * the wrong universe.
+ */
+ smp_wmb();
+}
+
+/*
+ * Start the transition to the specified target patch state so tasks can begin
+ * switching to it.
+ */
+void klp_start_transition(void)
+{
+ struct task_struct *g, *task;
+ unsigned int cpu;
+
+ pr_notice("'%s': %s...\n", klp_transition_patch->mod->name,
+ klp_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
+ /*
+ * If the patch can be applied or reverted immediately, skip the
+ * per-task transitions.
+ */
+ if (klp_transition_patch->immediate)
+ return;
+
+ /*
+ * Mark all normal tasks as needing a patch state update. As they pass
+ * through the syscall barrier they'll switch over to the target state
+ * (unless we switch them in klp_try_complete_transition() first).
+ */
+ read_lock(&tasklist_lock);
+ for_each_process_thread(g, task)
+ set_tsk_thread_flag(task, TIF_PATCH_PENDING);
+ read_unlock(&tasklist_lock);
+
+ /*
+ * Ditto for the idle "swapper" tasks, though they never cross the
+ * syscall barrier. Instead they switch over in cpu_idle_loop().
+ */
+ get_online_cpus();
+ for_each_online_cpu(cpu)
+ set_tsk_thread_flag(idle_task(cpu), TIF_PATCH_PENDING);
+ put_online_cpus();
+}
+
+/*
+ * The transition to the target patch state is complete. Clean up the data
+ * structures.
+ */
+void klp_complete_transition(void)
+{
+ struct klp_object *obj;
+ struct klp_func *func;
+ struct task_struct *g, *task;
+ unsigned int cpu;
+
+ if (klp_transition_patch->immediate)
+ goto done;
+
+ klp_for_each_object(klp_transition_patch, obj)
+ klp_for_each_func(obj, func)
+ func->transition = false;
+
+ read_lock(&tasklist_lock);
+ for_each_process_thread(g, task) {
+ clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
+ task->patch_state = KLP_UNDEFINED;
+ }
+ read_unlock(&tasklist_lock);
+
+ get_online_cpus();
+ for_each_online_cpu(cpu) {
+ task = idle_task(cpu);
+ clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
+ task->patch_state = KLP_UNDEFINED;
+ }
+ put_online_cpus();
+
+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->nr_entries; i++) {
+ address = trace->entries[i];
+
+ if (klp_target_state == KLP_UNPATCHED) {
+ /*
+ * 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 target patch state
+ * 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 target patch state. If it's currently
+ * running, or it's sleeping on a to-be-patched or to-be-unpatched function, or
+ * if the stack is unreliable, return false.
+ */
+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->patch_state == klp_target_state)
+ 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 target patch
+ * state.
+ */
+ 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;
+
+ success = true;
+
+ clear_tsk_thread_flag(task, TIF_PATCH_PENDING);
+ task->patch_state = klp_target_state;
+
+done:
+ task_rq_unlock(rq, task, &flags);
+ return success;
+}
+
+/*
+ * Try to switch all remaining tasks to the target patch state by walking the
+ * stacks of sleeping tasks and looking for any to-be-patched or
+ * to-be-unpatched functions. If such functions are found, the task can't be
+ * switched yet.
+ *
+ * If any tasks are still stuck in the initial patch state, schedule a retry.
+ */
+bool klp_try_complete_transition(void)
+{
+ unsigned int cpu;
+ struct task_struct *g, *task;
+ bool complete = true;
+
+ /*
+ * If the patch can be applied or reverted immediately, skip the
+ * per-task transitions.
+ */
+ if (klp_transition_patch->immediate)
+ goto success;
+
+ /*
+ * Try to switch the tasks to the target patch state by walking their
+ * stacks and looking for any to-be-patched or to-be-unpatched
+ * functions. If such functions are found on a stack, or if the stack
+ * is deemed unreliable, the task can't be switched yet.
+ *
+ * Usually this will transition most (or all) of the tasks on a system
+ * unless the patch includes changes to a very common function.
+ */
+ read_lock(&tasklist_lock);
+ for_each_process_thread(g, task)
+ if (!klp_try_switch_task(task))
+ complete = false;
+ read_unlock(&tasklist_lock);
+
+ /*
+ * Ditto for the idle "swapper" tasks.
+ */
+ get_online_cpus();
+ for_each_online_cpu(cpu)
+ if (!klp_try_switch_task(idle_task(cpu)))
+ complete = false;
+ put_online_cpus();
+
+ /*
+ * Some tasks weren't able to be switched over. Try again later and/or
+ * wait for other methods like syscall barrier switching.
+ */
+ if (!complete)
+ return false;
+
+success:
+ /*
+ * When unpatching, all tasks have transitioned to KLP_UNPATCHED so we
+ * can now remove the new functions from the func_stack.
+ */
+ if (klp_target_state == KLP_UNPATCHED) {
+ 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_target_state == KLP_PATCHED ? "patching" : "unpatching");
+
+ /* we're done, now cleanup the data structures */
+ klp_complete_transition();
+
+ return true;
+}
+
+/*
+ * This function can be called in the middle of an existing transition to
+ * reverse the direction of the target patch state. This can be done to
+ * effectively cancel an existing enable or disable operation if there are any
+ * tasks which are stuck in the initial patch state.
+ */
+void klp_reverse_transition(void)
+{
+ struct klp_patch *patch = klp_transition_patch;
+
+ klp_target_state = !klp_target_state;
+
+ /*
+ * Ensure that if another CPU goes through the syscall barrier, sees
+ * the TIF_PATCH_PENDING writes in klp_start_transition(), and calls
+ * klp_patch_task(), it also sees the above write to the target state.
+ * Otherwise it can put the task in the wrong universe.
+ */
+ smp_wmb();
+
+ klp_start_transition();
+ klp_try_complete_transition();
+
+ patch->enabled = !patch->enabled;
+}
+
diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
new file mode 100644
index 0000000..5191b96
--- /dev/null
+++ b/kernel/livepatch/transition.h
@@ -0,0 +1,14 @@
+#ifndef _LIVEPATCH_TRANSITION_H
+#define _LIVEPATCH_TRANSITION_H
+
+#include <linux/livepatch.h>
+
+extern struct klp_patch *klp_transition_patch;
+
+void klp_init_transition(struct klp_patch *patch, int state);
+void klp_start_transition(void);
+void klp_reverse_transition(void);
+bool klp_try_complete_transition(void);
+void klp_complete_transition(void);
+
+#endif /* _LIVEPATCH_TRANSITION_H */
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index bd12c6c..60d633f 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -9,6 +9,7 @@
#include <linux/mm.h>
#include <linux/stackprotector.h>
#include <linux/suspend.h>
+#include <linux/livepatch.h>
#include <asm/tlb.h>
@@ -266,6 +267,9 @@ static void cpu_idle_loop(void)
sched_ttwu_pending();
schedule_preempt_disabled();
+
+ if (unlikely(klp_patch_pending(current)))
+ klp_patch_task(current);
}
}
--
2.4.11
Move functions related to the actual patching of functions and objects
into a new patch.c file.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
kernel/livepatch/Makefile | 2 +-
kernel/livepatch/core.c | 202 +------------------------------------------
kernel/livepatch/patch.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++
kernel/livepatch/patch.h | 32 +++++++
4 files changed, 247 insertions(+), 202 deletions(-)
create mode 100644 kernel/livepatch/patch.c
create mode 100644 kernel/livepatch/patch.h
diff --git a/kernel/livepatch/Makefile b/kernel/livepatch/Makefile
index e8780c0..e136dad 100644
--- a/kernel/livepatch/Makefile
+++ b/kernel/livepatch/Makefile
@@ -1,3 +1,3 @@
obj-$(CONFIG_LIVEPATCH) += livepatch.o
-livepatch-objs := core.o
+livepatch-objs := core.o patch.o
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 2ad7892..f28504d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -24,32 +24,13 @@
#include <linux/kernel.h>
#include <linux/mutex.h>
#include <linux/slab.h>
-#include <linux/ftrace.h>
#include <linux/list.h>
#include <linux/kallsyms.h>
#include <linux/livepatch.h>
#include <linux/elf.h>
#include <linux/moduleloader.h>
#include <asm/cacheflush.h>
-
-/**
- * struct klp_ops - structure for tracking registered ftrace ops structs
- *
- * A single ftrace_ops is shared between all enabled replacement functions
- * (klp_func structs) which have the same old_addr. This allows the switch
- * between function versions to happen instantaneously by updating the klp_ops
- * struct's func_stack list. The winner is the klp_func at the top of the
- * func_stack (front of the list).
- *
- * @node: node for the global klp_ops list
- * @func_stack: list head for the stack of klp_func's (active func is on top)
- * @fops: registered ftrace ops struct
- */
-struct klp_ops {
- struct list_head node;
- struct list_head func_stack;
- struct ftrace_ops fops;
-};
+#include "patch.h"
/*
* The klp_mutex protects the global lists and state transitions of any
@@ -60,28 +41,12 @@ struct klp_ops {
static DEFINE_MUTEX(klp_mutex);
static LIST_HEAD(klp_patches);
-static LIST_HEAD(klp_ops);
static struct kobject *klp_root_kobj;
/* TODO: temporary stub */
void klp_patch_task(struct task_struct *task) {}
-static struct klp_ops *klp_find_ops(unsigned long old_addr)
-{
- struct klp_ops *ops;
- struct klp_func *func;
-
- list_for_each_entry(ops, &klp_ops, node) {
- func = list_first_entry(&ops->func_stack, struct klp_func,
- stack_node);
- if (func->old_addr == old_addr)
- return ops;
- }
-
- return NULL;
-}
-
static bool klp_is_module(struct klp_object *obj)
{
return obj->name;
@@ -316,171 +281,6 @@ static int klp_write_object_relocations(struct module *pmod,
return ret;
}
-static void notrace klp_ftrace_handler(unsigned long ip,
- unsigned long parent_ip,
- struct ftrace_ops *fops,
- struct pt_regs *regs)
-{
- struct klp_ops *ops;
- struct klp_func *func;
-
- 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))
- goto unlock;
-
- klp_arch_set_pc(regs, (unsigned long)func->new_func);
-unlock:
- rcu_read_unlock();
-}
-
-/*
- * Convert a function address into the appropriate ftrace location.
- *
- * Usually this is just the address of the function, but on some architectures
- * it's more complicated so allow them to provide a custom behaviour.
- */
-#ifndef klp_get_ftrace_location
-static unsigned long klp_get_ftrace_location(unsigned long faddr)
-{
- return faddr;
-}
-#endif
-
-static void klp_unpatch_func(struct klp_func *func)
-{
- struct klp_ops *ops;
-
- if (WARN_ON(!func->patched))
- return;
- if (WARN_ON(!func->old_addr))
- return;
-
- ops = klp_find_ops(func->old_addr);
- if (WARN_ON(!ops))
- return;
-
- if (list_is_singular(&ops->func_stack)) {
- unsigned long ftrace_loc;
-
- ftrace_loc = klp_get_ftrace_location(func->old_addr);
- if (WARN_ON(!ftrace_loc))
- return;
-
- WARN_ON(unregister_ftrace_function(&ops->fops));
- WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
-
- list_del_rcu(&func->stack_node);
- list_del(&ops->node);
- kfree(ops);
- } else {
- list_del_rcu(&func->stack_node);
- }
-
- func->patched = false;
-}
-
-static int klp_patch_func(struct klp_func *func)
-{
- struct klp_ops *ops;
- int ret;
-
- if (WARN_ON(!func->old_addr))
- return -EINVAL;
-
- if (WARN_ON(func->patched))
- return -EINVAL;
-
- ops = klp_find_ops(func->old_addr);
- if (!ops) {
- unsigned long ftrace_loc;
-
- ftrace_loc = klp_get_ftrace_location(func->old_addr);
- if (!ftrace_loc) {
- pr_err("failed to find location for function '%s'\n",
- func->old_name);
- return -EINVAL;
- }
-
- ops = kzalloc(sizeof(*ops), GFP_KERNEL);
- if (!ops)
- return -ENOMEM;
-
- ops->fops.func = klp_ftrace_handler;
- ops->fops.flags = FTRACE_OPS_FL_SAVE_REGS |
- FTRACE_OPS_FL_DYNAMIC |
- FTRACE_OPS_FL_IPMODIFY;
-
- list_add(&ops->node, &klp_ops);
-
- INIT_LIST_HEAD(&ops->func_stack);
- list_add_rcu(&func->stack_node, &ops->func_stack);
-
- ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0);
- if (ret) {
- pr_err("failed to set ftrace filter for function '%s' (%d)\n",
- func->old_name, ret);
- goto err;
- }
-
- ret = register_ftrace_function(&ops->fops);
- if (ret) {
- pr_err("failed to register ftrace handler for function '%s' (%d)\n",
- func->old_name, ret);
- ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0);
- goto err;
- }
-
-
- } else {
- list_add_rcu(&func->stack_node, &ops->func_stack);
- }
-
- func->patched = true;
-
- return 0;
-
-err:
- list_del_rcu(&func->stack_node);
- list_del(&ops->node);
- kfree(ops);
- return ret;
-}
-
-static void klp_unpatch_object(struct klp_object *obj)
-{
- struct klp_func *func;
-
- klp_for_each_func(obj, func)
- if (func->patched)
- klp_unpatch_func(func);
-
- obj->patched = false;
-}
-
-static int klp_patch_object(struct klp_object *obj)
-{
- struct klp_func *func;
- int ret;
-
- if (WARN_ON(obj->patched))
- return -EINVAL;
-
- klp_for_each_func(obj, func) {
- ret = klp_patch_func(func);
- if (ret) {
- klp_unpatch_object(obj);
- return ret;
- }
- }
- obj->patched = true;
-
- return 0;
-}
-
static int __klp_disable_patch(struct klp_patch *patch)
{
struct klp_object *obj;
diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
new file mode 100644
index 0000000..782fbb5
--- /dev/null
+++ b/kernel/livepatch/patch.c
@@ -0,0 +1,213 @@
+/*
+ * patch.c - livepatch patching functions
+ *
+ * Copyright (C) 2014 Seth Jennings <[email protected]>
+ * Copyright (C) 2014 SUSE
+ * Copyright (C) 2015 Josh Poimboeuf <[email protected]
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/livepatch.h>
+#include <linux/list.h>
+#include <linux/ftrace.h>
+#include <linux/rculist.h>
+#include <linux/slab.h>
+#include <linux/bug.h>
+#include <linux/printk.h>
+#include "patch.h"
+
+static LIST_HEAD(klp_ops);
+
+struct klp_ops *klp_find_ops(unsigned long old_addr)
+{
+ struct klp_ops *ops;
+ struct klp_func *func;
+
+ list_for_each_entry(ops, &klp_ops, node) {
+ func = list_first_entry(&ops->func_stack, struct klp_func,
+ stack_node);
+ if (func->old_addr == old_addr)
+ return ops;
+ }
+
+ return NULL;
+}
+
+static void notrace klp_ftrace_handler(unsigned long ip,
+ unsigned long parent_ip,
+ struct ftrace_ops *fops,
+ struct pt_regs *regs)
+{
+ struct klp_ops *ops;
+ struct klp_func *func;
+
+ 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))
+ goto unlock;
+
+ klp_arch_set_pc(regs, (unsigned long)func->new_func);
+unlock:
+ rcu_read_unlock();
+}
+
+/*
+ * Convert a function address into the appropriate ftrace location.
+ *
+ * Usually this is just the address of the function, but on some architectures
+ * it's more complicated so allow them to provide a custom behaviour.
+ */
+#ifndef klp_get_ftrace_location
+static unsigned long klp_get_ftrace_location(unsigned long faddr)
+{
+ return faddr;
+}
+#endif
+
+static void klp_unpatch_func(struct klp_func *func)
+{
+ struct klp_ops *ops;
+
+ if (WARN_ON(!func->patched))
+ return;
+ if (WARN_ON(!func->old_addr))
+ return;
+
+ ops = klp_find_ops(func->old_addr);
+ if (WARN_ON(!ops))
+ return;
+
+ if (list_is_singular(&ops->func_stack)) {
+ unsigned long ftrace_loc;
+
+ ftrace_loc = klp_get_ftrace_location(func->old_addr);
+ if (WARN_ON(!ftrace_loc))
+ return;
+
+ WARN_ON(unregister_ftrace_function(&ops->fops));
+ WARN_ON(ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0));
+
+ list_del_rcu(&func->stack_node);
+ list_del(&ops->node);
+ kfree(ops);
+ } else {
+ list_del_rcu(&func->stack_node);
+ }
+
+ func->patched = false;
+}
+
+static int klp_patch_func(struct klp_func *func)
+{
+ struct klp_ops *ops;
+ int ret;
+
+ if (WARN_ON(!func->old_addr))
+ return -EINVAL;
+
+ if (WARN_ON(func->patched))
+ return -EINVAL;
+
+ ops = klp_find_ops(func->old_addr);
+ if (!ops) {
+ unsigned long ftrace_loc;
+
+ ftrace_loc = klp_get_ftrace_location(func->old_addr);
+ if (!ftrace_loc) {
+ pr_err("failed to find location for function '%s'\n",
+ func->old_name);
+ return -EINVAL;
+ }
+
+ ops = kzalloc(sizeof(*ops), GFP_KERNEL);
+ if (!ops)
+ return -ENOMEM;
+
+ ops->fops.func = klp_ftrace_handler;
+ ops->fops.flags = FTRACE_OPS_FL_SAVE_REGS |
+ FTRACE_OPS_FL_DYNAMIC |
+ FTRACE_OPS_FL_IPMODIFY;
+
+ list_add(&ops->node, &klp_ops);
+
+ INIT_LIST_HEAD(&ops->func_stack);
+ list_add_rcu(&func->stack_node, &ops->func_stack);
+
+ ret = ftrace_set_filter_ip(&ops->fops, ftrace_loc, 0, 0);
+ if (ret) {
+ pr_err("failed to set ftrace filter for function '%s' (%d)\n",
+ func->old_name, ret);
+ goto err;
+ }
+
+ ret = register_ftrace_function(&ops->fops);
+ if (ret) {
+ pr_err("failed to register ftrace handler for function '%s' (%d)\n",
+ func->old_name, ret);
+ ftrace_set_filter_ip(&ops->fops, ftrace_loc, 1, 0);
+ goto err;
+ }
+
+
+ } else {
+ list_add_rcu(&func->stack_node, &ops->func_stack);
+ }
+
+ func->patched = true;
+
+ return 0;
+
+err:
+ list_del_rcu(&func->stack_node);
+ list_del(&ops->node);
+ kfree(ops);
+ return ret;
+}
+
+void klp_unpatch_object(struct klp_object *obj)
+{
+ struct klp_func *func;
+
+ klp_for_each_func(obj, func)
+ if (func->patched)
+ klp_unpatch_func(func);
+
+ obj->patched = false;
+}
+
+int klp_patch_object(struct klp_object *obj)
+{
+ struct klp_func *func;
+ int ret;
+
+ if (WARN_ON(obj->patched))
+ return -EINVAL;
+
+ klp_for_each_func(obj, func) {
+ ret = klp_patch_func(func);
+ if (ret) {
+ klp_unpatch_object(obj);
+ return ret;
+ }
+ }
+ obj->patched = true;
+
+ return 0;
+}
diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
new file mode 100644
index 0000000..2d0cce0
--- /dev/null
+++ b/kernel/livepatch/patch.h
@@ -0,0 +1,32 @@
+#ifndef _LIVEPATCH_PATCH_H
+#define _LIVEPATCH_PATCH_H
+
+#include <linux/livepatch.h>
+#include <linux/list.h>
+#include <linux/ftrace.h>
+
+/**
+ * struct klp_ops - structure for tracking registered ftrace ops structs
+ *
+ * A single ftrace_ops is shared between all enabled replacement functions
+ * (klp_func structs) which have the same old_addr. This allows the switch
+ * between function versions to happen instantaneously by updating the klp_ops
+ * struct's func_stack list. The winner is the klp_func at the top of the
+ * func_stack (front of the list).
+ *
+ * @node: node for the global klp_ops list
+ * @func_stack: list head for the stack of klp_func's (active func is on top)
+ * @fops: registered ftrace ops struct
+ */
+struct klp_ops {
+ struct list_head node;
+ struct list_head func_stack;
+ struct ftrace_ops fops;
+};
+
+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);
+
+#endif /* _LIVEPATCH_PATCH_H */
--
2.4.11
From: Jiri Slaby <[email protected]>
Group the TIF thread flag bits by their inclusion in the _TIF_WORK and
_TIF_TRACE macros.
Signed-off-by: Jiri Slaby <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/s390/include/asm/thread_info.h | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
index 2fffc2c..8642c1d 100644
--- a/arch/s390/include/asm/thread_info.h
+++ b/arch/s390/include/asm/thread_info.h
@@ -70,14 +70,12 @@ void arch_release_task_struct(struct task_struct *tsk);
/*
* thread information flags bit numbers
*/
+/* _TIF_WORK bits */
#define TIF_NOTIFY_RESUME 0 /* callback before returning to user */
#define TIF_SIGPENDING 1 /* signal pending */
#define TIF_NEED_RESCHED 2 /* rescheduling necessary */
-#define TIF_SYSCALL_TRACE 3 /* syscall trace active */
-#define TIF_SYSCALL_AUDIT 4 /* syscall auditing active */
-#define TIF_SECCOMP 5 /* secure computing */
-#define TIF_SYSCALL_TRACEPOINT 6 /* syscall tracepoint instrumentation */
-#define TIF_UPROBE 7 /* breakpointed or single-stepping */
+#define TIF_UPROBE 3 /* breakpointed or single-stepping */
+
#define TIF_31BIT 16 /* 32bit process */
#define TIF_MEMDIE 17 /* is terminating due to OOM killer */
#define TIF_RESTORE_SIGMASK 18 /* restore signal mask in do_signal() */
@@ -85,15 +83,23 @@ void arch_release_task_struct(struct task_struct *tsk);
#define TIF_BLOCK_STEP 20 /* This task is block stepped */
#define TIF_UPROBE_SINGLESTEP 21 /* This task is uprobe single stepped */
+/* _TIF_TRACE bits */
+#define TIF_SYSCALL_TRACE 24 /* syscall trace active */
+#define TIF_SYSCALL_AUDIT 25 /* syscall auditing active */
+#define TIF_SECCOMP 26 /* secure computing */
+#define TIF_SYSCALL_TRACEPOINT 27 /* syscall tracepoint instrumentation */
+
#define _TIF_NOTIFY_RESUME _BITUL(TIF_NOTIFY_RESUME)
#define _TIF_SIGPENDING _BITUL(TIF_SIGPENDING)
#define _TIF_NEED_RESCHED _BITUL(TIF_NEED_RESCHED)
+#define _TIF_UPROBE _BITUL(TIF_UPROBE)
+
+#define _TIF_31BIT _BITUL(TIF_31BIT)
+#define _TIF_SINGLE_STEP _BITUL(TIF_SINGLE_STEP)
+
#define _TIF_SYSCALL_TRACE _BITUL(TIF_SYSCALL_TRACE)
#define _TIF_SYSCALL_AUDIT _BITUL(TIF_SYSCALL_AUDIT)
#define _TIF_SECCOMP _BITUL(TIF_SECCOMP)
#define _TIF_SYSCALL_TRACEPOINT _BITUL(TIF_SYSCALL_TRACEPOINT)
-#define _TIF_UPROBE _BITUL(TIF_UPROBE)
-#define _TIF_31BIT _BITUL(TIF_31BIT)
-#define _TIF_SINGLE_STEP _BITUL(TIF_SINGLE_STEP)
#endif /* _ASM_THREAD_INFO_H */
--
2.4.11
Add the TIF_PATCH_PENDING thread flag to enable the new livepatch
per-task consistency model for x86_64. The bit getting set indicates
the thread has a pending patch which needs to be applied when the thread
exits the kernel.
The bit is placed in the least-significant word of the thread_info flags
so that it gets automatically included in the _TIF_ALLWORK_MASK macro.
This results in exit_to_usermode_loop() and klp_patch_task() getting
called when the bit is set.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/entry/common.c | 9 ++++++---
arch/x86/include/asm/thread_info.h | 2 ++
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index ec138e5..0eaa1d9 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -21,6 +21,7 @@
#include <linux/context_tracking.h>
#include <linux/user-return-notifier.h>
#include <linux/uprobes.h>
+#include <linux/livepatch.h>
#include <asm/desc.h>
#include <asm/traps.h>
@@ -202,14 +203,13 @@ long syscall_trace_enter(struct pt_regs *regs)
#define EXIT_TO_USERMODE_LOOP_FLAGS \
(_TIF_SIGPENDING | _TIF_NOTIFY_RESUME | _TIF_UPROBE | \
- _TIF_NEED_RESCHED | _TIF_USER_RETURN_NOTIFY)
+ _TIF_NEED_RESCHED | _TIF_USER_RETURN_NOTIFY | _TIF_PATCH_PENDING)
static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
{
/*
* In order to return to user mode, we need to have IRQs off with
- * none of _TIF_SIGPENDING, _TIF_NOTIFY_RESUME, _TIF_USER_RETURN_NOTIFY,
- * _TIF_UPROBE, or _TIF_NEED_RESCHED set. Several of these flags
+ * none of EXIT_TO_USERMODE_LOOP_FLAGS set. Several of these flags
* can be set at any time on preemptable kernels if we have IRQs on,
* so we need to loop. Disabling preemption wouldn't help: doing the
* work to clear some of the flags can sleep.
@@ -236,6 +236,9 @@ static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags)
if (cached_flags & _TIF_USER_RETURN_NOTIFY)
fire_user_return_notifiers();
+ if (cached_flags & _TIF_PATCH_PENDING)
+ klp_patch_task(current);
+
/* Disable IRQs and retry */
local_irq_disable();
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 30c133a..4e4f50e 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -97,6 +97,7 @@ struct thread_info {
#define TIF_SECCOMP 8 /* secure computing */
#define TIF_USER_RETURN_NOTIFY 11 /* notify kernel of userspace return */
#define TIF_UPROBE 12 /* breakpointed or singlestepping */
+#define TIF_PATCH_PENDING 13 /* pending live patching update */
#define TIF_NOTSC 16 /* TSC is not accessible in userland */
#define TIF_IA32 17 /* IA32 compatibility process */
#define TIF_FORK 18 /* ret_from_fork */
@@ -121,6 +122,7 @@ struct thread_info {
#define _TIF_SECCOMP (1 << TIF_SECCOMP)
#define _TIF_USER_RETURN_NOTIFY (1 << TIF_USER_RETURN_NOTIFY)
#define _TIF_UPROBE (1 << TIF_UPROBE)
+#define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING)
#define _TIF_NOTSC (1 << TIF_NOTSC)
#define _TIF_IA32 (1 << TIF_IA32)
#define _TIF_FORK (1 << TIF_FORK)
--
2.4.11
For live patching and possibly other use cases, a stack trace is only
useful if it can be assured that it's completely reliable. Add a new
save_stack_trace_tsk_reliable() function to achieve that.
Scenarios which indicate that a stack trace may be unreliable:
- running tasks
- interrupt stacks
- preemption
- corrupted stack data
- the stack grows the wrong way
- the stack walk doesn't reach the bottom
- 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 <[email protected]>
---
arch/Kconfig | 6 ++++
arch/x86/Kconfig | 1 +
arch/x86/kernel/dumpstack.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
arch/x86/kernel/stacktrace.c | 24 ++++++++++++++
include/linux/kernel.h | 1 +
include/linux/stacktrace.h | 20 +++++++++---
kernel/extable.c | 2 +-
kernel/stacktrace.c | 4 +--
lib/Kconfig.debug | 6 ++++
9 files changed, 134 insertions(+), 7 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 8f84fd2..ec4d480 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -598,6 +598,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.
+
#
# ABI hall of shame
#
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0b128b4..78c4e00 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -140,6 +140,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 13d240c..70d0013 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -145,6 +145,83 @@ int print_context_stack_bp(struct thread_info *tinfo,
}
EXPORT_SYMBOL_GPL(print_context_stack_bp);
+#ifdef CONFIG_RELIABLE_STACKTRACE
+/*
+ * Only succeeds if the stack trace is deemed reliable. This relies on the
+ * fact that frame pointers are reliable thanks to CONFIG_STACK_VALIDATION.
+ *
+ * The caller must ensure that the task is either sleeping or is the current
+ * task.
+ */
+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 = NULL;
+ unsigned long *ret_addr = &frame->return_address;
+
+ /*
+ * If the kernel was preempted by an IRQ, we can't trust the stack
+ * because the preempted function might not have gotten the chance to
+ * save the frame pointer on the stack before it was interrupted.
+ */
+ if (tinfo->task->flags & PF_PREEMPT_IRQ)
+ return -EINVAL;
+
+ /*
+ * A freshly forked task has an empty stack trace. We can consider
+ * that to be reliable.
+ */
+ if (test_ti_thread_flag(tinfo, TIF_FORK))
+ return 0;
+
+ while (valid_stack_ptr(tinfo, ret_addr, sizeof(*ret_addr), end)) {
+ unsigned long addr = *ret_addr;
+
+ /*
+ * Make sure the stack only grows down.
+ */
+ if (frame <= last_frame)
+ return -EINVAL;
+
+ /*
+ * Make sure the frame refers to a valid kernel function.
+ */
+ if (!core_kernel_text(addr) && !init_kernel_text(addr) &&
+ !is_module_text_address(addr))
+ return -EINVAL;
+
+ /*
+ * Save the kernel text address and make sure the entries array
+ * isn't full.
+ */
+ if (ops->address(data, addr, 1))
+ return -EINVAL;
+
+ /*
+ * If the function graph tracer is in effect, save the real
+ * function address.
+ */
+ print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
+
+ last_frame = frame;
+ frame = frame->next_frame;
+ ret_addr = &frame->return_address;
+ }
+
+ /*
+ * Make sure we reached the bottom of the stack.
+ */
+ if (last_frame + 1 != (void *)task_pt_regs(tinfo->task))
+ return -EINVAL;
+
+ *bp = (unsigned long)frame;
+ return 0;
+}
+#endif /* CONFIG_RELIABLE_STACKTRACE */
+
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..10882e4 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -148,3 +148,27 @@ void save_stack_trace_user(struct stack_trace *trace)
trace->entries[trace->nr_entries++] = ULONG_MAX;
}
+#ifdef CONFIG_RELIABLE_STACKTRACE
+
+static int save_stack_stack_reliable(void *data, char *name)
+{
+ return -EINVAL;
+}
+
+static const struct stacktrace_ops save_stack_ops_reliable = {
+ .stack = save_stack_stack_reliable,
+ .address = save_stack_address,
+ .walk_stack = print_context_stack_reliable,
+};
+
+/*
+ * 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)
+{
+ return dump_trace(tsk, NULL, NULL, 0, &save_stack_ops_reliable, trace);
+}
+
+#endif /* CONFIG_RELIABLE_STACKTRACE */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index cc73982..6be1e82 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -429,6 +429,7 @@ extern char *get_options(const char *str, int nints, int *ints);
extern unsigned long long memparse(const char *ptr, char **retptr);
extern bool parse_option_str(const char *str, const char *option);
+extern int init_kernel_text(unsigned long addr);
extern int core_kernel_text(unsigned long addr);
extern int core_kernel_data(unsigned long addr);
extern int __kernel_text_address(unsigned long addr);
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/extable.c b/kernel/extable.c
index e820cce..c085844 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -58,7 +58,7 @@ const struct exception_table_entry *search_exception_tables(unsigned long addr)
return e;
}
-static inline int init_kernel_text(unsigned long addr)
+int init_kernel_text(unsigned long addr)
{
if (addr >= (unsigned long)_sinittext &&
addr < (unsigned long)_einittext)
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 5d57177..189a2d7 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1164,6 +1164,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.11
A preempted function might not have had a chance to save the frame
pointer to the stack yet, which can result in its caller getting skipped
on a stack trace.
Add a flag to indicate when the task has been preempted so that stack
dump code can determine whether the stack trace is reliable.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
include/linux/sched.h | 1 +
kernel/fork.c | 2 +-
kernel/sched/core.c | 4 ++++
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 3d31572..fb364a0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2137,6 +2137,7 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,
#define PF_SWAPWRITE 0x00800000 /* Allowed to write to swap */
#define PF_NO_SETAFFINITY 0x04000000 /* Userland is not allowed to meddle with cpus_allowed */
#define PF_MCE_EARLY 0x08000000 /* Early kill for mce process policy */
+#define PF_PREEMPT_IRQ 0x10000000 /* Thread is preempted by an irq */
#define PF_MUTEX_TESTER 0x20000000 /* Thread belongs to the rt mutex tester */
#define PF_FREEZER_SKIP 0x40000000 /* Freezer should not count it as freezable */
#define PF_SUSPEND_TASK 0x80000000 /* this thread called freeze_processes and should not be frozen */
diff --git a/kernel/fork.c b/kernel/fork.c
index b73a539..d2fe04a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1373,7 +1373,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
goto bad_fork_cleanup_count;
delayacct_tsk_init(p); /* Must remain after dup_task_struct() */
- p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER);
+ p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_PREEMPT_IRQ);
p->flags |= PF_FORKNOEXEC;
INIT_LIST_HEAD(&p->children);
INIT_LIST_HEAD(&p->sibling);
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9d84d60..7594267 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3422,6 +3422,8 @@ asmlinkage __visible void __sched preempt_schedule_irq(void)
prev_state = exception_enter();
+ current->flags |= PF_PREEMPT_IRQ;
+
do {
preempt_disable();
local_irq_enable();
@@ -3430,6 +3432,8 @@ asmlinkage __visible void __sched preempt_schedule_irq(void)
sched_preempt_enable_no_resched();
} while (need_resched());
+ current->flags &= ~PF_PREEMPT_IRQ;
+
exception_exit(prev_state);
}
--
2.4.11
In preparation for being able to determine whether a given stack trace
is reliable, allow the stacktrace_ops functions to propagate errors to
dump_trace().
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/include/asm/stacktrace.h | 36 +++++++++++++++-----------
arch/x86/kernel/dumpstack.c | 31 +++++++++++------------
arch/x86/kernel/dumpstack_32.c | 22 ++++++++++------
arch/x86/kernel/dumpstack_64.c | 53 ++++++++++++++++++++++++++-------------
4 files changed, 87 insertions(+), 55 deletions(-)
diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
index 7c247e7..a64523f3 100644
--- a/arch/x86/include/asm/stacktrace.h
+++ b/arch/x86/include/asm/stacktrace.h
@@ -14,26 +14,32 @@ extern int kstack_depth_to_print;
struct thread_info;
struct stacktrace_ops;
-typedef unsigned long (*walk_stack_t)(struct thread_info *tinfo,
- unsigned long *stack,
- unsigned long bp,
- const struct stacktrace_ops *ops,
- void *data,
- unsigned long *end,
- int *graph);
-
-extern unsigned long
+typedef int (*walk_stack_t)(struct thread_info *tinfo,
+ unsigned long *stack,
+ unsigned long *bp,
+ const struct stacktrace_ops *ops,
+ void *data,
+ unsigned long *end,
+ int *graph);
+
+extern int
print_context_stack(struct thread_info *tinfo,
- unsigned long *stack, unsigned long bp,
+ unsigned long *stack, unsigned long *bp,
const struct stacktrace_ops *ops, void *data,
unsigned long *end, int *graph);
-extern unsigned long
+extern int
print_context_stack_bp(struct thread_info *tinfo,
- unsigned long *stack, unsigned long bp,
+ unsigned long *stack, unsigned long *bp,
const struct stacktrace_ops *ops, void *data,
unsigned long *end, int *graph);
+extern 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);
+
/* Generic stack tracer with callbacks */
struct stacktrace_ops {
@@ -43,9 +49,9 @@ struct stacktrace_ops {
walk_stack_t walk_stack;
};
-void dump_trace(struct task_struct *tsk, struct pt_regs *regs,
- unsigned long *stack, unsigned long bp,
- const struct stacktrace_ops *ops, void *data);
+int dump_trace(struct task_struct *tsk, struct pt_regs *regs,
+ unsigned long *stack, unsigned long bp,
+ const struct stacktrace_ops *ops, void *data);
#ifdef CONFIG_X86_32
#define STACKSLOTS_PER_LINE 8
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 2bb25c3..13d240c 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -92,23 +92,22 @@ static inline int valid_stack_ptr(struct thread_info *tinfo,
return p > t && p < t + THREAD_SIZE - size;
}
-unsigned long
-print_context_stack(struct thread_info *tinfo,
- unsigned long *stack, unsigned long bp,
- const struct stacktrace_ops *ops, void *data,
- unsigned long *end, int *graph)
+int print_context_stack(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 *frame = (struct stack_frame *)*bp;
while (valid_stack_ptr(tinfo, stack, sizeof(*stack), end)) {
unsigned long addr;
addr = *stack;
if (__kernel_text_address(addr)) {
- if ((unsigned long) stack == bp + sizeof(long)) {
+ if ((unsigned long) stack == *bp + sizeof(long)) {
ops->address(data, addr, 1);
frame = frame->next_frame;
- bp = (unsigned long) frame;
+ *bp = (unsigned long) frame;
} else {
ops->address(data, addr, 0);
}
@@ -116,17 +115,16 @@ print_context_stack(struct thread_info *tinfo,
}
stack++;
}
- return bp;
+ return 0;
}
EXPORT_SYMBOL_GPL(print_context_stack);
-unsigned long
-print_context_stack_bp(struct thread_info *tinfo,
- unsigned long *stack, unsigned long bp,
- const struct stacktrace_ops *ops, void *data,
- unsigned long *end, int *graph)
+int print_context_stack_bp(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 *frame = (struct stack_frame *)*bp;
unsigned long *ret_addr = &frame->return_address;
while (valid_stack_ptr(tinfo, ret_addr, sizeof(*ret_addr), end)) {
@@ -142,7 +140,8 @@ print_context_stack_bp(struct thread_info *tinfo,
print_ftrace_graph_addr(addr, data, ops, tinfo, graph);
}
- return (unsigned long)frame;
+ *bp = (unsigned long)frame;
+ return 0;
}
EXPORT_SYMBOL_GPL(print_context_stack_bp);
diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 464ffd6..e710bab 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -38,13 +38,14 @@ static void *is_softirq_stack(unsigned long *stack, int cpu)
return is_irq_stack(stack, irq);
}
-void dump_trace(struct task_struct *task, struct pt_regs *regs,
- unsigned long *stack, unsigned long bp,
- const struct stacktrace_ops *ops, void *data)
+int dump_trace(struct task_struct *task, struct pt_regs *regs,
+ unsigned long *stack, unsigned long bp,
+ const struct stacktrace_ops *ops, void *data)
{
const unsigned cpu = get_cpu();
int graph = 0;
u32 *prev_esp;
+ int ret;
if (!task)
task = current;
@@ -69,8 +70,10 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
end_stack = is_softirq_stack(stack, cpu);
context = task_thread_info(task);
- bp = ops->walk_stack(context, stack, bp, ops, data,
- end_stack, &graph);
+ ret = ops->walk_stack(context, stack, &bp, ops, data,
+ end_stack, &graph);
+ if (ret)
+ goto out;
/* Stop if not on irq stack */
if (!end_stack)
@@ -82,11 +85,16 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
if (!stack)
break;
- if (ops->stack(data, "IRQ") < 0)
- break;
+ ret = ops->stack(data, "IRQ");
+ if (ret)
+ goto out;
+
touch_nmi_watchdog();
}
+
+out:
put_cpu();
+ return ret;
}
EXPORT_SYMBOL(dump_trace);
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 5f1c626..0c810ba 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -148,9 +148,9 @@ analyze_stack(int cpu, struct task_struct *task, unsigned long *stack,
* severe exception (double fault, nmi, stack fault, debug, mce) hardware stack
*/
-void dump_trace(struct task_struct *task, struct pt_regs *regs,
- unsigned long *stack, unsigned long bp,
- const struct stacktrace_ops *ops, void *data)
+int dump_trace(struct task_struct *task, struct pt_regs *regs,
+ unsigned long *stack, unsigned long bp,
+ const struct stacktrace_ops *ops, void *data)
{
const unsigned cpu = get_cpu();
struct thread_info *tinfo;
@@ -159,6 +159,7 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
unsigned used = 0;
int graph = 0;
int done = 0;
+ int ret;
if (!task)
task = current;
@@ -198,13 +199,18 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
break;
case STACK_IS_EXCEPTION:
-
- if (ops->stack(data, id) < 0)
- break;
-
- bp = ops->walk_stack(tinfo, stack, bp, ops,
- data, stack_end, &graph);
- ops->stack(data, "<EOE>");
+ ret = ops->stack(data, id);
+ if (ret)
+ goto out;
+
+ ret = ops->walk_stack(tinfo, stack, &bp, ops, data,
+ stack_end, &graph);
+ if (ret)
+ goto out;
+
+ ret = ops->stack(data, "<EOE>");
+ if (ret)
+ goto out;
/*
* We link to the next stack via the
* second-to-last pointer (index -2 to end) in the
@@ -215,11 +221,15 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
break;
case STACK_IS_IRQ:
+ ret = ops->stack(data, "IRQ");
+ if (ret)
+ goto out;
+
+ ret = ops->walk_stack(tinfo, stack, &bp, ops, data,
+ stack_end, &graph);
+ if (ret)
+ goto out;
- if (ops->stack(data, "IRQ") < 0)
- break;
- bp = ops->walk_stack(tinfo, stack, bp,
- ops, data, stack_end, &graph);
/*
* We link to the next stack (which would be
* the process stack normally) the last
@@ -227,12 +237,18 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
*/
stack = (unsigned long *) (stack_end[-1]);
irq_stack = NULL;
- ops->stack(data, "EOI");
+
+ ret = ops->stack(data, "EOI");
+ if (ret)
+ goto out;
+
done = 0;
break;
case STACK_IS_UNKNOWN:
- ops->stack(data, "UNK");
+ ret = ops->stack(data, "UNK");
+ if (ret)
+ goto out;
break;
}
}
@@ -240,8 +256,11 @@ void dump_trace(struct task_struct *task, struct pt_regs *regs,
/*
* This handles the process stack:
*/
- bp = ops->walk_stack(tinfo, stack, bp, ops, data, NULL, &graph);
+ ret = ops->walk_stack(tinfo, stack, &bp, ops, data, NULL, &graph);
+
+out:
put_cpu();
+ return ret;
}
EXPORT_SYMBOL(dump_trace);
--
2.4.11
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 <[email protected]>
---
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 4c941f8..79e15ef 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.11
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 nice 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 TIF_FORK tasks which don't have a stack at all
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 <[email protected]>
---
arch/x86/kernel/head_64.S | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 6dbd2c0..0b12311 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -296,8 +296,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
@@ -325,7 +326,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.11
The stack_start variable is similar in usage to initial_code and
initial_gs: they're all stored in head_64.S and they're all updated by
SMP and suspend/resume before starting a CPU.
Rename stack_start to initial_stack to be more consistent with the
others.
Also do a few other related cleanups:
- Remove the unused init_rsp variable declaration.
- Remove the ".word 0" statement after the initial_stack definition
because it has no apparent function.
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/include/asm/realmode.h | 2 +-
arch/x86/include/asm/smp.h | 3 ---
arch/x86/kernel/acpi/sleep.c | 2 +-
arch/x86/kernel/head_32.S | 8 ++++----
arch/x86/kernel/head_64.S | 10 ++++------
arch/x86/kernel/smpboot.c | 2 +-
6 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/arch/x86/include/asm/realmode.h b/arch/x86/include/asm/realmode.h
index 9c6b890..677a671 100644
--- a/arch/x86/include/asm/realmode.h
+++ b/arch/x86/include/asm/realmode.h
@@ -44,9 +44,9 @@ struct trampoline_header {
extern struct real_mode_header *real_mode_header;
extern unsigned char real_mode_blob_end[];
-extern unsigned long init_rsp;
extern unsigned long initial_code;
extern unsigned long initial_gs;
+extern unsigned long initial_stack;
extern unsigned char real_mode_blob[];
extern unsigned char real_mode_relocs[];
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 66b0573..a9ac31b 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -38,9 +38,6 @@ DECLARE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_bios_cpu_apicid);
DECLARE_EARLY_PER_CPU_READ_MOSTLY(int, x86_cpu_to_logical_apicid);
#endif
-/* Static state in head.S used to set up a CPU */
-extern unsigned long stack_start; /* Initial stack pointer address */
-
struct task_struct;
struct smp_ops {
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index adb3eaf..4858733 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -99,7 +99,7 @@ int x86_acpi_suspend_lowlevel(void)
saved_magic = 0x12345678;
#else /* CONFIG_64BIT */
#ifdef CONFIG_SMP
- stack_start = (unsigned long)temp_stack + sizeof(temp_stack);
+ initial_stack = (unsigned long)temp_stack + sizeof(temp_stack);
early_gdt_descr.address =
(unsigned long)get_cpu_gdt_table(smp_processor_id());
initial_gs = per_cpu_offset(smp_processor_id());
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 6770865..da840be 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -94,7 +94,7 @@ RESERVE_BRK(pagetables, INIT_MAP_SIZE)
*/
__HEAD
ENTRY(startup_32)
- movl pa(stack_start),%ecx
+ movl pa(initial_stack),%ecx
/* test KEEP_SEGMENTS flag to see if the bootloader is asking
us to not reload segments */
@@ -286,7 +286,7 @@ num_subarch_entries = (. - subarch_entries) / 4
* start_secondary().
*/
ENTRY(start_cpu0)
- movl stack_start, %ecx
+ movl initial_stack, %ecx
movl %ecx, %esp
jmp *(initial_code)
ENDPROC(start_cpu0)
@@ -307,7 +307,7 @@ ENTRY(startup_32_smp)
movl %eax,%es
movl %eax,%fs
movl %eax,%gs
- movl pa(stack_start),%ecx
+ movl pa(initial_stack),%ecx
movl %eax,%ss
leal -__PAGE_OFFSET(%ecx),%esp
@@ -709,7 +709,7 @@ ENTRY(initial_page_table)
.data
.balign 4
-ENTRY(stack_start)
+ENTRY(initial_stack)
.long init_thread_union+THREAD_SIZE
__INITRODATA
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 5df831e..792c3bb 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -226,7 +226,7 @@ ENTRY(secondary_startup_64)
movq %rax, %cr0
/* Setup a boot time stack */
- movq stack_start(%rip), %rsp
+ movq initial_stack(%rip), %rsp
/* zero EFLAGS after setting rsp */
pushq $0
@@ -309,7 +309,7 @@ ENTRY(secondary_startup_64)
* start_secondary().
*/
ENTRY(start_cpu0)
- movq stack_start(%rip),%rsp
+ movq initial_stack(%rip),%rsp
movq initial_code(%rip),%rax
pushq $0 # fake return address to stop unwinder
pushq $__KERNEL_CS # set correct cs
@@ -318,17 +318,15 @@ ENTRY(start_cpu0)
ENDPROC(start_cpu0)
#endif
- /* SMP bootup changes these two */
+ /* SMP bootup changes these variables */
__REFDATA
.balign 8
GLOBAL(initial_code)
.quad x86_64_start_kernel
GLOBAL(initial_gs)
.quad INIT_PER_CPU_VAR(irq_stack_union)
-
- GLOBAL(stack_start)
+ GLOBAL(initial_stack)
.quad init_thread_union+THREAD_SIZE-8
- .word 0
__FINITDATA
bad_address:
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 1fe4130..503682a 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -950,7 +950,7 @@ static int do_boot_cpu(int apicid, int cpu, struct task_struct *idle)
early_gdt_descr.address = (unsigned long)get_cpu_gdt_table(cpu);
initial_code = (unsigned long)start_secondary;
- stack_start = idle->thread.sp;
+ initial_stack = idle->thread.sp;
/*
* Enable the espfix hack for this CPU
--
2.4.11
On 04/28/16 at 03:44P, Josh Poimboeuf wrote:
> In preparation for being able to determine whether a given stack trace
> is reliable, allow the stacktrace_ops functions to propagate errors to
> dump_trace().
Hi, Josh.
Have you considered to make walk_stack function as non-return function,
since there is no obvious error during detecting the frame points?
Thanks
Minfei
On Fri, Apr 29, 2016 at 09:45:58PM +0800, Minfei Huang wrote:
> On 04/28/16 at 03:44P, Josh Poimboeuf wrote:
> > In preparation for being able to determine whether a given stack trace
> > is reliable, allow the stacktrace_ops functions to propagate errors to
> > dump_trace().
>
> Hi, Josh.
>
> Have you considered to make walk_stack function as non-return function,
> since there is no obvious error during detecting the frame points?
If you look at the next patch 07/18, there are several cases where
walk_stack (print_context_stack_reliable) returns an error.
For example, if a function gets preempted before it gets a chance to
save the frame pointer, the function's caller would get skipped on the
stack trace. So for preempted tasks, we always have to consider their
stacks unreliable.
--
Josh
On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> A preempted function might not have had a chance to save the frame
> pointer to the stack yet, which can result in its caller getting skipped
> on a stack trace.
>
> Add a flag to indicate when the task has been preempted so that stack
> dump code can determine whether the stack trace is reliable.
I think I like this, but how do you handle the rather similar case in
which a task goes to sleep because it's waiting on IO that happened in
response to get_user, put_user, copy_from_user, etc?
--Andy
On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> Add the TIF_PATCH_PENDING thread flag to enable the new livepatch
> per-task consistency model for x86_64. The bit getting set indicates
> the thread has a pending patch which needs to be applied when the thread
> exits the kernel.
>
> The bit is placed in the least-significant word of the thread_info flags
NAK to that part.
The least-significant word thing is a huge hack that has gotten out of
control. Please add the thing explicitly to all relevant masks.
--Andy
On Thu, Apr 28, 2016 at 4:44 PM, Josh Poimboeuf <[email protected]> wrote:
> 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 nice 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 TIF_FORK tasks which don't have a stack at all
>
> 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 <[email protected]>
> ---
> arch/x86/kernel/head_64.S | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 6dbd2c0..0b12311 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -296,8 +296,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
This chunk looks like it should be a separate patch.
--
Brian Gerst
On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> 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 nice 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 TIF_FORK tasks which don't have a stack at all
>
> 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 <[email protected]>
> ---
> arch/x86/kernel/head_64.S | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 6dbd2c0..0b12311 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -296,8 +296,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
> @@ -325,7 +326,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
As long as you're doing this, could you also set orig_ax to -1? I
remember running into some oddities resulting from orig_ax containing
garbage at some point.
--Andy
On Fri, Apr 29, 2016 at 11:06:53AM -0700, Andy Lutomirski wrote:
> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> > A preempted function might not have had a chance to save the frame
> > pointer to the stack yet, which can result in its caller getting skipped
> > on a stack trace.
> >
> > Add a flag to indicate when the task has been preempted so that stack
> > dump code can determine whether the stack trace is reliable.
>
> I think I like this, but how do you handle the rather similar case in
> which a task goes to sleep because it's waiting on IO that happened in
> response to get_user, put_user, copy_from_user, etc?
Hm, good question. I was thinking that page faults had a dedicated
stack, but now looking at the entry and traps code, that doesn't seem to
be the case.
Anyway I think it shouldn't be a problem if we make sure that any kernel
function which might trigger a valid page fault (e.g.,
copy_user_generic_string) do the proper frame pointer setup first. Then
the stack should still be reliable.
In fact I might be able to teach objtool to enforce that: any function
which uses an exception table should create a stack frame.
Or alternatively, maybe set some kind of flag for page faults, similar
to what I did with this patch.
--
Josh
On Fri, Apr 29, 2016 at 11:08:04AM -0700, Andy Lutomirski wrote:
> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> > Add the TIF_PATCH_PENDING thread flag to enable the new livepatch
> > per-task consistency model for x86_64. The bit getting set indicates
> > the thread has a pending patch which needs to be applied when the thread
> > exits the kernel.
> >
> > The bit is placed in the least-significant word of the thread_info flags
>
> NAK to that part.
>
> The least-significant word thing is a huge hack that has gotten out of
> control. Please add the thing explicitly to all relevant masks.
Yeah, it is quite dangerous. I'll make it explicit, and make all the
other _TIF_ALLWORK_MASK flags explicit while I'm at it.
--
Josh
On Fri, Apr 29, 2016 at 1:11 PM, Josh Poimboeuf <[email protected]> wrote:
> On Fri, Apr 29, 2016 at 11:06:53AM -0700, Andy Lutomirski wrote:
>> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
>> > A preempted function might not have had a chance to save the frame
>> > pointer to the stack yet, which can result in its caller getting skipped
>> > on a stack trace.
>> >
>> > Add a flag to indicate when the task has been preempted so that stack
>> > dump code can determine whether the stack trace is reliable.
>>
>> I think I like this, but how do you handle the rather similar case in
>> which a task goes to sleep because it's waiting on IO that happened in
>> response to get_user, put_user, copy_from_user, etc?
>
> Hm, good question. I was thinking that page faults had a dedicated
> stack, but now looking at the entry and traps code, that doesn't seem to
> be the case.
>
> Anyway I think it shouldn't be a problem if we make sure that any kernel
> function which might trigger a valid page fault (e.g.,
> copy_user_generic_string) do the proper frame pointer setup first. Then
> the stack should still be reliable.
>
> In fact I might be able to teach objtool to enforce that: any function
> which uses an exception table should create a stack frame.
>
> Or alternatively, maybe set some kind of flag for page faults, similar
> to what I did with this patch.
>
How about doing it the other way around: teach the unwinder to detect
when it hits a non-outermost entry (i.e. it lands in idtentry, etc)
and use some reasonable heuristic as to whether it's okay to keep
unwinding. You should be able to handle preemption like that, too --
the unwind process will end up in an IRQ frame.
--Andy
On Fri, Apr 29, 2016 at 01:19:23PM -0700, Andy Lutomirski wrote:
> On Fri, Apr 29, 2016 at 1:11 PM, Josh Poimboeuf <[email protected]> wrote:
> > On Fri, Apr 29, 2016 at 11:06:53AM -0700, Andy Lutomirski wrote:
> >> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> >> > A preempted function might not have had a chance to save the frame
> >> > pointer to the stack yet, which can result in its caller getting skipped
> >> > on a stack trace.
> >> >
> >> > Add a flag to indicate when the task has been preempted so that stack
> >> > dump code can determine whether the stack trace is reliable.
> >>
> >> I think I like this, but how do you handle the rather similar case in
> >> which a task goes to sleep because it's waiting on IO that happened in
> >> response to get_user, put_user, copy_from_user, etc?
> >
> > Hm, good question. I was thinking that page faults had a dedicated
> > stack, but now looking at the entry and traps code, that doesn't seem to
> > be the case.
> >
> > Anyway I think it shouldn't be a problem if we make sure that any kernel
> > function which might trigger a valid page fault (e.g.,
> > copy_user_generic_string) do the proper frame pointer setup first. Then
> > the stack should still be reliable.
> >
> > In fact I might be able to teach objtool to enforce that: any function
> > which uses an exception table should create a stack frame.
> >
> > Or alternatively, maybe set some kind of flag for page faults, similar
> > to what I did with this patch.
> >
>
> How about doing it the other way around: teach the unwinder to detect
> when it hits a non-outermost entry (i.e. it lands in idtentry, etc)
> and use some reasonable heuristic as to whether it's okay to keep
> unwinding. You should be able to handle preemption like that, too --
> the unwind process will end up in an IRQ frame.
How exactly would the unwinder detect if a text address is in an
idtentry? Maybe put all the idt entries in a special ELF section?
--
Josh
On Fri, Apr 29, 2016 at 02:46:10PM -0400, Brian Gerst wrote:
> On Thu, Apr 28, 2016 at 4:44 PM, Josh Poimboeuf <[email protected]> wrote:
> > 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 nice 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 TIF_FORK tasks which don't have a stack at all
> >
> > 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 <[email protected]>
> > ---
> > arch/x86/kernel/head_64.S | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> > index 6dbd2c0..0b12311 100644
> > --- a/arch/x86/kernel/head_64.S
> > +++ b/arch/x86/kernel/head_64.S
> > @@ -296,8 +296,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
>
> This chunk looks like it should be a separate patch.
Agreed, thanks.
--
Josh
On Fri, Apr 29, 2016 at 1:27 PM, Josh Poimboeuf <[email protected]> wrote:
> On Fri, Apr 29, 2016 at 01:19:23PM -0700, Andy Lutomirski wrote:
>> On Fri, Apr 29, 2016 at 1:11 PM, Josh Poimboeuf <[email protected]> wrote:
>> > On Fri, Apr 29, 2016 at 11:06:53AM -0700, Andy Lutomirski wrote:
>> >> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
>> >> > A preempted function might not have had a chance to save the frame
>> >> > pointer to the stack yet, which can result in its caller getting skipped
>> >> > on a stack trace.
>> >> >
>> >> > Add a flag to indicate when the task has been preempted so that stack
>> >> > dump code can determine whether the stack trace is reliable.
>> >>
>> >> I think I like this, but how do you handle the rather similar case in
>> >> which a task goes to sleep because it's waiting on IO that happened in
>> >> response to get_user, put_user, copy_from_user, etc?
>> >
>> > Hm, good question. I was thinking that page faults had a dedicated
>> > stack, but now looking at the entry and traps code, that doesn't seem to
>> > be the case.
>> >
>> > Anyway I think it shouldn't be a problem if we make sure that any kernel
>> > function which might trigger a valid page fault (e.g.,
>> > copy_user_generic_string) do the proper frame pointer setup first. Then
>> > the stack should still be reliable.
>> >
>> > In fact I might be able to teach objtool to enforce that: any function
>> > which uses an exception table should create a stack frame.
>> >
>> > Or alternatively, maybe set some kind of flag for page faults, similar
>> > to what I did with this patch.
>> >
>>
>> How about doing it the other way around: teach the unwinder to detect
>> when it hits a non-outermost entry (i.e. it lands in idtentry, etc)
>> and use some reasonable heuristic as to whether it's okay to keep
>> unwinding. You should be able to handle preemption like that, too --
>> the unwind process will end up in an IRQ frame.
>
> How exactly would the unwinder detect if a text address is in an
> idtentry? Maybe put all the idt entries in a special ELF section?
>
Hmm.
What actually happens when you unwind all the way into the entry code?
Don't you end up in something that isn't in an ELF function? Can you
detect that? Ideally, the unwinder could actually detect that it's
hit a pt_regs struct and report that. If used for stack dumps, it
could display some indication of this and then continue its unwinding
by decoding the pt_regs. If used for patching, it could take some
other appropriate action.
I would have no objection to annotating all the pt_regs-style entry
code, whether by putting it in a separate section or by making a table
of addresses.
There are a couple of nasty cases if NMI or MCE is involved but, as of
4.6, outside of NMI, MCE, and vmalloc faults (ugh!), there should
always be a complete pt_regs on the stack before interrupts get
enabled for each entry. Of course, finding the thing may be
nontrivial in case other things were pushed. I suppose we could try
to rejigger the code so that rbp points to pt_regs or similar.
--Andy
On Fri, Apr 29, 2016 at 12:39:16PM -0700, Andy Lutomirski wrote:
> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> > 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 nice 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 TIF_FORK tasks which don't have a stack at all
> >
> > 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 <[email protected]>
> > ---
> > arch/x86/kernel/head_64.S | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> > index 6dbd2c0..0b12311 100644
> > --- a/arch/x86/kernel/head_64.S
> > +++ b/arch/x86/kernel/head_64.S
> > @@ -296,8 +296,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
> > @@ -325,7 +326,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
>
> As long as you're doing this, could you also set orig_ax to -1? I
> remember running into some oddities resulting from orig_ax containing
> garbage at some point.
I assume you mean to initialize the orig_rax value in the pt_regs at the
bottom of the stack of the idle task?
How could that cause a problem? Since the idle task never returns from
a system call, I'd assume that memory never gets accessed?
--
Josh
On Fri, Apr 29, 2016 at 01:32:53PM -0700, Andy Lutomirski wrote:
> On Fri, Apr 29, 2016 at 1:27 PM, Josh Poimboeuf <[email protected]> wrote:
> > On Fri, Apr 29, 2016 at 01:19:23PM -0700, Andy Lutomirski wrote:
> >> On Fri, Apr 29, 2016 at 1:11 PM, Josh Poimboeuf <[email protected]> wrote:
> >> > On Fri, Apr 29, 2016 at 11:06:53AM -0700, Andy Lutomirski wrote:
> >> >> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> >> >> > A preempted function might not have had a chance to save the frame
> >> >> > pointer to the stack yet, which can result in its caller getting skipped
> >> >> > on a stack trace.
> >> >> >
> >> >> > Add a flag to indicate when the task has been preempted so that stack
> >> >> > dump code can determine whether the stack trace is reliable.
> >> >>
> >> >> I think I like this, but how do you handle the rather similar case in
> >> >> which a task goes to sleep because it's waiting on IO that happened in
> >> >> response to get_user, put_user, copy_from_user, etc?
> >> >
> >> > Hm, good question. I was thinking that page faults had a dedicated
> >> > stack, but now looking at the entry and traps code, that doesn't seem to
> >> > be the case.
> >> >
> >> > Anyway I think it shouldn't be a problem if we make sure that any kernel
> >> > function which might trigger a valid page fault (e.g.,
> >> > copy_user_generic_string) do the proper frame pointer setup first. Then
> >> > the stack should still be reliable.
> >> >
> >> > In fact I might be able to teach objtool to enforce that: any function
> >> > which uses an exception table should create a stack frame.
> >> >
> >> > Or alternatively, maybe set some kind of flag for page faults, similar
> >> > to what I did with this patch.
> >> >
> >>
> >> How about doing it the other way around: teach the unwinder to detect
> >> when it hits a non-outermost entry (i.e. it lands in idtentry, etc)
> >> and use some reasonable heuristic as to whether it's okay to keep
> >> unwinding. You should be able to handle preemption like that, too --
> >> the unwind process will end up in an IRQ frame.
> >
> > How exactly would the unwinder detect if a text address is in an
> > idtentry? Maybe put all the idt entries in a special ELF section?
> >
>
> Hmm.
>
> What actually happens when you unwind all the way into the entry code?
> Don't you end up in something that isn't in an ELF function? Can you
> detect that?
For entry from user space (e.g., syscalls), it's easy to detect because
there's always a pt_regs at the bottom of the stack. So if the unwinder
reaches the stack address at (thread.sp0 - sizeof(pt_regs)), it knows
it's done.
But for nested entry (e.g. in-kernel irqs/exceptions like preemption and
page faults which don't have dedicated stacks), where the pt_regs is
stored somewhere in the middle of the stack instead of the bottom,
there's no reliable way to detect that.
> Ideally, the unwinder could actually detect that it's
> hit a pt_regs struct and report that. If used for stack dumps, it
> could display some indication of this and then continue its unwinding
> by decoding the pt_regs. If used for patching, it could take some
> other appropriate action.
>
> I would have no objection to annotating all the pt_regs-style entry
> code, whether by putting it in a separate section or by making a table
> of addresses.
I think the easiest way to make it work would be to modify the idtentry
macro to put all the idt entries in a dedicated section. Then the
unwinder could easily detect any calls from that code.
> There are a couple of nasty cases if NMI or MCE is involved but, as of
> 4.6, outside of NMI, MCE, and vmalloc faults (ugh!), there should
> always be a complete pt_regs on the stack before interrupts get
> enabled for each entry. Of course, finding the thing may be
> nontrivial in case other things were pushed.
NMI, MCE and interrupts aren't a problem because they have dedicated
stacks, which are easy to detect. If the tasks' stack is on an
exception stack or an irq stack, we consider it unreliable.
And also, they don't sleep. The stack of any running task (other than
current) is automatically considered unreliable anyway, since they could
be modifying it while we're reading it.
> I suppose we could try to rejigger the code so that rbp points to
> pt_regs or similar.
I think we should avoid doing something like that because it would break
gdb and all the other unwinders who don't know about it.
--
Josh
On Fri, Apr 29, 2016 at 2:25 PM, Josh Poimboeuf <[email protected]> wrote:
> On Fri, Apr 29, 2016 at 01:32:53PM -0700, Andy Lutomirski wrote:
>> On Fri, Apr 29, 2016 at 1:27 PM, Josh Poimboeuf <[email protected]> wrote:
>> > On Fri, Apr 29, 2016 at 01:19:23PM -0700, Andy Lutomirski wrote:
>> >> On Fri, Apr 29, 2016 at 1:11 PM, Josh Poimboeuf <[email protected]> wrote:
>> >> > On Fri, Apr 29, 2016 at 11:06:53AM -0700, Andy Lutomirski wrote:
>> >> >> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
>> >> >> > A preempted function might not have had a chance to save the frame
>> >> >> > pointer to the stack yet, which can result in its caller getting skipped
>> >> >> > on a stack trace.
>> >> >> >
>> >> >> > Add a flag to indicate when the task has been preempted so that stack
>> >> >> > dump code can determine whether the stack trace is reliable.
>> >> >>
>> >> >> I think I like this, but how do you handle the rather similar case in
>> >> >> which a task goes to sleep because it's waiting on IO that happened in
>> >> >> response to get_user, put_user, copy_from_user, etc?
>> >> >
>> >> > Hm, good question. I was thinking that page faults had a dedicated
>> >> > stack, but now looking at the entry and traps code, that doesn't seem to
>> >> > be the case.
>> >> >
>> >> > Anyway I think it shouldn't be a problem if we make sure that any kernel
>> >> > function which might trigger a valid page fault (e.g.,
>> >> > copy_user_generic_string) do the proper frame pointer setup first. Then
>> >> > the stack should still be reliable.
>> >> >
>> >> > In fact I might be able to teach objtool to enforce that: any function
>> >> > which uses an exception table should create a stack frame.
>> >> >
>> >> > Or alternatively, maybe set some kind of flag for page faults, similar
>> >> > to what I did with this patch.
>> >> >
>> >>
>> >> How about doing it the other way around: teach the unwinder to detect
>> >> when it hits a non-outermost entry (i.e. it lands in idtentry, etc)
>> >> and use some reasonable heuristic as to whether it's okay to keep
>> >> unwinding. You should be able to handle preemption like that, too --
>> >> the unwind process will end up in an IRQ frame.
>> >
>> > How exactly would the unwinder detect if a text address is in an
>> > idtentry? Maybe put all the idt entries in a special ELF section?
>> >
>>
>> Hmm.
>>
>> What actually happens when you unwind all the way into the entry code?
>> Don't you end up in something that isn't in an ELF function? Can you
>> detect that?
>
> For entry from user space (e.g., syscalls), it's easy to detect because
> there's always a pt_regs at the bottom of the stack. So if the unwinder
> reaches the stack address at (thread.sp0 - sizeof(pt_regs)), it knows
> it's done.
>
> But for nested entry (e.g. in-kernel irqs/exceptions like preemption and
> page faults which don't have dedicated stacks), where the pt_regs is
> stored somewhere in the middle of the stack instead of the bottom,
> there's no reliable way to detect that.
>
>> Ideally, the unwinder could actually detect that it's
>> hit a pt_regs struct and report that. If used for stack dumps, it
>> could display some indication of this and then continue its unwinding
>> by decoding the pt_regs. If used for patching, it could take some
>> other appropriate action.
>>
>> I would have no objection to annotating all the pt_regs-style entry
>> code, whether by putting it in a separate section or by making a table
>> of addresses.
>
> I think the easiest way to make it work would be to modify the idtentry
> macro to put all the idt entries in a dedicated section. Then the
> unwinder could easily detect any calls from that code.
That would work. Would it make sense to do the same for the irq entries?
I'd be glad to review a patch. It should be straightforward.
>
>> There are a couple of nasty cases if NMI or MCE is involved but, as of
>> 4.6, outside of NMI, MCE, and vmalloc faults (ugh!), there should
>> always be a complete pt_regs on the stack before interrupts get
>> enabled for each entry. Of course, finding the thing may be
>> nontrivial in case other things were pushed.
>
> NMI, MCE and interrupts aren't a problem because they have dedicated
> stacks, which are easy to detect. If the tasks' stack is on an
> exception stack or an irq stack, we consider it unreliable.
Only on x86_64.
>
> And also, they don't sleep. The stack of any running task (other than
> current) is automatically considered unreliable anyway, since they could
> be modifying it while we're reading it.
True.
>
>> I suppose we could try to rejigger the code so that rbp points to
>> pt_regs or similar.
>
> I think we should avoid doing something like that because it would break
> gdb and all the other unwinders who don't know about it.
How so?
Currently, rbp in the entry code is meaningless. I'm suggesting that,
when we do, for example, 'call \do_sym' in idtentry, we point rbp to
the pt_regs. Currently it points to something stale (which the
dump_stack code might be relying on. Hmm.) But it's probably also
safe to assume that if you unwind to the 'call \do_sym', then pt_regs
is the next thing on the stack, so just doing the section thing would
work.
We should really re-add DWARF some day.
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
On Fri, Apr 29, 2016 at 1:50 PM, Josh Poimboeuf <[email protected]> wrote:
> On Fri, Apr 29, 2016 at 12:39:16PM -0700, Andy Lutomirski wrote:
>> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
>> > 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 nice 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 TIF_FORK tasks which don't have a stack at all
>> >
>> > 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 <[email protected]>
>> > ---
>> > arch/x86/kernel/head_64.S | 7 ++++---
>> > 1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>> > index 6dbd2c0..0b12311 100644
>> > --- a/arch/x86/kernel/head_64.S
>> > +++ b/arch/x86/kernel/head_64.S
>> > @@ -296,8 +296,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
>> > @@ -325,7 +326,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
>>
>> As long as you're doing this, could you also set orig_ax to -1? I
>> remember running into some oddities resulting from orig_ax containing
>> garbage at some point.
>
> I assume you mean to initialize the orig_rax value in the pt_regs at the
> bottom of the stack of the idle task?
>
> How could that cause a problem? Since the idle task never returns from
> a system call, I'd assume that memory never gets accessed?
>
Look at collect_syscall in lib/syscall.c
> --
> Josh
--
Andy Lutomirski
AMA Capital Management, LLC
On Fri, 29 Apr 2016, Andy Lutomirski wrote:
> > NMI, MCE and interrupts aren't a problem because they have dedicated
> > stacks, which are easy to detect. If the tasks' stack is on an
> > exception stack or an irq stack, we consider it unreliable.
>
> Only on x86_64.
Well, MCEs are more or less x86-specific as well. But otherwise good
point, thanks Andy.
So, how does stack layout generally look like in case when NMI is actually
running on proper kernel stack? I thought it's guaranteed to contain
pt_regs anyway in all cases. Is that not guaranteed to be the case?
Thanks,
--
Jiri Kosina
SUSE Labs
On Fri, Apr 29, 2016 at 02:37:41PM -0700, Andy Lutomirski wrote:
> On Fri, Apr 29, 2016 at 2:25 PM, Josh Poimboeuf <[email protected]> wrote:
> > I think the easiest way to make it work would be to modify the idtentry
> > macro to put all the idt entries in a dedicated section. Then the
> > unwinder could easily detect any calls from that code.
>
> That would work. Would it make sense to do the same for the irq entries?
Yes, I think so.
> >> I suppose we could try to rejigger the code so that rbp points to
> >> pt_regs or similar.
> >
> > I think we should avoid doing something like that because it would break
> > gdb and all the other unwinders who don't know about it.
>
> How so?
>
> Currently, rbp in the entry code is meaningless. I'm suggesting that,
> when we do, for example, 'call \do_sym' in idtentry, we point rbp to
> the pt_regs. Currently it points to something stale (which the
> dump_stack code might be relying on. Hmm.) But it's probably also
> safe to assume that if you unwind to the 'call \do_sym', then pt_regs
> is the next thing on the stack, so just doing the section thing would
> work.
Yes, rbp is meaningless on the entry from user space. But if an
in-kernel interrupt occurs (e.g. page fault, preemption) and you have
nested entry, rbp keeps its old value, right? So the unwinder can walk
past the nested entry frame and keep going until it gets to the original
entry.
> We should really re-add DWARF some day.
Working on it :-)
--
Josh
On Sat, Apr 30, 2016 at 12:11:45AM +0200, Jiri Kosina wrote:
> On Fri, 29 Apr 2016, Andy Lutomirski wrote:
> > > NMI, MCE and interrupts aren't a problem because they have dedicated
> > > stacks, which are easy to detect. If the tasks' stack is on an
> > > exception stack or an irq stack, we consider it unreliable.
> >
> > Only on x86_64.
>
> Well, MCEs are more or less x86-specific as well. But otherwise good
> point, thanks Andy.
>
> So, how does stack layout generally look like in case when NMI is actually
> running on proper kernel stack? I thought it's guaranteed to contain
> pt_regs anyway in all cases. Is that not guaranteed to be the case?
If the NMI were using the normal kernel stack and it interrupted kernel
space, pt_regs would be placed in the "middle" of the stack rather than
the bottom, and there's currently no way to detect that.
However, NMIs don't sleep, and we only consider sleeping tasks for stack
reliability, so it wouldn't be an issue anyway.
--
Josh
On Fri, Apr 29, 2016 at 02:38:02PM -0700, Andy Lutomirski wrote:
> On Fri, Apr 29, 2016 at 1:50 PM, Josh Poimboeuf <[email protected]> wrote:
> > On Fri, Apr 29, 2016 at 12:39:16PM -0700, Andy Lutomirski wrote:
> >> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> >> > 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 nice 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 TIF_FORK tasks which don't have a stack at all
> >> >
> >> > 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 <[email protected]>
> >> > ---
> >> > arch/x86/kernel/head_64.S | 7 ++++---
> >> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> >> > index 6dbd2c0..0b12311 100644
> >> > --- a/arch/x86/kernel/head_64.S
> >> > +++ b/arch/x86/kernel/head_64.S
> >> > @@ -296,8 +296,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
> >> > @@ -325,7 +326,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
> >>
> >> As long as you're doing this, could you also set orig_ax to -1? I
> >> remember running into some oddities resulting from orig_ax containing
> >> garbage at some point.
> >
> > I assume you mean to initialize the orig_rax value in the pt_regs at the
> > bottom of the stack of the idle task?
> >
> > How could that cause a problem? Since the idle task never returns from
> > a system call, I'd assume that memory never gets accessed?
> >
>
> Look at collect_syscall in lib/syscall.c
I don't see how collect_syscall() can be called for the per-cpu idle
"swapper" tasks (which is what the above code affects). They don't have
pids or /proc entries so you can't do /proc/<pid>/syscall on them.
--
Josh
On Apr 29, 2016 3:41 PM, "Josh Poimboeuf" <[email protected]> wrote:
>
> On Fri, Apr 29, 2016 at 02:37:41PM -0700, Andy Lutomirski wrote:
> > On Fri, Apr 29, 2016 at 2:25 PM, Josh Poimboeuf <[email protected]> wrote:
> > > I think the easiest way to make it work would be to modify the idtentry
> > > macro to put all the idt entries in a dedicated section. Then the
> > > unwinder could easily detect any calls from that code.
> >
> > That would work. Would it make sense to do the same for the irq entries?
>
> Yes, I think so.
>
> > >> I suppose we could try to rejigger the code so that rbp points to
> > >> pt_regs or similar.
> > >
> > > I think we should avoid doing something like that because it would break
> > > gdb and all the other unwinders who don't know about it.
> >
> > How so?
> >
> > Currently, rbp in the entry code is meaningless. I'm suggesting that,
> > when we do, for example, 'call \do_sym' in idtentry, we point rbp to
> > the pt_regs. Currently it points to something stale (which the
> > dump_stack code might be relying on. Hmm.) But it's probably also
> > safe to assume that if you unwind to the 'call \do_sym', then pt_regs
> > is the next thing on the stack, so just doing the section thing would
> > work.
>
> Yes, rbp is meaningless on the entry from user space. But if an
> in-kernel interrupt occurs (e.g. page fault, preemption) and you have
> nested entry, rbp keeps its old value, right? So the unwinder can walk
> past the nested entry frame and keep going until it gets to the original
> entry.
Yes.
It would be nice if we could do better, though, and actually notice
the pt_regs and identify the entry. For example, I'd love to see
"page fault, RIP=xyz" printed in the middle of a stack dump on a
crash. Also, I think that just following rbp links will lose the
actual function that took the page fault (or whatever function
pt_regs->ip actually points to).
>
> > We should really re-add DWARF some day.
>
> Working on it :-)
Excellent.
Have you looked at my vdso unwinding test at all? If we could do
something similar for the kernel, IMO it would make testing much more
pleasant.
--Andy
On Apr 29, 2016 3:11 PM, "Jiri Kosina" <[email protected]> wrote:
>
> On Fri, 29 Apr 2016, Andy Lutomirski wrote:
>
> > > NMI, MCE and interrupts aren't a problem because they have dedicated
> > > stacks, which are easy to detect. If the tasks' stack is on an
> > > exception stack or an irq stack, we consider it unreliable.
> >
> > Only on x86_64.
>
> Well, MCEs are more or less x86-specific as well. But otherwise good
> point, thanks Andy.
>
> So, how does stack layout generally look like in case when NMI is actually
> running on proper kernel stack? I thought it's guaranteed to contain
> pt_regs anyway in all cases. Is that not guaranteed to be the case?
>
On x86, at least, there will still be pt_regs for the NMI. For the
interrupted state, though, there might not be pt_regs, as the NMI
might have happened while still populating pt_regs. In fact, the NMI
stack could overlap task_pt_regs.
For x86_32, there's no guarantee that pt_regs contains sp due to
hardware silliness. You need to parse it more carefully, as,
!user_mode(regs), then the old sp is just above pt_regs.
--Andy
On Apr 29, 2016 4:27 PM, "Josh Poimboeuf" <[email protected]> wrote:
>
> On Fri, Apr 29, 2016 at 02:38:02PM -0700, Andy Lutomirski wrote:
> > On Fri, Apr 29, 2016 at 1:50 PM, Josh Poimboeuf <[email protected]> wrote:
> > > On Fri, Apr 29, 2016 at 12:39:16PM -0700, Andy Lutomirski wrote:
> > >> On Thu, Apr 28, 2016 at 1:44 PM, Josh Poimboeuf <[email protected]> wrote:
> > >> > 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 nice 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 TIF_FORK tasks which don't have a stack at all
> > >> >
> > >> > 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 <[email protected]>
> > >> > ---
> > >> > arch/x86/kernel/head_64.S | 7 ++++---
> > >> > 1 file changed, 4 insertions(+), 3 deletions(-)
> > >> >
> > >> > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> > >> > index 6dbd2c0..0b12311 100644
> > >> > --- a/arch/x86/kernel/head_64.S
> > >> > +++ b/arch/x86/kernel/head_64.S
> > >> > @@ -296,8 +296,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
> > >> > @@ -325,7 +326,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
> > >>
> > >> As long as you're doing this, could you also set orig_ax to -1? I
> > >> remember running into some oddities resulting from orig_ax containing
> > >> garbage at some point.
> > >
> > > I assume you mean to initialize the orig_rax value in the pt_regs at the
> > > bottom of the stack of the idle task?
> > >
> > > How could that cause a problem? Since the idle task never returns from
> > > a system call, I'd assume that memory never gets accessed?
> > >
> >
> > Look at collect_syscall in lib/syscall.c
>
> I don't see how collect_syscall() can be called for the per-cpu idle
> "swapper" tasks (which is what the above code affects). They don't have
> pids or /proc entries so you can't do /proc/<pid>/syscall on them.
If so, then never mind.
--Andy
>
> --
> Josh