All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Olsa <jolsa@redhat.com>
To: Andy Lutomirski <luto@amacapital.net>
Cc: "Stephane Eranian" <eranian@google.com>,
	"Ingo Molnar" <mingo@redhat.com>, root <chenggang.qin@gmail.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"秦承刚(承刚)" <chenggang.qcg@taobao.com>,
	"Wu Fengguang" <fengguang.wu@intel.com>,
	"Namhyung Kim" <namhyung@gmail.com>,
	"Mike Galbraith" <efault@gmx.de>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Arjan van de Ven" <arjan@linux.intel.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	"David Ahern" <dsahern@gmail.com>,
	"Paul Mackerras" <paulus@samba.org>,
	"秦承刚(承刚)" <chenggang.qcg@alibaba-inc.com>,
	"Yanmin Zhang" <yanmin.zhang@intel.com>
Subject: Re: 答复:[PATCH] perf core: Use KSTK_ESP() instead of pt_regs->sp while output user regs
Date: Sun, 4 Jan 2015 18:41:05 +0100	[thread overview]
Message-ID: <20150104174105.GA29388@krava.brq.redhat.com> (raw)
In-Reply-To: <CALCETrVNPmMW_ZABfaEFX6pKN6JL-K-hpR-7xM4qJh+wAReO5g@mail.gmail.com>

On Sun, Jan 04, 2015 at 09:18:59AM -0800, Andy Lutomirski wrote:
> On Jan 4, 2015 8:11 AM, "Jiri Olsa" <jolsa@redhat.com> wrote:
> >
> > On Tue, Dec 30, 2014 at 08:03:27PM +0100, Peter Zijlstra wrote:
> > > On Thu, Dec 25, 2014 at 07:48:28AM -0800, Andy Lutomirski wrote:
> > > > On a quick look, there are plenty of other bugs in there besides just
> > > > the stack pointer issue.  The ABI check that uses TIF_IA32 in the perf
> > > > core is completely wrong.  TIF_IA32 may be equal to the actual
> > > > userspace bitness by luck, but, if so, that's more or less just luck.
> > > > And there's a user_mode test that should be user_mode_vm.
> > > >
> > > > Also, it's not just sp that's wrong.  There are various places that
> > > > you can interrupt in which many of the registers have confusing
> > > > locations.  You could try using the cfi unwind data, but that's
> > > > unlikely to work for regs like cs and ss, and, during context switch,
> > > > this has very little chance of working.
> > > >
> > > > What's the point of this feature?  Honestly, my suggestion would be to
> > > > delete it instead of trying to fix it.  It's also not clear to me that
> > > > there aren't serious security problems here -- it's entirely possible
> > > > for sensitive *kernel* values to and up in task_pt_regs at certain
> > > > times, and if you run during context switch and there's no code to
> > > > suppress this dump during context switch, then you could be showing
> > > > regs that belong to the wrong task.
> > >
> > > Of course the people who actually wrote the code are not on CC :/
> > >
> > > There's two users of this iirc;
> > >
> > >  1) the dwarf stack unwinder thingy, which basically dumps the userspace
> > >  regs and the top of userspace stack on 'event'.
> >
> > looks like this solves the issue I was trying to fix
> > long time ago:
> >   http://marc.info/?l=linux-kernel&m=134934717011451&w=2
> >
> > this seems a lot simpler ;-) I'll test..
> 
> I suspect that, if it works, then it's pure luck.
> (task)->thread.usersp in KSTK_ESP is bogus -- your code was more
> accurate.
> 
> I think we should seriously consider making use of this feature by
> non-root users require an explicit sysctl.  Sending values to user
> code that are, at best, free of sensitive kernel data most of the time
> is IMO inappropriate for an unprivileged API.
> 
> I'm currently working on a patch to try to clean this up better.

ook.. well FWIW I tested and if I used KSTK_ESP as a perf user stack
pointer (attached patch) I've got all callchains resolved properly,
as when I tested my original patch

NOTE the patch from Chenggang Qin isnt enough to fix my issue for perf
     dwarf unwind, I needed to use attached change

I'd be happy to test any change you make for this,
because this has been pain for a long time :-\

thanks,
jirka


---
diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
index 5da8df8303cf..eb7c385a6f8e 100644
--- a/arch/x86/kernel/perf_regs.c
+++ b/arch/x86/kernel/perf_regs.c
@@ -66,6 +66,11 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
 	return regs_get_register(regs, pt_regs_offset[idx]);
 }
 
+unsigned long arch_perf_user_stack_pointer(struct pt_regs *regs)
+{
+	return KSTK_ESP(current);
+}
+
 #define REG_RESERVED (~((1ULL << PERF_REG_X86_MAX) - 1ULL))
 
 #ifdef CONFIG_X86_32
diff --git a/kernel/events/core.c b/kernel/events/core.c
index af0a5ba4e21d..d99236173f3b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4486,6 +4486,19 @@ static void perf_sample_regs_intr(struct perf_regs *regs_intr,
 	regs_intr->abi  = perf_reg_abi(current);
 }
 
+#ifdef CONFIG_HAVE_PERF_USER_STACK_DUMP
+unsigned long __weak arch_perf_user_stack_pointer(struct pt_regs *regs)
+{
+        return user_stack_pointer(regs);
+}
+
+static unsigned long perf_user_stack_pointer(struct pt_regs *regs)
+{
+        return arch_perf_user_stack_pointer(regs);
+}
+#else
+#define perf_user_stack_pointer(regs) 0
+#endif /* CONFIG_HAVE_PERF_USER_STACK_DUMP */
 
 /*
  * Get remaining task size from user stack pointer.
diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 569b218782ad..b85e4fd52980 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -180,19 +180,17 @@ static inline void put_recursion_context(int *recursion, int rctx)
 }
 
 #ifdef CONFIG_HAVE_PERF_USER_STACK_DUMP
+unsigned long arch_perf_user_stack_pointer(struct pt_regs *regs);
+
 static inline bool arch_perf_have_user_stack_dump(void)
 {
 	return true;
 }
-
-#define perf_user_stack_pointer(regs) user_stack_pointer(regs)
 #else
 static inline bool arch_perf_have_user_stack_dump(void)
 {
 	return false;
 }
-
-#define perf_user_stack_pointer(regs) 0
 #endif /* CONFIG_HAVE_PERF_USER_STACK_DUMP */
 
 #endif /* _KERNEL_EVENTS_INTERNAL_H */

  reply	other threads:[~2015-01-04 17:41 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-23  6:22 [PATCH] " root
2014-12-23  8:30 ` Andy Lutomirski
     [not found] ` <c027bde0-5f4f-441f-8d45-3e7f6f702231@alibaba-inc.com>
2014-12-25 15:48   ` 答复:[PATCH] " Andy Lutomirski
2014-12-25 16:21     ` Andy Lutomirski
2014-12-30 19:03     ` Peter Zijlstra
2014-12-30 23:29       ` Andy Lutomirski
2014-12-31  2:00         ` Andy Lutomirski
2015-01-02 16:11           ` Jan Beulich
2015-01-02 18:03             ` Andy Lutomirski
2015-01-05  8:47               ` Jan Beulich
2015-01-04 16:10       ` Jiri Olsa
2015-01-04 17:18         ` Andy Lutomirski
2015-01-04 17:41           ` Jiri Olsa [this message]
2015-01-04 18:36             ` [PATCH 0/2] perf: Improve user regs sampling Andy Lutomirski
2015-01-04 18:36               ` [PATCH 1/2] perf: Move task_pt_regs sampling into arch code Andy Lutomirski
2015-01-05 14:07                 ` Peter Zijlstra
2015-01-05 16:13                   ` Andy Lutomirski
2015-01-05 16:44                     ` Peter Zijlstra
2015-01-05 18:28                       ` Andy Lutomirski
2015-01-09 12:32                 ` [tip:perf/urgent] " tip-bot for Andy Lutomirski
2015-01-04 18:36               ` [PATCH 2/2] x86_64, perf: Improve user regs sampling Andy Lutomirski
2015-01-09 12:32                 ` [tip:perf/urgent] perf/x86_64: " tip-bot for Andy Lutomirski
2015-01-05 10:46               ` [PATCH 0/2] perf: " Jiri Olsa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150104174105.GA29388@krava.brq.redhat.com \
    --to=jolsa@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=arjan@linux.intel.com \
    --cc=chenggang.qcg@alibaba-inc.com \
    --cc=chenggang.qcg@taobao.com \
    --cc=chenggang.qin@gmail.com \
    --cc=dsahern@gmail.com \
    --cc=efault@gmx.de \
    --cc=eranian@google.com \
    --cc=fengguang.wu@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mingo@redhat.com \
    --cc=namhyung@gmail.com \
    --cc=paulus@samba.org \
    --cc=peterz@infradead.org \
    --cc=yanmin.zhang@intel.com \
    --subject='Re: 答复:[PATCH] perf core: Use KSTK_ESP() instead of pt_regs->sp while output user regs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.