[PATCH 0/3] perf: Support multiple stacks (v2)

[PATCH 0/3] perf: Support multiple stacks (v2)

[Arun Sharma]

If userspace has two or more stacks and we happen to take a sample when
the stack pointer is pointing to a higher stack, we might get truncated
callchains. This patch series tries to address that.

Changelog:

v2: do this for compat32 as well
v2: improve safety checks and reduce max callchain size

Arun Sharma (3):
  perf, x86: Allow multiple stacks
  perf, x86: Don't leak kernel data via callchains
  perf, x86: Limit callchains to 127

 arch/x86/kernel/cpu/perf_event.c |   14 +++++++-------
 include/linux/perf_event.h       |    2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

-- 
1.7.8.4

[PATCH 1/3] perf, x86: Allow multiple stacks

[Arun Sharma]

Without this patch, applications with two different stack
regions (eg: native stack vs JIT stack) get truncated
callchains even when RBP chaining is present. GDB shows proper
stack traces and the frame pointer chaining is intact.

This patch disables the (fp < RSP) check, hoping that other checks
in the code save the day for us. In our limited testing, this
didn't seem to break anything.

In the long term, we could potentially have userspace advise
the kernel on the range of valid stack addresses, so we don't
spend a lot of time unwinding from bogus addresses.

Signed-off-by: Arun Sharma <asharma@fb.com>
Cc: Ingo Molnar <mingo@elte.hu>
CC: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Tom Zanussi <tzanussi@gmail.com>
Cc: linux-kernel@vger.kernel.org
Cc: linux-perf-users@vger.kernel.org
---
 arch/x86/kernel/cpu/perf_event.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index bb8e034..9f98636 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1785,9 +1785,6 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry *entry)
 		if (bytes != sizeof(frame))
 			break;
 
-		if (fp < compat_ptr(regs->sp))
-			break;
-
 		perf_callchain_store(entry, frame.return_address);
 		fp = compat_ptr(frame.next_frame);
 	}
@@ -1831,9 +1828,6 @@ perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs)
 		if (bytes != sizeof(frame))
 			break;
 
-		if ((unsigned long)fp < regs->sp)
-			break;
-
 		perf_callchain_store(entry, frame.return_address);
 		fp = frame.next_frame;
 	}
-- 
1.7.8.4

[PATCH 2/3] perf, x86: Don't leak kernel data via callchains

[Arun Sharma]

The NMI could fire when the kernel has done set_fs(KERNEL_DS).
Since we're interested in user space stack pointers only,
check against USER_DS explicitly.

Signed-off-by: Arun Sharma <asharma@fb.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: linux-kernel@vger.kernel.org
---
 arch/x86/kernel/cpu/perf_event.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
index 9f98636..ccd8c07 100644
--- a/arch/x86/kernel/cpu/perf_event.c
+++ b/arch/x86/kernel/cpu/perf_event.c
@@ -1803,6 +1803,7 @@ perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs)
 {
 	struct stack_frame frame;
 	const void __user *fp;
+	mm_segment_t oldfs = get_fs();
 
 	if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
 		/* TODO: We don't support guest os callchain now */
@@ -1816,8 +1817,9 @@ perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs)
 	if (!current->mm)
 		return;
 
+	set_fs(USER_DS);
 	if (perf_callchain_user32(regs, entry))
-		return;
+		goto out;
 
 	while (entry->nr < PERF_MAX_STACK_DEPTH) {
 		unsigned long bytes;
@@ -1831,6 +1833,10 @@ perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs *regs)
 		perf_callchain_store(entry, frame.return_address);
 		fp = frame.next_frame;
 	}
+
+out:
+	set_fs(oldfs);
+	return;
 }
 
 unsigned long perf_instruction_pointer(struct pt_regs *regs)
-- 
1.7.8.4

[PATCH 3/3] perf: Limit callchains to 127

[Arun Sharma]

Stack depth of 255 seems excessive, given that copy_from_user_nmi()
could be slow.

Signed-off-by: Arun Sharma <asharma@fb.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: linux-kernel@vger.kernel.org
---
 include/linux/perf_event.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index ddbb6a9..d1e71c1 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -609,7 +609,7 @@ struct perf_guest_info_callbacks {
 #include <linux/sysfs.h>
 #include <asm/local.h>
 
-#define PERF_MAX_STACK_DEPTH		255
+#define PERF_MAX_STACK_DEPTH		127
 
 struct perf_callchain_entry {
 	__u64				nr;
-- 
1.7.8.4

Re: [PATCH 2/3] perf, x86: Don't leak kernel data via callchains

[Linus Torvalds]

On Fri, Apr 20, 2012 at 12:35 PM, Arun Sharma <asharma@fb.com> wrote:
>
> +       set_fs(USER_DS);

NAK.

I really don't think this is the right thing to do. We're in random
NMI context, set_fs() is not at all necessarily safe.

No, the code needs to check the frame range by hand, not depend on current-fs.

In fact, I'd argue that this is a bug in copy_from_user_nmi(), which
should simply check the *user* address space explicitly, not using
access_ok().

                   Linus