lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 23 Nov 2008 09:37:40 +0100 From: Ingo Molnar <mingo@...e.hu> To: Török Edwin <edwintorok@...il.com> Cc: srostedt@...hat.com, a.p.zijlstra@...llo.nl, sandmann@...mi.au.dk, linux-kernel@...r.kernel.org, viro@...IV.linux.org.uk Subject: Re: [PATCH 1/2] tracing: add support for userspace stacktraces in tracing/iter_ctrl * Török Edwin <edwintorok@...il.com> wrote: > +struct stack_frame { > + const void __user *next_fp; > + unsigned long return_address; > +}; Small detail: please s/return_address/ret_addr - its expressive power is the same but as a bonus it will cause less col-80 related linebreaks in usage sites. > +void save_stack_trace_user(struct stack_trace *trace) > +{ > + /* > + * Trace user stack if we are not a kernel thread > + */ > + if (current->mm) { > + const struct pt_regs *regs = task_pt_regs(current); > + const void __user *fp = (const void __user *)regs->bp; > + > + if (trace->nr_entries < trace->max_entries) > + trace->entries[trace->nr_entries++] = regs->ip; > + > + while (trace->nr_entries < trace->max_entries) { > + struct stack_frame frame; > + frame.next_fp = NULL; Style: put a newline after variable definitions please. > + frame.return_address = 0; > + if (!copy_stack_frame(fp, &frame)) > + break; > + if ((unsigned long)fp < regs->sp) > + break; > + if (frame.return_address) > + trace->entries[trace->nr_entries++] = > + frame.return_address; Style: please use curly braces around multi-line conditional statements. > + if (fp == frame.next_fp) > + break; > + fp = frame.next_fp; > + } > + } Detail: please move the whole "if (current->mm)" into a __save_stack_trace_user() helper function - that way it reads cleaner. > +++ b/include/linux/stacktrace.h > @@ -18,9 +18,17 @@ extern void save_stack_trace_tsk(struct task_struct *tsk, > struct stack_trace *trace); > > extern void print_stack_trace(struct stack_trace *trace, int spaces); > + > +#ifdef CONFIG_X86 > +extern void save_stack_trace_user(struct stack_trace *trace); > +#else > +# define save_stack_trace_user(trace) do { } while (0) > +#endif Bug: this should not be CONFIG_X86. Please introduce (in a separate patch from other cleanups) a CONFIG_USER_STACKTRACE_SUPPORT in arch/x86/Kconfig and use that instead. > + > #else > # define save_stack_trace(trace) do { } while (0) > # define save_stack_trace_tsk(tsk, trace) do { } while (0) > +# define save_stack_trace_user(trace) do { } while (0) Style: these should be tabs, not spaces ^^^^^^^^^^^^ > +static void ftrace_trace_userstack(struct trace_array *tr, > + struct trace_array_cpu *data, > + unsigned long flags, int pc) > +{ > + struct userstack_entry *entry; > + struct stack_trace trace; > + struct ring_buffer_event *event; > + unsigned long irq_flags; Detail: please use the customary ftrace variable definitions style: > + struct ring_buffer_event *event; > + struct userstack_entry *entry; > + struct stack_trace trace; > + unsigned long irq_flags; note how it's ordered by line length. > +static int > +seq_print_userip_objs(const struct userstack_entry *entry, struct trace_seq *s, > + unsigned long sym_flags) > +{ > + int ret = 1; > + unsigned i; Detail: please use "unsigned int" so that we have less type patterns to look for. > + > + for (i = 0; i < FTRACE_STACK_ENTRIES; i++) { > + unsigned long ip = entry->caller[i]; > + > + if (ip == ULONG_MAX || !ret) > + break; > + if (i) > + ret = trace_seq_puts(s, " <- "); > + if (!ip) { > + ret = trace_seq_puts(s, "??"); > + continue; > + } > + if (ret /*&& (sym_flags & TRACE_ITER_SYM_ADDR)*/) > + ret = trace_seq_printf(s, " <" IP_FMT ">", ip); Detail: do we need that commented-out condition? Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists