LinuxLists.cc - [PATCH v3 0/2] kretprobe: produce sane stack traces

2018-11-01 08:36:53

Subject: [PATCH v3 0/2] kretprobe: produce sane stack traces

2018-11-01 08:38:32

Subject: [PATCH v3 2/2] trace: remove kretprobed checks

This is effectively a reversion of commit 76094a2cf46e ("ftrace:
distinguish kretprobe'd functions in trace logs"), as the checking of
kretprobe_trampoline *for tracing* is no longer necessary with the new
kretprobe stack trace changes.

Signed-off-by: Aleksa Sarai <[email protected]>
---
kernel/trace/trace_output.c | 34 ++++------------------------------
1 file changed, 4 insertions(+), 30 deletions(-)

diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 6e6cc64faa38..951de16bd4fd 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -321,36 +321,14 @@ int trace_output_call(struct trace_iterator *iter, char *name, char *fmt, ...)
}
EXPORT_SYMBOL_GPL(trace_output_call);

-#ifdef CONFIG_KRETPROBES
-static inline const char *kretprobed(const char *name)
-{
- static const char tramp_name[] = "kretprobe_trampoline";
- int size = sizeof(tramp_name);
-
- if (strncmp(tramp_name, name, size) == 0)
- return "[unknown/kretprobe'd]";
- return name;
-}
-#else
-static inline const char *kretprobed(const char *name)
-{
- return name;
-}
-#endif /* CONFIG_KRETPROBES */
-
static void
seq_print_sym_short(struct trace_seq *s, const char *fmt, unsigned long address)
{
char str[KSYM_SYMBOL_LEN];
#ifdef CONFIG_KALLSYMS
- const char *name;
-
kallsyms_lookup(address, NULL, NULL, NULL, str);
-
- name = kretprobed(str);
-
- if (name && strlen(name)) {
- trace_seq_printf(s, fmt, name);
+ if (strlen(str)) {
+ trace_seq_printf(s, fmt, str);
return;
}
#endif
@@ -364,13 +342,9 @@ seq_print_sym_offset(struct trace_seq *s, const char *fmt,
{
char str[KSYM_SYMBOL_LEN];
#ifdef CONFIG_KALLSYMS
- const char *name;
-
sprint_symbol(str, address);
- name = kretprobed(str);
-
- if (name && strlen(name)) {
- trace_seq_printf(s, fmt, name);
+ if (strlen(str)) {
+ trace_seq_printf(s, fmt, str);
return;
}
#endif
--
2.19.1

2018-11-01 08:39:32

by Aleksa Sarai

[permalink] [raw]

Subject: [PATCH v3 1/2] kretprobe: produce sane stack traces

Historically, kretprobe has always produced unusable stack traces
(kretprobe_trampoline is the only entry in most cases, because of the
funky stack pointer overwriting). This has caused quite a few annoyances
when using tracing to debug problems[1] -- since return values are only
available with kretprobes but stack traces were only usable for kprobes,
users had to probe both and then manually associate them.

With the advent of bpf_trace, users would have been able to do this
association in bpf, but this was less than ideal (because
bpf_get_stackid would still produce rubbish and programs that didn't
know better would get silly results). The main usecase for stack traces
(at least with bpf_trace) is for DTrace-style aggregation on stack
traces (both entry and exit). Therefore we cannot simply correct the
stack trace on exit -- we must stash away the stack trace and return the
entry stack trace when it is requested.

[1]: https://github.com/iovisor/bpftrace/issues/101

Cc: Brendan Gregg <[email protected]>
Cc: Christian Brauner <[email protected]>
Signed-off-by: Aleksa Sarai <[email protected]>
---
Documentation/kprobes.txt | 6 +-
include/linux/kprobes.h | 27 +++++
kernel/events/callchain.c | 8 +-
kernel/kprobes.c | 101 +++++++++++++++++-
kernel/trace/trace.c | 11 +-
.../test.d/kprobe/kretprobe_stacktrace.tc | 25 +++++
6 files changed, 173 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 10f4499e677c..1965585848f4 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -597,7 +597,11 @@ address with the trampoline's address, stack backtraces and calls
to __builtin_return_address() will typically yield the trampoline's
address instead of the real return address for kretprobed functions.
(As far as we can tell, __builtin_return_address() is used only
-for instrumentation and error reporting.)
+for instrumentation and error reporting.) However, since return probes
+are used extensively in tracing (where stack backtraces are useful),
+return probes will stash away the stack backtrace during function entry
+so that return probe handlers can use the entry backtrace instead of
+having a trace with just kretprobe_trampoline.

If the number of times a function is called does not match the number
of times it returns, registering a return probe on that function may
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index e909413e4e38..1a1629544e56 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -40,6 +40,8 @@
#include <linux/rcupdate.h>
#include <linux/mutex.h>
#include <linux/ftrace.h>
+#include <linux/stacktrace.h>
+#include <linux/perf_event.h>
#include <asm/kprobes.h>

#ifdef CONFIG_KPROBES
@@ -168,11 +170,18 @@ struct kretprobe {
raw_spinlock_t lock;
};

+#define KRETPROBE_TRACE_SIZE 127
+struct kretprobe_trace {
+ int nr_entries;
+ unsigned long entries[KRETPROBE_TRACE_SIZE];
+};
+
struct kretprobe_instance {
struct hlist_node hlist;
struct kretprobe *rp;
kprobe_opcode_t *ret_addr;
struct task_struct *task;
+ struct kretprobe_trace entry;
char data[0];
};

@@ -371,6 +380,12 @@ void unregister_kretprobe(struct kretprobe *rp);
int register_kretprobes(struct kretprobe **rps, int num);
void unregister_kretprobes(struct kretprobe **rps, int num);

+struct kretprobe_instance *current_kretprobe_instance(void);
+void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
+ struct stack_trace *trace);
+void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
+ struct perf_callchain_entry_ctx *ctx);
+
void kprobe_flush_task(struct task_struct *tk);
void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);

@@ -397,6 +412,18 @@ static inline struct kprobe *kprobe_running(void)
{
return NULL;
}
+static inline struct kretprobe_instance *current_kretprobe_instance(void)
+{
+ return NULL;
+}
+static inline void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
+ struct stack_trace *trace)
+{
+}
+static inline void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
+ struct perf_callchain_entry_ctx *ctx)
+{
+}
static inline int register_kprobe(struct kprobe *p)
{
return -ENOSYS;
diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index 24a77c34e9ad..98edcd8a6987 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -12,6 +12,7 @@
#include <linux/perf_event.h>
#include <linux/slab.h>
#include <linux/sched/task_stack.h>
+#include <linux/kprobes.h>

#include "internal.h"

@@ -197,9 +198,14 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user,
ctx.contexts_maxed = false;

if (kernel && !user_mode(regs)) {
+ struct kretprobe_instance *ri = current_kretprobe_instance();
+
if (add_mark)
perf_callchain_store_context(&ctx, PERF_CONTEXT_KERNEL);
- perf_callchain_kernel(&ctx, regs);
+ if (ri)
+ kretprobe_perf_callchain_kernel(ri, &ctx);
+ else
+ perf_callchain_kernel(&ctx, regs);
}

if (user) {
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 90e98e233647..fca3964d18cd 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1206,6 +1206,16 @@ __releases(hlist_lock)
}
NOKPROBE_SYMBOL(kretprobe_table_unlock);

+static bool kretprobe_hash_is_locked(struct task_struct *tsk)
+{
+ unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
+ raw_spinlock_t *hlist_lock;
+
+ hlist_lock = kretprobe_table_lock_ptr(hash);
+ return raw_spin_is_locked(hlist_lock);
+}
+NOKPROBE_SYMBOL(kretprobe_hash_is_locked);
+
/*
* This function is called from finish_task_switch when task tk becomes dead,
* so that we can recycle any function-return probe instances associated
@@ -1800,6 +1810,13 @@ unsigned long __weak arch_deref_entry_point(void *entry)
return (unsigned long)entry;
}

+static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs);
+
+static inline bool kprobe_is_retprobe(struct kprobe *kp)
+{
+ return kp->pre_handler == pre_handler_kretprobe;
+}
+
#ifdef CONFIG_KRETPROBES
/*
* This kprobe pre_handler is registered with every kretprobe. When probe
@@ -1826,6 +1843,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
hash = hash_ptr(current, KPROBE_HASH_BITS);
raw_spin_lock_irqsave(&rp->lock, flags);
if (!hlist_empty(&rp->free_instances)) {
+ struct stack_trace trace = {};
+
ri = hlist_entry(rp->free_instances.first,
struct kretprobe_instance, hlist);
hlist_del(&ri->hlist);
@@ -1834,6 +1853,11 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
ri->rp = rp;
ri->task = current;

+ trace.entries = &ri->entry.entries[0];
+ trace.max_entries = KRETPROBE_TRACE_SIZE;
+ save_stack_trace_regs(regs, &trace);
+ ri->entry.nr_entries = trace.nr_entries;
+
if (rp->entry_handler && rp->entry_handler(ri, regs)) {
raw_spin_lock_irqsave(&rp->lock, flags);
hlist_add_head(&ri->hlist, &rp->free_instances);
@@ -1856,6 +1880,65 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
}
NOKPROBE_SYMBOL(pre_handler_kretprobe);

+/*
+ * Return the kretprobe_instance associated with the current_kprobe. Calling
+ * this is only reasonable from within a kretprobe handler context (otherwise
+ * return NULL).
+ *
+ * Must be called within a kretprobe_hash_lock(current, ...) context.
+ */
+struct kretprobe_instance *current_kretprobe_instance(void)
+{
+ struct kprobe *kp;
+ struct kretprobe *rp;
+ struct kretprobe_instance *ri;
+ struct hlist_head *head;
+ unsigned long hash = hash_ptr(current, KPROBE_HASH_BITS);
+
+ kp = kprobe_running();
+ if (!kp || !kprobe_is_retprobe(kp))
+ return NULL;
+ if (WARN_ON(!kretprobe_hash_is_locked(current)))
+ return NULL;
+
+ rp = container_of(kp, struct kretprobe, kp);
+ head = &kretprobe_inst_table[hash];
+
+ hlist_for_each_entry(ri, head, hlist) {
+ if (ri->task == current && ri->rp == rp)
+ return ri;
+ }
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(current_kretprobe_instance);
+NOKPROBE_SYMBOL(current_kretprobe_instance);
+
+void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
+ struct stack_trace *trace)
+{
+ int i;
+ struct kretprobe_trace *krt = &ri->entry;
+
+ for (i = trace->skip; i < krt->nr_entries; i++) {
+ if (trace->nr_entries >= trace->max_entries)
+ break;
+ trace->entries[trace->nr_entries++] = krt->entries[i];
+ }
+}
+
+void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
+ struct perf_callchain_entry_ctx *ctx)
+{
+ int i;
+ struct kretprobe_trace *krt = &ri->entry;
+
+ for (i = 0; i < krt->nr_entries; i++) {
+ if (krt->entries[i] == ULONG_MAX)
+ break;
+ perf_callchain_store(ctx, (u64) krt->entries[i]);
+ }
+}
+
bool __weak arch_kprobe_on_func_entry(unsigned long offset)
{
return !offset;
@@ -2005,6 +2088,22 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
}
NOKPROBE_SYMBOL(pre_handler_kretprobe);

+struct kretprobe_instance *current_kretprobe_instance(void)
+{
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(current_kretprobe_instance);
+NOKPROBE_SYMBOL(current_kretprobe_instance);
+
+void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
+ struct stack_trace *trace)
+{
+}
+
+void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
+ struct perf_callchain_entry_ctx *ctx)
+{
+}
#endif /* CONFIG_KRETPROBES */

/* Set the kprobe gone and remove its instruction buffer. */
@@ -2241,7 +2340,7 @@ static void report_probe(struct seq_file *pi, struct kprobe *p,
char *kprobe_type;
void *addr = p->addr;

- if (p->pre_handler == pre_handler_kretprobe)
+ if (kprobe_is_retprobe(p))
kprobe_type = "r";
else
kprobe_type = "k";
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index bf6f1d70484d..2210d38a4dbf 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -42,6 +42,7 @@
#include <linux/nmi.h>
#include <linux/fs.h>
#include <linux/trace.h>
+#include <linux/kprobes.h>
#include <linux/sched/clock.h>
#include <linux/sched/rt.h>

@@ -2590,6 +2591,7 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
struct ring_buffer_event *event;
struct stack_entry *entry;
struct stack_trace trace;
+ struct kretprobe_instance *ri = current_kretprobe_instance();
int use_stack;
int size = FTRACE_STACK_ENTRIES;

@@ -2626,7 +2628,9 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
trace.entries = this_cpu_ptr(ftrace_stack.calls);
trace.max_entries = FTRACE_STACK_MAX_ENTRIES;

- if (regs)
+ if (ri)
+ kretprobe_save_stack_trace(ri, &trace);
+ else if (regs)
save_stack_trace_regs(regs, &trace);
else
save_stack_trace(&trace);
@@ -2653,7 +2657,10 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
else {
trace.max_entries = FTRACE_STACK_ENTRIES;
trace.entries = entry->caller;
- if (regs)
+
+ if (ri)
+ kretprobe_save_stack_trace(ri, &trace);
+ else if (regs)
save_stack_trace_regs(regs, &trace);
else
save_stack_trace(&trace);
diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
new file mode 100644
index 000000000000..03146c6a1a3c
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
@@ -0,0 +1,25 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0+
+# description: Kretprobe dynamic event with a stacktrace
+
+[ -f kprobe_events ] || exit_unsupported # this is configurable
+
+echo 0 > events/enable
+echo 1 > options/stacktrace
+
+echo 'r:teststackprobe sched_fork $retval' > kprobe_events
+grep teststackprobe kprobe_events
+test -d events/kprobes/teststackprobe
+
+clear_trace
+echo 1 > events/kprobes/teststackprobe/enable
+( echo "forked")
+echo 0 > events/kprobes/teststackprobe/enable
+
+# Make sure we don't see kretprobe_trampoline and we see _do_fork.
+! grep 'kretprobe' trace
+grep '_do_fork' trace
+
+echo '-:teststackprobe' >> kprobe_events
+clear_trace
+test -d events/kprobes/teststackprobe && exit_fail || exit_pass
--
2.19.1

2018-11-01 15:23:10

by Masami Hiramatsu

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] kretprobe: produce sane stack traces

Hi Aleksa,

On Thu, 1 Nov 2018 19:35:50 +1100
Aleksa Sarai <[email protected]> wrote:
[...]
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc

Please split the test case as an independent patch.

> new file mode 100644
> index 000000000000..03146c6a1a3c
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
> +# description: Kretprobe dynamic event with a stacktrace
> +
> +[ -f kprobe_events ] || exit_unsupported # this is configurable
> +
> +echo 0 > events/enable
> +echo 1 > options/stacktrace
> +
> +echo 'r:teststackprobe sched_fork $retval' > kprobe_events
> +grep teststackprobe kprobe_events
> +test -d events/kprobes/teststackprobe

Hmm, what happen if we have 2 or more kretprobes on same stack?
It seems you just save stack in pre_handler, but that stack can already
includes another kretprobe's trampline address...

Thank you,

> +
> +clear_trace
> +echo 1 > events/kprobes/teststackprobe/enable
> +( echo "forked")
> +echo 0 > events/kprobes/teststackprobe/enable
> +
> +# Make sure we don't see kretprobe_trampoline and we see _do_fork.
> +! grep 'kretprobe' trace
> +grep '_do_fork' trace
> +
> +echo '-:teststackprobe' >> kprobe_events
> +clear_trace
> +test -d events/kprobes/teststackprobe && exit_fail || exit_pass
> --
> 2.19.1
>

--
Masami Hiramatsu <[email protected]>

2018-11-01 21:14:45

by Aleksa Sarai

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] kretprobe: produce sane stack traces

On 2018-11-02, Masami Hiramatsu <[email protected]> wrote:
> Please split the test case as an independent patch.

Will do. Should the Documentation/ change also be a separate patch?

> > new file mode 100644
> > index 000000000000..03146c6a1a3c
> > --- /dev/null
> > +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
> > @@ -0,0 +1,25 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# description: Kretprobe dynamic event with a stacktrace
> > +
> > +[ -f kprobe_events ] || exit_unsupported # this is configurable
> > +
> > +echo 0 > events/enable
> > +echo 1 > options/stacktrace
> > +
> > +echo 'r:teststackprobe sched_fork $retval' > kprobe_events
> > +grep teststackprobe kprobe_events
> > +test -d events/kprobes/teststackprobe
>
> Hmm, what happen if we have 2 or more kretprobes on same stack?
> It seems you just save stack in pre_handler, but that stack can already
> includes another kretprobe's trampline address...

Yeah, you're quite right...

My first instinct was to do something awful like iterate over the set of
"kretprobe_instance"s with ->task == current, and then correct
kretprobe_trampoline entries using ->ret_addr. (I think this would be
correct because each task can only be in one context at once, and in
order to get to a particular kretprobe all of your caller kretprobes
were inserted before you and any sibling or later kretprobe_instances
will have been removed. But I might be insanely wrong on this front.)

However (as I noted in the other thread), there is a problem where
kretprobe_trampoline actually stops the unwinder in its tracks and thus
you only get the first kretprobe_trampoline. This is something I'm going
to look into some more (despite not having made progress on it last
time) since now it's something that actually needs to be fixed (and
as I mentioned in the other thread, show_stack() actually works on x86
in this context unlike the other stack_trace users).

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (2.06 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-02 00:48:05

by Steven Rostedt

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] kretprobe: produce sane stack traces

On Thu, 1 Nov 2018 19:35:50 +1100
Aleksa Sarai <[email protected]> wrote:

> Historically, kretprobe has always produced unusable stack traces
> (kretprobe_trampoline is the only entry in most cases, because of the
> funky stack pointer overwriting). This has caused quite a few annoyances
> when using tracing to debug problems[1] -- since return values are only
> available with kretprobes but stack traces were only usable for kprobes,
> users had to probe both and then manually associate them.
>
> With the advent of bpf_trace, users would have been able to do this
> association in bpf, but this was less than ideal (because
> bpf_get_stackid would still produce rubbish and programs that didn't
> know better would get silly results). The main usecase for stack traces
> (at least with bpf_trace) is for DTrace-style aggregation on stack
> traces (both entry and exit). Therefore we cannot simply correct the
> stack trace on exit -- we must stash away the stack trace and return the
> entry stack trace when it is requested.
>
> [1]: https://github.com/iovisor/bpftrace/issues/101
>
> Cc: Brendan Gregg <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Signed-off-by: Aleksa Sarai <[email protected]>
> ---
> Documentation/kprobes.txt | 6 +-
> include/linux/kprobes.h | 27 +++++
> kernel/events/callchain.c | 8 +-
> kernel/kprobes.c | 101 +++++++++++++++++-
> kernel/trace/trace.c | 11 +-
> .../test.d/kprobe/kretprobe_stacktrace.tc | 25 +++++
> 6 files changed, 173 insertions(+), 5 deletions(-)
> create mode 100644 tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
>
> diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
> index 10f4499e677c..1965585848f4 100644
> --- a/Documentation/kprobes.txt
> +++ b/Documentation/kprobes.txt
> @@ -597,7 +597,11 @@ address with the trampoline's address, stack backtraces and calls
> to __builtin_return_address() will typically yield the trampoline's
> address instead of the real return address for kretprobed functions.
> (As far as we can tell, __builtin_return_address() is used only
> -for instrumentation and error reporting.)
> +for instrumentation and error reporting.) However, since return probes
> +are used extensively in tracing (where stack backtraces are useful),
> +return probes will stash away the stack backtrace during function entry
> +so that return probe handlers can use the entry backtrace instead of
> +having a trace with just kretprobe_trampoline.
>
> If the number of times a function is called does not match the number
> of times it returns, registering a return probe on that function may
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index e909413e4e38..1a1629544e56 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -40,6 +40,8 @@
> #include <linux/rcupdate.h>
> #include <linux/mutex.h>
> #include <linux/ftrace.h>
> +#include <linux/stacktrace.h>
> +#include <linux/perf_event.h>
> #include <asm/kprobes.h>
>
> #ifdef CONFIG_KPROBES
> @@ -168,11 +170,18 @@ struct kretprobe {
> raw_spinlock_t lock;
> };
>
> +#define KRETPROBE_TRACE_SIZE 127
> +struct kretprobe_trace {
> + int nr_entries;
> + unsigned long entries[KRETPROBE_TRACE_SIZE];
> +};
> +
> struct kretprobe_instance {
> struct hlist_node hlist;
> struct kretprobe *rp;
> kprobe_opcode_t *ret_addr;
> struct task_struct *task;
> + struct kretprobe_trace entry;
> char data[0];
> };
>
> @@ -371,6 +380,12 @@ void unregister_kretprobe(struct kretprobe *rp);
> int register_kretprobes(struct kretprobe **rps, int num);
> void unregister_kretprobes(struct kretprobe **rps, int num);
>
> +struct kretprobe_instance *current_kretprobe_instance(void);
> +void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace);
> +void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx);
> +
> void kprobe_flush_task(struct task_struct *tk);
> void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>
> @@ -397,6 +412,18 @@ static inline struct kprobe *kprobe_running(void)
> {
> return NULL;
> }
> +static inline struct kretprobe_instance *current_kretprobe_instance(void)
> +{
> + return NULL;
> +}
> +static inline void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace)
> +{
> +}
> +static inline void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx)
> +{
> +}
> static inline int register_kprobe(struct kprobe *p)
> {
> return -ENOSYS;
> diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
> index 24a77c34e9ad..98edcd8a6987 100644
> --- a/kernel/events/callchain.c
> +++ b/kernel/events/callchain.c
> @@ -12,6 +12,7 @@
> #include <linux/perf_event.h>
> #include <linux/slab.h>
> #include <linux/sched/task_stack.h>
> +#include <linux/kprobes.h>
>
> #include "internal.h"
>
> @@ -197,9 +198,14 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user,
> ctx.contexts_maxed = false;
>
> if (kernel && !user_mode(regs)) {
> + struct kretprobe_instance *ri = current_kretprobe_instance();
> +
> if (add_mark)
> perf_callchain_store_context(&ctx, PERF_CONTEXT_KERNEL);
> - perf_callchain_kernel(&ctx, regs);
> + if (ri)
> + kretprobe_perf_callchain_kernel(ri, &ctx);
> + else
> + perf_callchain_kernel(&ctx, regs);
> }
>
> if (user) {
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 90e98e233647..fca3964d18cd 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1206,6 +1206,16 @@ __releases(hlist_lock)
> }
> NOKPROBE_SYMBOL(kretprobe_table_unlock);
>
> +static bool kretprobe_hash_is_locked(struct task_struct *tsk)
> +{
> + unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> + raw_spinlock_t *hlist_lock;
> +
> + hlist_lock = kretprobe_table_lock_ptr(hash);
> + return raw_spin_is_locked(hlist_lock);
> +}
> +NOKPROBE_SYMBOL(kretprobe_hash_is_locked);
> +
> /*
> * This function is called from finish_task_switch when task tk becomes dead,
> * so that we can recycle any function-return probe instances associated
> @@ -1800,6 +1810,13 @@ unsigned long __weak arch_deref_entry_point(void *entry)
> return (unsigned long)entry;
> }
>
> +static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs);
> +
> +static inline bool kprobe_is_retprobe(struct kprobe *kp)
> +{
> + return kp->pre_handler == pre_handler_kretprobe;
> +}
> +
> #ifdef CONFIG_KRETPROBES
> /*
> * This kprobe pre_handler is registered with every kretprobe. When probe
> @@ -1826,6 +1843,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> hash = hash_ptr(current, KPROBE_HASH_BITS);
> raw_spin_lock_irqsave(&rp->lock, flags);
> if (!hlist_empty(&rp->free_instances)) {
> + struct stack_trace trace = {};
> +
> ri = hlist_entry(rp->free_instances.first,
> struct kretprobe_instance, hlist);
> hlist_del(&ri->hlist);
> @@ -1834,6 +1853,11 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> ri->rp = rp;
> ri->task = current;
>
> + trace.entries = &ri->entry.entries[0];
> + trace.max_entries = KRETPROBE_TRACE_SIZE;
> + save_stack_trace_regs(regs, &trace);
> + ri->entry.nr_entries = trace.nr_entries;
> +

So basically your saving a copy of the stack trace for each probe, for
something that may not ever be used?

This adds quite a lot of overhead for something not used often.

I think the real answer is to fix kretprobes (and I just checked, the
return call of function graph tracer stack trace doesn't go past the
return_to_handler function either. It's just that a stack trace was
never done on the return side).

The more I look at this patch, the less I like it.

-- Steve

> if (rp->entry_handler && rp->entry_handler(ri, regs)) {
> raw_spin_lock_irqsave(&rp->lock, flags);
> hlist_add_head(&ri->hlist, &rp->free_instances);
> @@ -1856,6 +1880,65 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +/*
> + * Return the kretprobe_instance associated with the current_kprobe. Calling
> + * this is only reasonable from within a kretprobe handler context (otherwise
> + * return NULL).
> + *
> + * Must be called within a kretprobe_hash_lock(current, ...) context.
> + */
> +struct kretprobe_instance *current_kretprobe_instance(void)
> +{
> + struct kprobe *kp;
> + struct kretprobe *rp;
> + struct kretprobe_instance *ri;
> + struct hlist_head *head;
> + unsigned long hash = hash_ptr(current, KPROBE_HASH_BITS);
> +
> + kp = kprobe_running();
> + if (!kp || !kprobe_is_retprobe(kp))
> + return NULL;
> + if (WARN_ON(!kretprobe_hash_is_locked(current)))
> + return NULL;
> +
> + rp = container_of(kp, struct kretprobe, kp);
> + head = &kretprobe_inst_table[hash];
> +
> + hlist_for_each_entry(ri, head, hlist) {
> + if (ri->task == current && ri->rp == rp)
> + return ri;
> + }
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(current_kretprobe_instance);
> +NOKPROBE_SYMBOL(current_kretprobe_instance);
> +
> +void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace)
> +{
> + int i;
> + struct kretprobe_trace *krt = &ri->entry;
> +
> + for (i = trace->skip; i < krt->nr_entries; i++) {
> + if (trace->nr_entries >= trace->max_entries)
> + break;
> + trace->entries[trace->nr_entries++] = krt->entries[i];
> + }
> +}
> +
> +void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx)
> +{
> + int i;
> + struct kretprobe_trace *krt = &ri->entry;
> +
> + for (i = 0; i < krt->nr_entries; i++) {
> + if (krt->entries[i] == ULONG_MAX)
> + break;
> + perf_callchain_store(ctx, (u64) krt->entries[i]);
> + }
> +}
> +
> bool __weak arch_kprobe_on_func_entry(unsigned long offset)
> {
> return !offset;
> @@ -2005,6 +2088,22 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> }
> NOKPROBE_SYMBOL(pre_handler_kretprobe);
>
> +struct kretprobe_instance *current_kretprobe_instance(void)
> +{
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(current_kretprobe_instance);
> +NOKPROBE_SYMBOL(current_kretprobe_instance);
> +
> +void kretprobe_save_stack_trace(struct kretprobe_instance *ri,
> + struct stack_trace *trace)
> +{
> +}
> +
> +void kretprobe_perf_callchain_kernel(struct kretprobe_instance *ri,
> + struct perf_callchain_entry_ctx *ctx)
> +{
> +}
> #endif /* CONFIG_KRETPROBES */
>
> /* Set the kprobe gone and remove its instruction buffer. */
> @@ -2241,7 +2340,7 @@ static void report_probe(struct seq_file *pi, struct kprobe *p,
> char *kprobe_type;
> void *addr = p->addr;
>
> - if (p->pre_handler == pre_handler_kretprobe)
> + if (kprobe_is_retprobe(p))
> kprobe_type = "r";
> else
> kprobe_type = "k";
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index bf6f1d70484d..2210d38a4dbf 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -42,6 +42,7 @@
> #include <linux/nmi.h>
> #include <linux/fs.h>
> #include <linux/trace.h>
> +#include <linux/kprobes.h>
> #include <linux/sched/clock.h>
> #include <linux/sched/rt.h>
>
> @@ -2590,6 +2591,7 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
> struct ring_buffer_event *event;
> struct stack_entry *entry;
> struct stack_trace trace;
> + struct kretprobe_instance *ri = current_kretprobe_instance();
> int use_stack;
> int size = FTRACE_STACK_ENTRIES;
>
> @@ -2626,7 +2628,9 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
> trace.entries = this_cpu_ptr(ftrace_stack.calls);
> trace.max_entries = FTRACE_STACK_MAX_ENTRIES;
>
> - if (regs)
> + if (ri)
> + kretprobe_save_stack_trace(ri, &trace);
> + else if (regs)
> save_stack_trace_regs(regs, &trace);
> else
> save_stack_trace(&trace);
> @@ -2653,7 +2657,10 @@ static void __ftrace_trace_stack(struct ring_buffer *buffer,
> else {
> trace.max_entries = FTRACE_STACK_ENTRIES;
> trace.entries = entry->caller;
> - if (regs)
> +
> + if (ri)
> + kretprobe_save_stack_trace(ri, &trace);
> + else if (regs)
> save_stack_trace_regs(regs, &trace);
> else
> save_stack_trace(&trace);
> diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
> new file mode 100644
> index 000000000000..03146c6a1a3c
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/kprobe/kretprobe_stacktrace.tc
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
> +# description: Kretprobe dynamic event with a stacktrace
> +
> +[ -f kprobe_events ] || exit_unsupported # this is configurable
> +
> +echo 0 > events/enable
> +echo 1 > options/stacktrace
> +
> +echo 'r:teststackprobe sched_fork $retval' > kprobe_events
> +grep teststackprobe kprobe_events
> +test -d events/kprobes/teststackprobe
> +
> +clear_trace
> +echo 1 > events/kprobes/teststackprobe/enable
> +( echo "forked")
> +echo 0 > events/kprobes/teststackprobe/enable
> +
> +# Make sure we don't see kretprobe_trampoline and we see _do_fork.
> +! grep 'kretprobe' trace
> +grep '_do_fork' trace
> +
> +echo '-:teststackprobe' >> kprobe_events
> +clear_trace
> +test -d events/kprobes/teststackprobe && exit_fail || exit_pass

2018-11-02 03:05:21

by Masami Hiramatsu

On Fri, 2 Nov 2018 17:59:32 +1100
Aleksa Sarai <[email protected]> wrote:

> As an aside, I just tested with the frame unwinder and it isn't thrown
> off-course by kretprobe_trampoline (though obviously the stack is still
> wrong). So I think we just need to hook into the ORC unwinder to get it
> to continue skipping up the stack, as well as add the rewriting code for
> the stack traces (for all unwinders I guess -- though ideally we should

I agree that this is the right solution.

> do this without having to add the same code to every architecture).

True, and there's an art to consolidating the code between
architectures.

I'm currently looking at function graph and seeing if I can consolidate
it too. And I'm also trying to get multiple uses to hook into its
infrastructure. I think I finally figured out a way to do so.

The reason it is difficult, is that you need to maintain state between
the entry of a function and the exit for each task and callback that is
registered. Hence, it's a 3x tuple (function stack, task, callbacks).
And this must be maintained with preemption. A task may sleep for
minutes, and the state needs to be retained.

The only state that must be retained is the function stack with the
task, because if that gets out of sync, the system crashes. But the
callback state can be removed.

Here's what is there now:

When something is registered with the function graph tracer, every
task gets a shadowed stack. A hook is added to fork to add shadow
stacks to new tasks. Once a shadow stack is added to a task, that
shadow stack is never removed until the task exits.

When the function is entered, the real return code is stored in the
shadow stack and the trampoline address is put in its place.

On return, the trampoline is called, and it will pop off the return
code from the shadow stack and return to that.

The issue with multiple users, is that different users may want to
trace different functions. On entry, the user could say it doesn't want
to trace the current function, and the return part must not be called
on exit. Keeping track of which user needs the return called is the
tricky part.

Here's what I plan on implementing:

Along with a shadow stack, I was going to add a 4096 byte (one page)
array that holds 64 8 byte masks to every task as well. This will allow
64 simultaneous users (which is rather extreme). If we need to support
more, we could allocate another page for all tasks. The 8 byte mask
will represent each depth (allowing to do this for 64 function call
stack depth, which should also be enough).

Each user will be assigned one of the masks. Each bit in the mask
represents the depth of the shadow stack. When a function is called,
each user registered with the function graph tracer will get called
(if they asked to be called for this function, via the ftrace_ops
hashes) and if they want to trace the function, then the bit is set in
the mask for that stack depth.

When the function exits the function and we pop off the return code
from the shadow stack, we then look at all the bits set for the
corresponding users, and call their return callbacks, and ignore
anything that is not set.

When a user is unregistered, it the corresponding bits that represent
it are cleared, and it the return callback will not be called. But the
tasks being traced will still have their shadow stack to allow it to
get back to normal.

I'll hopefully have a prototype ready by plumbers.

And this too will require each architecture to probably change. As a
side project to this, I'm going to try to consolidate the function
graph code among all the architectures as well. Not an easy task.

-- Steve

2018-11-02 15:44:11

by Josh Poimboeuf

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] kretprobe: produce sane stack traces

On Fri, Nov 02, 2018 at 09:16:58AM -0400, Steven Rostedt wrote:
> On Fri, 2 Nov 2018 17:59:32 +1100
> Aleksa Sarai <[email protected]> wrote:
>
> > As an aside, I just tested with the frame unwinder and it isn't thrown
> > off-course by kretprobe_trampoline (though obviously the stack is still
> > wrong). So I think we just need to hook into the ORC unwinder to get it
> > to continue skipping up the stack, as well as add the rewriting code for
> > the stack traces (for all unwinders I guess -- though ideally we should
>
> I agree that this is the right solution.

Sounds good to me.

However, it would be *really* nice if function graph and kretprobes
shared the same infrastructure, like they do for function entry.
There's a lot of duplicated effort there.

> > do this without having to add the same code to every architecture).
>
> True, and there's an art to consolidating the code between
> architectures.
>
> I'm currently looking at function graph and seeing if I can consolidate
> it too. And I'm also trying to get multiple uses to hook into its
> infrastructure. I think I finally figured out a way to do so.
>
> The reason it is difficult, is that you need to maintain state between
> the entry of a function and the exit for each task and callback that is
> registered. Hence, it's a 3x tuple (function stack, task, callbacks).
> And this must be maintained with preemption. A task may sleep for
> minutes, and the state needs to be retained.
>
> The only state that must be retained is the function stack with the
> task, because if that gets out of sync, the system crashes. But the
> callback state can be removed.
>
> Here's what is there now:
>
> When something is registered with the function graph tracer, every
> task gets a shadowed stack. A hook is added to fork to add shadow
> stacks to new tasks. Once a shadow stack is added to a task, that
> shadow stack is never removed until the task exits.
>
> When the function is entered, the real return code is stored in the
> shadow stack and the trampoline address is put in its place.
>
> On return, the trampoline is called, and it will pop off the return
> code from the shadow stack and return to that.
>
> The issue with multiple users, is that different users may want to
> trace different functions. On entry, the user could say it doesn't want
> to trace the current function, and the return part must not be called
> on exit. Keeping track of which user needs the return called is the
> tricky part.
>
> Here's what I plan on implementing:
>
> Along with a shadow stack, I was going to add a 4096 byte (one page)
> array that holds 64 8 byte masks to every task as well. This will allow
> 64 simultaneous users (which is rather extreme). If we need to support
> more, we could allocate another page for all tasks. The 8 byte mask
> will represent each depth (allowing to do this for 64 function call
> stack depth, which should also be enough).
>
> Each user will be assigned one of the masks. Each bit in the mask
> represents the depth of the shadow stack. When a function is called,
> each user registered with the function graph tracer will get called
> (if they asked to be called for this function, via the ftrace_ops
> hashes) and if they want to trace the function, then the bit is set in
> the mask for that stack depth.
>
> When the function exits the function and we pop off the return code
> from the shadow stack, we then look at all the bits set for the
> corresponding users, and call their return callbacks, and ignore
> anything that is not set.
>
>
> When a user is unregistered, it the corresponding bits that represent
> it are cleared, and it the return callback will not be called. But the
> tasks being traced will still have their shadow stack to allow it to
> get back to normal.
>
> I'll hopefully have a prototype ready by plumbers.

Why do we need multiple users? It would be a lot simpler if we could
just enforce a single user per fgraphed/kretprobed function (and return
-EBUSY if it's already being traced/probed).

> And this too will require each architecture to probably change. As a
> side project to this, I'm going to try to consolidate the function
> graph code among all the architectures as well. Not an easy task.

Do you mean implementing HAVE_FUNCTION_GRAPH_RET_ADDR_PTR for all the
arches? If so, I think have an old crusty patch which attempted to
that. I could try to dig it up if you're interested.

--
Josh

2018-11-02 16:13:57

by Steven Rostedt

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] kretprobe: produce sane stack traces

On Fri, 2 Nov 2018 10:43:26 -0500
Josh Poimboeuf <[email protected]> wrote:

> > I'll hopefully have a prototype ready by plumbers.
>
> Why do we need multiple users? It would be a lot simpler if we could
> just enforce a single user per fgraphed/kretprobed function (and return
> -EBUSY if it's already being traced/probed).

Because that means if function graph tracer is active, then you can't
do a kretprobe, and vice versa. I'd really like to have it working for
multiple users, then we could trace different graph functions and store
them in different buffers. It would also allow for perf to use function
graph tracer too.

>
> > And this too will require each architecture to probably change. As a
> > side project to this, I'm going to try to consolidate the function
> > graph code among all the architectures as well. Not an easy task.
>
> Do you mean implementing HAVE_FUNCTION_GRAPH_RET_ADDR_PTR for all the
> arches? If so, I think have an old crusty patch which attempted to
> that. I could try to dig it up if you're interested.
>

I'd like to have that, but it still requires some work. But I'd just
the truly architecture dependent code be in the architecture (basically
the asm code), and have the ability to move most of the duplicate code
out of the archs.

-- Steve

2018-11-03 07:03:53

On 2018-11-03, Aleksa Sarai <[email protected]> wrote:
> This is actually a general bug in ftrace as well, because (for
> instance) while the unwinder calls into ftrace_graph_ret_addr() while
> walking up the stack this isn't used to correct regs->ip in
> perf_callchain_kernel(). I think this is the cause of the bug in
> ftrace-graph (if you switch to the "frame" unwinder you might be able
> to see it more clearly) -- but when trying to fix it I'm having
> trouble figuring out what retp should be (stack_addr(regs) doesn't
> give the right result for some reason).

@Steven, @Masami: Can you verify whether this is an actual bug? The
snippet is like (arch/x86/events/core.c):

void
perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs)
{
/* ... */
if (perf_callchain_store(entry, regs->ip))
return;
/* rest of unwind code */
}

And it seems to me that there's an ftrace_graph_ret_addr missing here
(though I was having trouble figuring out what the right retp would be
in this context -- both stack_addr(regs) and &regs->sp appear to be
wrong when I was trying to get this to work with kretprobes -- while my
patch worked with all other kretprobe_trampoline cases in the unwinder).

The same issue is present in __save_stack_trace
(arch/x86/kernel/stacktrace.c). This is likely the only reason that --
as Steven said -- stacktraces wouldn't work with ftrace-graph (and thus
with the refactor both of you are discussing).

Thanks.

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (1.58 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-06 22:17:26

by Steven Rostedt

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] kretprobe: produce sane stack traces

On Sun, 4 Nov 2018 22:59:13 +1100
Aleksa Sarai <[email protected]> wrote:

> The same issue is present in __save_stack_trace
> (arch/x86/kernel/stacktrace.c). This is likely the only reason that --
> as Steven said -- stacktraces wouldn't work with ftrace-graph (and thus
> with the refactor both of you are discussing).

By the way, I was playing with the the orc unwinder and stack traces
from the function graph tracer return code, and got it working with the
below patch. Caution, that patch also has a stack trace hardcoded in
the return path of the function graph tracer, so you don't want to run
function graph tracing without filtering.

You can apply the patch and do:

# cd /sys/kernel/debug/tracing
# echo schedule > set_ftrace_filter
# echo function_graph > current_tracer
# cat trace

3) | schedule() {
rcu_preempt-10 [003] .... 91.160297: <stack trace>
=> ftrace_return_to_handler
=> return_to_handler
=> schedule_timeout
=> rcu_gp_kthread
=> kthread
=> ret_from_fork
3) # 4009.085 us | }
3) | schedule() {
kworker/1:0-17 [001] .... 91.163288: <stack trace>
=> ftrace_return_to_handler
=> return_to_handler
=> worker_thread
=> kthread
=> ret_from_fork
1) # 7000.070 us | }
1) | schedule() {
rcu_preempt-10 [003] .... 91.164311: <stack trace>
=> ftrace_return_to_handler
=> return_to_handler
=> schedule_timeout
=> rcu_gp_kthread
=> kthread
=> ret_from_fork
3) # 4006.540 us | }

Where just adding the stack trace without the other code, these traces
ended at "return_to_handler".

This patch is not for inclusion, it was just a test to see how to make
this work.

-- Steve

diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 91b2cff4b79a..4bcd646ae1f4 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -320,13 +320,15 @@ ENTRY(ftrace_graph_caller)
ENDPROC(ftrace_graph_caller)

ENTRY(return_to_handler)
- UNWIND_HINT_EMPTY
- subq $24, %rsp
+ subq $8, %rsp
+ UNWIND_HINT_FUNC
+ subq $16, %rsp

/* Save the return values */
movq %rax, (%rsp)
movq %rdx, 8(%rsp)
movq %rbp, %rdi
+ leaq 16(%rsp), %rsi

call ftrace_return_to_handler

diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 169b3c44ee97..aaeca73218cc 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -242,13 +242,16 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret,
trace->calltime = current->ret_stack[index].calltime;
trace->overrun = atomic_read(&current->trace_overrun);
trace->depth = index;
+
+ trace_dump_stack(0);
}

/*
* Send the trace to the ring-buffer.
* @return the original return address.
*/
-unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
+unsigned long ftrace_return_to_handler(unsigned long frame_pointer,
+ unsigned long *ptr)
{
struct ftrace_graph_ret trace;
unsigned long ret;
@@ -257,6 +260,8 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)
trace.rettime = trace_clock_local();
barrier();
current->curr_ret_stack--;
+ *ptr = ret;
+
/*
* The curr_ret_stack can be less than -1 only if it was
* filtered out and it's about to return from the function.

2018-11-08 07:48:26

by Aleksa Sarai

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] kretprobe: produce sane stack traces

On 2018-11-06, Steven Rostedt <[email protected]> wrote:
> On Sun, 4 Nov 2018 22:59:13 +1100
> Aleksa Sarai <[email protected]> wrote:
>
> > The same issue is present in __save_stack_trace
> > (arch/x86/kernel/stacktrace.c). This is likely the only reason that --
> > as Steven said -- stacktraces wouldn't work with ftrace-graph (and thus
> > with the refactor both of you are discussing).
>
> By the way, I was playing with the the orc unwinder and stack traces
> from the function graph tracer return code, and got it working with the
> below patch. Caution, that patch also has a stack trace hardcoded in
> the return path of the function graph tracer, so you don't want to run
> function graph tracing without filtering.

Neat!

> diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> index 169b3c44ee97..aaeca73218cc 100644
> --- a/kernel/trace/trace_functions_graph.c
> +++ b/kernel/trace/trace_functions_graph.c
> @@ -242,13 +242,16 @@ ftrace_pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret,
> trace->calltime = current->ret_stack[index].calltime;
> trace->overrun = atomic_read(&current->trace_overrun);
> trace->depth = index;
> +
> + trace_dump_stack(0);

Right, this works because save_stack is not being passed a pt_regs. But if
you pass a pt_regs (as happens with bpf_getstackid -- which is what
spawned this discussion) then the top-most entry of the stack will still
be a trampoline because there is no ftrace_graph_ret_addr call.

(I'm struggling with how to fix this -- I can't figure out what retp
should be if you have a pt_regs. ->sp doesn't appear to work -- it's off
by a few bytes.)

I will attach what I have at the moment to hopefully explain what the
issue I've found is (re-using the kretprobe architecture but with the
shadow-stack idea).

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (1.93 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-08 08:05:51

On 2018-11-09, Masami Hiramatsu <[email protected]> wrote:
> > diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> > index ee696efec99f..c4dfafd43e11 100644
> > --- a/arch/x86/include/asm/ptrace.h
> > +++ b/arch/x86/include/asm/ptrace.h
> > @@ -172,6 +172,7 @@ static inline unsigned long kernel_stack_pointer(struct pt_regs *regs)
> > return regs->sp;
> > }
> > #endif
> > +#define stack_addr(regs) ((unsigned long *) kernel_stack_pointer(regs))
>
> No, you should use kernel_stack_pointer(regs) itself instead of stack_addr().
>
> >
> > #define GET_IP(regs) ((regs)->ip)
> > #define GET_FP(regs) ((regs)->bp)
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index b0d1e81c96bb..eb4da885020c 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -69,8 +69,6 @@
> > DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> > DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> >
> > -#define stack_addr(regs) ((unsigned long *)kernel_stack_pointer(regs))
>
> I don't like keeping this meaningless macro... this should be replaced with generic
> kernel_stack_pointer() macro.

Sure. This patch was just an example -- I can remove stack_addr() all
over.

> > - if (regs)
> > - save_stack_address(trace, regs->ip, nosched);
> > + if (regs) {
> > + /* XXX: Currently broken -- stack_addr(regs) doesn't match entry. */
> > + addr = regs->ip;
>
> Since this part is for storing regs->ip as a top of call-stack, this
> seems correct code. Stack unwind will be done next block.

This comment was referring to the usage of stack_addr(). stack_addr()
doesn't give you the right result (it isn't the address of the return
address -- it's slightly wrong). This is the main issue I was having --
am I doing something wrong here?

> > + //addr = ftrace_graph_ret_addr(current, &state.graph_idx, addr, stack_addr(regs));
>
> so func graph return trampoline address will be shown only when unwinding stack entries.
> I mean func-graph tracer is not used as an event, so it never kicks stackdump.

Just to make sure I understand what you're saying -- func-graph trace
will never actually call __ftrace_stack_trace? Because if it does, then
this code will be necessary (and then I'm a bit confused why the
unwinder has func-graph trace code -- if stack traces are never taken
under func-graph then the code in the unwinder is not necessary)

My reason for commenting this out is because at this point "state" isn't
initialised and thus .graph_idx would not be correctly handled during
unwind (and it's the same reason I commented it out later).

> > + addr = kretprobe_ret_addr(current, addr, stack_addr(regs));
>
> But since kretprobe will be an event, which can kick the stackdump.
> BTW, from kretprobe, regs->ip should always be the trampoline handler,
> see arch/x86/kernel/kprobes/core.c:772 :-)
> So it must be fixed always.

Right, but kretprobe_ret_addr() is returning the *original* return
address (and we need to do an (addr == kretprobe_trampoline)). The
real problem is that stack_addr(regs) isn't the same as it is during
kretprobe setup (but kretprobe_ret_addr() works everywhere else).

> > @@ -1856,6 +1870,41 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> > }
> > NOKPROBE_SYMBOL(pre_handler_kretprobe);
> >
> > +unsigned long kretprobe_ret_addr(struct task_struct *tsk, unsigned long ret,
> > + unsigned long *retp)
> > +{
> > + struct kretprobe_instance *ri;
> > + unsigned long flags = 0;
> > + struct hlist_head *head;
> > + bool need_lock;
> > +
> > + if (likely(ret != (unsigned long) &kretprobe_trampoline))
> > + return ret;
> > +
> > + need_lock = !kretprobe_hash_is_locked(tsk);
> > + if (WARN_ON(need_lock))
> > + kretprobe_hash_lock(tsk, &head, &flags);
> > + else
> > + head = kretprobe_inst_table_head(tsk);
>
> This may not work unless this is called from the kretprobe handler context,
> since if we are out of kretprobe handler context, another CPU can lock the
> hash table and it can be detected by kretprobe_hash_is_locked();.

Yeah, I noticed this as well when writing it (but needed a quick impl
that I could test). I will fix this, thanks!

By is_kretprobe_handler_context() I imagine you are referring to
checking is_kretprobe(current_kprobe())?

> So, we should check we are in the kretprobe handler context if tsk == current,
> if not, we definately can lock the hash lock without any warning. This can
> be something like;
>
> if (is_kretprobe_handler_context()) {
> // kretprobe_hash_lock(current == tsk) has been locked by caller
> if (tsk != current && kretprobe_hash(tsk) != kretprobe_hash(current))
> // the hash of tsk and current can be same.
> need_lock = true;
> } else
> // we should take a lock for tsk.
> need_lock = true;

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (4.96 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-09 15:10:31

On 2018-11-11, Masami Hiramatsu <[email protected]> wrote:
> > > > + addr = kretprobe_ret_addr(current, addr, stack_addr(regs));
> > >
> > > But since kretprobe will be an event, which can kick the stackdump.
> > > BTW, from kretprobe, regs->ip should always be the trampoline handler,
> > > see arch/x86/kernel/kprobes/core.c:772 :-)
> > > So it must be fixed always.
> >
> > Right, but kretprobe_ret_addr() is returning the *original* return
> > address (and we need to do an (addr == kretprobe_trampoline)). The
> > real problem is that stack_addr(regs) isn't the same as it is during
> > kretprobe setup (but kretprobe_ret_addr() works everywhere else).
>
> I think stack_addr(regs) should be same when this is called from kretprobe
> handler context. Otherwise, yes, it is not same, but in that case, regs->ip
> is not kretprobe_trampoline too.

I figured it out.

It should be (regs->sp - 1) (just like it is inside the relevant
unwinder function for ORC). I now have a prototype which works under the
frame unwinder[*] -- however under ORC you can only see the top-most
function (the unwinder doesn't see the other function calls). I'm
playing with ORC hints with kretprobe_trampoline to try to improve
things but it's still a bit screwy.

[*]: However, I've noticed that the stack traces between the two traces
no longer match. On kprobe you get function_name+1, but on
kretprobe you get function_caller+foo. Obviously it's working but
the return address results in slightly different stack traces. This
means that stack trace aggregation between kprobe and kretprobe
won't work anymore -- at least not like it did in my original
patch. So I'm really not sure where to go from here.

I can send around another patchset to illustrate the problem if you like
(as well as show how the current unwinding code works).

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (1.94 kB)
signature.asc (849.00 B)
Download all attachments