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

[Petr Mladek]

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

Just for record, this version looks fine to me:

Reviewed-by: Petr Mladek 

Best Regards,
Petr

PS: I was on the sick leave longer then expected. The patch set
has been pushed into the for-4.12 branch in jikos/livepatching.git
in the meantime. I check it there just for completeness. You do not
need to add my Reviewed-by tags.

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

[Balbir Singh]

On Tue, 2017-03-07 at 10:12 -0600, Josh Poimboeuf wrote:
> On Tue, Mar 07, 2017 at 05:50:55PM +1100, Balbir Singh wrote:
> > On Mon, 2017-02-13 at 19:42 -0600, Josh Poimboeuf wrote:
> > > For live patching and possibly other use cases, a stack trace is only
> > > useful if it can be assured that it's completely reliable.  Add a new
> > > save_stack_trace_tsk_reliable() function to achieve that.
> > > 
> > > Note that if the target task isn't the current task, and the target task
> > > is allowed to run, then it could be writing the stack while the unwinder
> > > is reading it, resulting in possible corruption.  So the caller of
> > > save_stack_trace_tsk_reliable() must ensure that the task is either
> > > 'current' or inactive.
> > > 
> > > save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection
> > > of pt_regs on the stack.  If the pt_regs are not user-mode registers
> > > from a syscall, then they indicate an in-kernel interrupt or exception
> > > (e.g. preemption or a page fault), in which case the stack is considered
> > > unreliable due to the nature of frame pointers.
> > > 
> > > It also relies on the x86 unwinder's detection of other issues, such as:
> > > 
> > > - corrupted stack data
> > > - stack grows the wrong way
> > > - stack walk doesn't reach the bottom
> > > - user didn't provide a large enough entries array
> > > 
> > > Such issues are reported by checking unwind_error() and !unwind_done().
> > > 
> > > Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can
> > > determine at build time whether the function is implemented.
> > > 
> > > Signed-off-by: Josh Poimboeuf 
> > > ---
> > 
> > Could you comment on why we need a reliable trace for live-patching? Are
> > we in any way reliant on the stack trace to patch something broken?
> 
> I tried to cover this comprehensively in patch 13/15 in
> Documentation/livepatch/livepatch.txt.  Does that answer your questions?
>

Yes, it answers my questions

Thanks,
Balbir Singh.

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

[Josh Poimboeuf]

On Tue, Mar 07, 2017 at 05:50:55PM +1100, Balbir Singh wrote:
> On Mon, 2017-02-13 at 19:42 -0600, Josh Poimboeuf wrote:
> > For live patching and possibly other use cases, a stack trace is only
> > useful if it can be assured that it's completely reliable.  Add a new
> > save_stack_trace_tsk_reliable() function to achieve that.
> > 
> > Note that if the target task isn't the current task, and the target task
> > is allowed to run, then it could be writing the stack while the unwinder
> > is reading it, resulting in possible corruption.  So the caller of
> > save_stack_trace_tsk_reliable() must ensure that the task is either
> > 'current' or inactive.
> > 
> > save_stack_trace_tsk_reliable() relies on the x86 unwinder's detection
> > of pt_regs on the stack.  If the pt_regs are not user-mode registers
> > from a syscall, then they indicate an in-kernel interrupt or exception
> > (e.g. preemption or a page fault), in which case the stack is considered
> > unreliable due to the nature of frame pointers.
> > 
> > It also relies on the x86 unwinder's detection of other issues, such as:
> > 
> > - corrupted stack data
> > - stack grows the wrong way
> > - stack walk doesn't reach the bottom
> > - user didn't provide a large enough entries array
> > 
> > Such issues are reported by checking unwind_error() and !unwind_done().
> > 
> > Also add CONFIG_HAVE_RELIABLE_STACKTRACE so arch-independent code can
> > determine at build time whether the function is implemented.
> > 
> > Signed-off-by: Josh Poimboeuf 
> > ---
> 
> Could you comment on why we need a reliable trace for live-patching? Are
> we in any way reliant on the stack trace to patch something broken?

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

-- 
Josh

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

[Balbir Singh]

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

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

Thanks,
Balbir Singh.

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

[Josh Poimboeuf]

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

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

-- 
Josh

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

[Miroslav Benes]

On Mon, 13 Feb 2017, Josh Poimboeuf wrote:

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

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

Reviewed-by: Miroslav Benes 

stays.

Regards,
Miroslav

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

[Josh Poimboeuf]

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

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

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

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

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

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

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

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

diff --git a/arch/Kconfig b/arch/Kconfig
index 80f3e5e..478b939 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -749,6 +749,12 @@ config HAVE_STACK_VALIDATION
  Architecture supports the 'objtool check' host tool command, which
  performs compile-time stack metadata validation.
 
+config HAVE_RELIABLE_STACKTRACE
+   bool
+   help
+ Architecture has a save_stack_trace_tsk_reliable() function which
+ only returns a stack trace if it can guarantee the trace is reliable.
+
 config HAVE_ARCH_HASH
bool
default n
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ea82a7b..e79fbf8 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -160,6 +160,7 @@ config X86
select HAVE_PERF_REGS
select HAVE_PERF_USER_STACK_DUMP
select HAVE_REGS_AND_STACK_ACCESS_API
+   select HAVE_RELIABLE_STACKTRACE if X86_64 && FRAME_POINTER && 
STACK_VALIDATION
select HAVE_STACK_VALIDATIONif X86_64
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_UNSTABLE_SCHED_CLOCK
diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
index 6fa75b1..137e9cce 100644
--- a/arch/x86/include/asm/unwind.h
+++ b/arch/x86/include/asm/unwind.h
@@ -11,6 +11,7 @@ struct unwind_state {
unsigned long stack_mask;
struct task_struct *task;
int graph_idx;
+   bool error;
 #ifdef CONFIG_FRAME_POINTER
unsigned long *bp, *orig_sp;
struct pt_regs *regs;
@@ -40,6 +41,11 @@ void unwind_start(struct unwind_state *state, struct 
task_struct *task,
__unwind_start(state, task, regs, first_frame);
 }
 
+static inline bool unwind_error(struct unwind_state *state)
+{
+   return state->error;
+}
+
 #ifdef CONFIG_FRAME_POINTER
 
 static inline
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 0653788..c5490d9 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -74,6 +74,101 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct 
stack_trace *trace)
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
 
+#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
+
+#define STACKTRACE_DUMP_ONCE(task) ({  \
+   static bool __section(.data.unlikely) __dumped; \
+   \
+   if (!__dumped) {\
+   __dumped = true;\
+   WARN_ON(1); \
+   show_stack(task, NULL); \
+   }   \
+})
+
+static int __save_stack_trace_reliable(struct stack_trace *trace,
+  struct task_struct *task)
+{
+   struct unwind_state state;
+   struct pt_regs *regs;
+   unsigned long addr;
+
+   for (unwind_start(, task, NULL, NULL); !unwind_done();
+unwind_next_frame()) {
+
+   regs = unwind_get_entry_regs();
+   if (regs) {
+   /*
+* Kernel mode registers on the stack indicate an
+* in-kernel interrupt or exception (e.g., preemption
+*