Messages in this thread | | | Date | Fri, 21 Aug 2009 16:21:51 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 1/4] perf_counter: Default to higher paranoia level |
| |
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, 2009-08-19 at 16:07 +0200, Peter Zijlstra wrote: > > On Wed, 2009-08-19 at 11:18 +0200, Peter Zijlstra wrote: > > > > > +static inline bool perf_paranoid_anon(void) > > > +{ > > > + return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 1; > > > } > > > > > > static inline bool perf_paranoid_kernel(void) > > > { > > > - return sysctl_perf_counter_paranoid > 1; > > > + return !capable(CAP_SYS_ADMIN) && sysctl_perf_counter_paranoid > 2; > > > +} > > > > OK, this is buggy: > > > > - capable() uses current, which is unlikely to be counter->owner, > > - but even security_real_capable(counter->owner, ...) wouldn't > > work, since the ->capable() callback isn't NMI safe > > (selinux takes locks and does allocations in that path). > > > > This puts a severe strain on more complex anonymizers since its > > basically impossible to tell if counter->owner has permissions on > > current from NMI context. > > > > I'll fix up this patch to pre-compute the perf_paranoid_anon_ip() per > > counter based on creation time state, unless somebody has a better idea. > > > > I could possibly only anonymize IRQ context (SoftIRQ context is > > difficult since in_softirq() means both in-softirq and > > softirq-disabled). > > Something like the below maybe.. its 3 patches folded and I've no clue > how to adapt the ppc code, what do people think? > > compile tested on x86-64 only. > > --- > arch/x86/include/asm/stacktrace.h | 14 +++++-- > arch/x86/kernel/cpu/perf_counter.c | 66 +++++++++++++++++++++++++++++++---- > arch/x86/kernel/dumpstack.c | 10 +++-- > arch/x86/kernel/dumpstack_32.c | 8 +---- > arch/x86/kernel/dumpstack_64.c | 13 ++----- > arch/x86/kernel/stacktrace.c | 8 +++-- > arch/x86/oprofile/backtrace.c | 5 ++- > include/linux/perf_counter.h | 5 ++- > kernel/perf_counter.c | 30 ++++++++++++---- > kernel/trace/trace_sysprof.c | 5 ++- > 10 files changed, 117 insertions(+), 47 deletions(-) > > diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h > index cf86a5e..7066caa 100644 > --- a/arch/x86/include/asm/stacktrace.h > +++ b/arch/x86/include/asm/stacktrace.h > @@ -3,17 +3,23 @@ > > extern int kstack_depth_to_print; > > -int x86_is_stack_id(int id, char *name); > - > /* Generic stack tracer with callbacks */ > > +enum stack_type { > + STACK_UNKNOWN = -1, > + STACK_PROCESS = 0, > + STACK_INTERRUPT = 1, > + STACK_EXCEPTION = 2, > +}; > + > struct stacktrace_ops { > void (*warning)(void *data, char *msg); > /* msg must contain %s for the symbol */ > void (*warning_symbol)(void *data, char *msg, unsigned long symbol); > - void (*address)(void *data, unsigned long address, int reliable); > + void (*address)(void *data, unsigned long stack, > + unsigned long address, int reliable); > /* On negative return stop dumping */ > - int (*stack)(void *data, char *name); > + int (*stack)(void *data, int type, char *name); > };
nice generalization ...
> diff --git a/include/linux/perf_counter.h b/include/linux/perf_counter.h > index 9ba1822..2b0528f 100644 > --- a/include/linux/perf_counter.h > +++ b/include/linux/perf_counter.h > @@ -439,6 +439,7 @@ enum perf_callchain_context { > struct perf_callchain_entry { > __u64 nr; > __u64 ip[PERF_MAX_STACK_DEPTH]; > + int restricted; > };
i'd love to have something more specific here - i.e. a context type ID that identifies these basic types:
- process - softirq - hardirq - NMI
and then let it be up to upper layers to decide what they do with a restricted entry, and how to further process this information.
And it's not just security: for example it would be interesting to sample pure, non-irq overhead - as IRQ overhead is often unrelated to the process being measured.
Ingo
| |