tracing: Updates for the next (coming soon) merge window
Subsystems (11)
Steven Rostedt
maintainercover tracing: Updates for the next (coming soon) merge window
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
for-next
Head SHA1: 05cd667ce570cb82a967af7166c54c610324861c
Mathieu Malaterre (1):
tracing: Make function ‘ftrace_exports’ static
Michael Ellerman (2):
seq_buf: Make seq_buf_puts() null-terminate the buffer
seq_buf: Use size_t for len in seq_buf_puts()
Rasmus Villemoes (3):
tracing: Merge seq_print_sym_short() and seq_print_sym_offset()
tracing: Avoid -Wformat-nonliteral warning
tracing: Simplify printf'ing in seq_print_sym
Steven Rostedt (1):
string.h: Add strncmp_prefix() helper macro
Steven Rostedt (VMware) (10):
ftrace: Allow ftrace_replace_code() to be schedulable
arm64: ftrace: Set FTRACE_MAY_SLEEP before ftrace_modify_all_code()
fgraph: Add comment to describe ftrace_graph_get_ret_stack x86/ftrace: Do not call function graph from dynamic trampolines powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
sparc64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
sh: ftrace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
tracing: Fix ftrace_graph_get_ret_stack() to use task and not current
tracing: Use strncmp_prefix() helper for histogram code
Tom Zanussi (7):
tracing: Remove unnecessary hist trigger struct field
tracing: Change strlen to sizeof for hist trigger static strings
tracing: Use var_refs[] for hist trigger reference checking
tracing: Remove open-coding of hist trigger var_ref management
tracing: Use hist trigger's var_ref array to destroy var_refs
tracing: Remove hist trigger synth_var_refs
tracing: Add hist trigger comments for variable-related fields
----
arch/arm64/kernel/ftrace.c | 1 + arch/arm64/kernel/perf_callchain.c | 2 +- arch/arm64/kernel/process.c | 2 +- arch/arm64/kernel/return_address.c | 2 +- arch/arm64/kernel/stacktrace.c | 12 +- arch/arm64/kernel/time.c | 2 +- arch/arm64/kernel/traps.c | 2 +- arch/powerpc/kernel/process.c | 13 +- arch/sh/kernel/dumpstack.c | 11 +- arch/sh/kernel/dwarf.c | 9 +- arch/sparc/kernel/perf_event.c | 8 +- arch/sparc/kernel/stacktrace.c | 8 +- arch/sparc/kernel/traps_64.c | 7 +- arch/x86/kernel/ftrace.c | 41 +++--- arch/x86/kernel/ftrace_64.S | 8 +- include/linux/ftrace.h | 1 + include/linux/string.h | 22 +++ kernel/trace/fgraph.c | 15 +- kernel/trace/ftrace.c | 19 ++- kernel/trace/trace.c | 2 +- kernel/trace/trace_events_hist.c | 278 +++++++++++++++++++++---------------- kernel/trace/trace_output.c | 38 ++--- lib/seq_buf.c | 8 +- 23 files changed, 301 insertions(+), 210 deletions(-)
Steven Rostedt
maintainerPATCH 1/24 ftrace: Allow ftrace_replace_code() to be schedulable
From: "Steven Rostedt (VMware)" <[email protected]>
The function ftrace_replace_code() is the ftrace engine that does the work to modify all the nops into the calls to the function callback in all the functions being traced.
The generic version which is normally called from stop machine, but an architecture can implement a non stop machine version and still use the generic ftrace_replace_code(). When an architecture does this, ftrace_replace_code() may be called from a schedulable context, where it can allow the code to be preemptible, and schedule out.
In order to allow an architecture to make ftrace_replace_code() schedulable, a new command flag is added called:
FTRACE_MAY_SLEEP
Which can be or'd to the command that is passed to ftrace_modify_all_code() that calls ftrace_replace_code() and will have it call cond_resched() in the loop that modifies the nops into the calls to the ftrace trampolines.
--- include/linux/ftrace.h | 1 + kernel/trace/ftrace.c | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)
include/linux/ftrace.h +1 -0
@@ -389,6 +389,7 @@ enum {FTRACE_UPDATE_TRACE_FUNC = (1 <<2 ),FTRACE_START_FUNC_RET = (1 <<3 ),FTRACE_STOP_FUNC_RET = (1 <<4 ), + FTRACE_MAY_SLEEP = (1 << 5), };/*
kernel/trace/ftrace.c +16 -3
@@ -77,6 +77,11 @@#define ASSIGN_OPS_HASH (opsname, val)#endif +enum { +FTRACE_MODIFY_ENABLE_FL = (1 <<0 ), +FTRACE_MODIFY_MAY_SLEEP_FL = (1 <<1 ), +}; +struct ftrace_ops ftrace_list_end__read_mostly = { .func = ftrace_stub, .flags =FTRACE_OPS_FL_RECURSION_SAFE |FTRACE_OPS_FL_STUB ,
@@ -2389,10 +2394,12 @@ __ftrace_replace_code(struct dyn_ftrace *rec, int enable)return -1 ;/* unknow ftrace bug */ } -void __weak ftrace_replace_code (int enable) +void __weak ftrace_replace_code (int mod_flags) {struct dyn_ftrace *rec;struct ftrace_page *pg; +int enable = mod_flags &FTRACE_MODIFY_ENABLE_FL ; +int schedulable = mod_flags &FTRACE_MODIFY_MAY_SLEEP_FL ;int failed;if (unlikely (ftrace_disabled))
@@ -2409,6 +2416,8 @@ void __weak ftrace_replace_code(int enable)/* Stop processing */ return ; } +if (schedulable) +cond_resched (); }while_for_each_ftrace_rec (); }
@@ -2522,8 +2531,12 @@ int __weak ftrace_arch_code_modify_post_process(void)void ftrace_modify_all_code (int command) {int update = command &FTRACE_UPDATE_TRACE_FUNC ; +int mod_flags =0 ;int err =0 ; +if (command &FTRACE_MAY_SLEEP ) + mod_flags =FTRACE_MODIFY_MAY_SLEEP_FL ; +/* * If the ftrace_caller calls a ftrace_ops func directly, * we need to make sure that it only traces functions it
@@ -2541,9 +2554,9 @@ void ftrace_modify_all_code(int command) }if (command &FTRACE_UPDATE_CALLS ) -ftrace_replace_code (1 ); +ftrace_replace_code (mod_flags |FTRACE_MODIFY_ENABLE_FL );else if (command &FTRACE_DISABLE_CALLS ) -ftrace_replace_code (0 ); +ftrace_replace_code (mod_flags);if (update && ftrace_trace_function != ftrace_ops_list_func) { function_trace_op = set_function_trace_op;
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 2/24 arm64: ftrace: Set FTRACE_MAY_SLEEP before ftrace_modify_all_code()
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
It has been reported that ftrace_replace_code() which is called by ftrace_modify_all_code() can cause a soft lockup warning for an allmodconfig kernel. This is because all the debug options enabled causes the loop in ftrace_replace_code() (which loops over all the functions being enabled where there can be 10s of thousands), is too slow, and never schedules out.
To solve this, setting FTRACE_MAY_SLEEP to the command passed into ftrace_replace_code() will make it call cond_resched() in the loop, which prevents the soft lockup warning from triggering.
--- arch/arm64/kernel/ftrace.c | 1 + 1 file changed, 1 insertion(+)
arch/arm64/kernel/ftrace.c +1 -0
@@ -193,6 +193,7 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,void arch_ftrace_update_code (int command) { + command |=FTRACE_MAY_SLEEP ;ftrace_modify_all_code (command); }
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 3/24 fgraph: Add comment to describe ftrace_graph_get_ret_stack
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The ret_stack should not be accessed directly via the curr_ret_stack variable on the task_struct. This is because the ret_stack is going to be converted into a series of longs and not an array of ret_stack structures. The way that a ret_stack should be retrieved is via the ftrace_graph_get_ret_stack structure, but it needs to be documented on how to use it.
--- kernel/trace/fgraph.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
kernel/trace/fgraph.c +11 -0
@@ -232,6 +232,17 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)return ret; } +/** +* ftrace_graph_get_ret_stack - return the entry of the shadow stack +* @task: The task to read the shadow stack from +* @idx: Index down the shadow stack +* +* Return the ret_struct on the shadow stack of the @task at the +* call graph at @idx starting with zero. If @idx is zero, it +* will return the last saved ret_stack entry. If it is greater than +* zero, it will return the corresponding ret_stack for the depth +* of saved return addresses. +*/ struct ftrace_ret_stack *ftrace_graph_get_ret_stack (struct task_struct *task,int idx) {
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 4/24 x86/ftrace: Do not call function graph from dynamic trampolines
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
Since commit 79922b8009c07 ("ftrace: Optimize function graph to be called directly"), dynamic trampolines should not be calling the function graph tracer at the end. If they do, it could cause the function graph tracer to trace functions that it filtered out.
Right now it does not cause a problem because there's a test to check if the function graph tracer is attached to the same function as the function tracer, which for now is true. But the function graph tracer is undergoing changes that can make this no longer true which will cause the function graph tracer to trace other functions.
For example:
# cd /sys/kernel/tracing/ # echo do_IRQ > set_ftrace_filter # mkdir instances/foo # echo ip_rcv > instances/foo/set_ftrace_filter # echo function_graph > current_tracer # echo function > instances/foo/current_tracer
Would cause the function graph tracer to trace both do_IRQ and ip_rcv, if the current tests change.
As the current tests prevent this from being a problem, this code does not need to be backported. But it does make the code cleaner.
--- arch/x86/kernel/ftrace.c | 41 ++++++++++++++++++++----------------- arch/x86/kernel/ftrace_64.S | 8 ++++---- 2 files changed, 26 insertions(+), 23 deletions(-)
arch/x86/kernel/ftrace.c +22 -19
@@ -733,18 +733,20 @@ union ftrace_op_code_union { } __attribute__((packed)); }; +#define RET_SIZE 1 +static unsigned long create_trampoline (struct ftrace_ops *ops,unsigned int *tramp_size) { -unsigned const char *jmp;unsigned long start_offset;unsigned long end_offset;unsigned long op_offset;unsigned long offset;unsigned long size; -unsigned long ip; +unsigned long retq;unsigned long *ptr;void *trampoline; +void *ip;/* 48 8b 15 <offset> is movq <offset>(%rip), %rdx */ unsigned const char op_ref[] = {0x48 ,0x8b ,0x15 };union ftrace_op_code_union op_ptr;
@@ -764,27 +766,27 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)/* * Allocate enough size to store the ftrace_caller code, -* the jmp to ftrace_epilogue, as well as the address of -* the ftrace_ops this trampoline is used for. +* the iret , as well as the address of the ftrace_ops this +* trampoline is used for. */ - trampoline =alloc_tramp (size +MCOUNT_INSN_SIZE +sizeof (void *)); + trampoline =alloc_tramp (size +RET_SIZE +sizeof (void *));if (!trampoline)return 0 ; - *tramp_size = size +MCOUNT_INSN_SIZE +sizeof (void *); + *tramp_size = size +RET_SIZE +sizeof (void *);/* Copy ftrace_caller onto the trampoline memory */ ret =probe_kernel_read (trampoline, (void *)start_offset, size); -if (WARN_ON (ret <0 )) { -tramp_free (trampoline, *tramp_size); -return 0 ; - } +if (WARN_ON (ret <0 )) + goto fail; - ip = (unsigned long )trampoline + size; + ip = trampoline + size; -/* The trampoline ends with a jmp to ftrace_epilogue */ - jmp =ftrace_jmp_replace (ip, (unsigned long )ftrace_epilogue); -memcpy (trampoline + size, jmp,MCOUNT_INSN_SIZE ); +/* The trampoline ends with ret(q) */ + retq = (unsigned long )ftrace_stub; + ret =probe_kernel_read (ip, (void *)retq,RET_SIZE ); +if (WARN_ON (ret <0 )) + goto fail;/* * The address of the ftrace_ops that is used for this trampoline
@@ -794,17 +796,15 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)* the global function_trace_op variable. */ - ptr = (unsigned long *)(trampoline + size +MCOUNT_INSN_SIZE ); + ptr = (unsigned long *)(trampoline + size +RET_SIZE ); *ptr = (unsigned long )ops; op_offset -= start_offset;memcpy (&op_ptr, trampoline + op_offset,OP_REF_SIZE );/* Are we pointing to the reference? */ -if (WARN_ON (memcmp (op_ptr.op, op_ref,3 ) !=0 )) { -tramp_free (trampoline, *tramp_size); -return 0 ; - } +if (WARN_ON (memcmp (op_ptr.op, op_ref,3 ) !=0 )) + goto fail;/* Load the contents of ptr into the callback parameter */ offset = (unsigned long )ptr;
@@ -819,6 +819,9 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size) ops->flags |=FTRACE_OPS_FL_ALLOC_TRAMP ;return (unsigned long )trampoline; +fail: +tramp_free (trampoline, *tramp_size); +return 0 ; }static unsigned long calc_trampoline_call_offset (bool save_regs)
arch/x86/kernel/ftrace_64.S +4 -4
@@ -171,9 +171,6 @@ GLOBAL(ftrace_call) restore_mcount_regs /* - * The copied trampoline must call ftrace_epilogue as it - * still may need to call the function graph tracer. - * * The code up to this label is copied into trampolines so * think twice before adding any new code or changing the * layout here.
@@ -185,7 +182,10 @@ GLOBAL(ftrace_graph_call) jmp ftrace_stub #endif -/* This is weak to keep gas from relaxing the jumps */ +/* + * This is weak to keep gas from relaxing the jumps. + * It is also used to copy the retq for trampolines. + */ WEAK(ftrace_stub) retq ENDPROC(ftrace_caller)
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 5/24 powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The structure of the ret_stack array on the task struct is going to change, and accessing it directly via the curr_ret_stack index will no longer give the ret_stack entry that holds the return address. To access that, architectures must now use ftrace_graph_get_ret_stack() to get the associated ret_stack that matches the saved return address.
--- arch/powerpc/kernel/process.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
arch/powerpc/kernel/process.c +9 -4
@@ -2061,9 +2061,10 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)int count =0 ;int firstframe =1 ;#ifdef CONFIG_FUNCTION_GRAPH_TRACER -int curr_frame = current->curr_ret_stack; +struct ftrace_ret_stack *ret_stack;extern void return_to_handler (void );unsigned long rth = (unsigned long )return_to_handler; +int curr_frame =0 ;#endif sp = (unsigned long ) stack;
@@ -2089,9 +2090,13 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)printk ("[" REG "] [" REG "] %pS" , sp, ip, (void *)ip);#ifdef CONFIG_FUNCTION_GRAPH_TRACER if ((ip == rth) && curr_frame >=0 ) { -pr_cont (" (%pS)" , - (void *)current->ret_stack[curr_frame].ret); - curr_frame--; + ret_stack =ftrace_graph_get_ret_stack (current, + curr_frame++); +if (ret_stack) +pr_cont (" (%pS)" , + (void *)ret_stack->ret); +else + curr_frame =-1 ; }#endif if (firstframe)
Signature · 1 line
2.19.2
Re: [for-next][PATCH 05/24] powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
Quoted 8 lines · Steven Rostedt
From: "Steven Rostedt (VMware)" <[email protected]>
The structure of the
ret_stackarray on the task struct is going to change, and accessing it directly via thecurr_ret_stackindex will no longer give theret_stackentry that holds the return address. To access that, architectures must now useftrace_graph_get_ret_stack()to get the associatedret_stackthat matches the saved return address.
Quoted 8 lines
Cc Benjamin Herrenschmidt <[email protected]>Cc Paul Mackerras <[email protected]>Cc Michael Ellerman <[email protected]>Signed-off-by Steven Rostedt (VMware) <[email protected]>--- arch/powerpc/kernel/process.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
I did test the previous series you posted and I think everything was OK running the ftracetest suite (I got one or two fails but I think that was because of missing CONFIG options).
cheers
Steven Rostedt
maintainerRe: [for-next][PATCH 05/24] powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
Quoted 5 lines · On Sat, 22 Dec 2018 20:51:21 +1100 Michael Ellerman <[email protected]> wrote:
I did test the previous series you posted and I think everything was OK running the ftracetest suite (I got one or two fails but I think that was because of missing CONFIG options).
Thanks! I'll add your ack.
-- Steve
Steven Rostedt
maintainerPATCH 6/24 sparc64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The structure of the ret_stack array on the task struct is going to change, and accessing it directly via the curr_ret_stack index will no longer give the ret_stack entry that holds the return address. To access that, architectures must now use ftrace_graph_get_ret_stack() to get the associated ret_stack that matches the saved return address.
--- arch/sparc/kernel/perf_event.c | 8 +++++--- arch/sparc/kernel/stacktrace.c | 8 +++++--- arch/sparc/kernel/traps_64.c | 7 ++++--- 3 files changed, 14 insertions(+), 9 deletions(-)
arch/sparc/kernel/perf_event.c +5 -3
@@ -1767,9 +1767,11 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,perf_callchain_store (entry, pc);#ifdef CONFIG_FUNCTION_GRAPH_TRACER if ((pc +8UL ) = (unsigned long ) &return_to_handler) { -int index = current->curr_ret_stack; -if (current->ret_stack && index >= graph) { - pc = current->ret_stack[index - graph].ret; +struct ftrace_ret_stack *ret_stack; + ret_stack =ftrace_graph_get_ret_stack (current, + graph); +if (ret_stack) { + pc = ret_stack->ret;perf_callchain_store (entry, pc); graph++; }
arch/sparc/kernel/stacktrace.c +5 -3
@@ -57,9 +57,11 @@ static void __save_stack_trace(struct thread_info *tp, trace->entries[trace->nr_entries++] = pc;#ifdef CONFIG_FUNCTION_GRAPH_TRACER if ((pc +8UL ) = (unsigned long ) &return_to_handler) { -int index = t->curr_ret_stack; -if (t->ret_stack && index >= graph) { - pc = t->ret_stack[index - graph].ret; +struct ftrace_ret_stack *ret_stack; + ret_stack =ftrace_graph_get_ret_stack (t, + graph); +if (ret_stack) { + pc = ret_stack->ret;if (trace->nr_entries < trace->max_entries) trace->entries[trace->nr_entries++] = pc;
arch/sparc/kernel/traps_64.c +4 -3
@@ -2502,9 +2502,10 @@ void show_stack(struct task_struct *tsk, unsigned long *_ksp)printk (" [%016lx] %pS\n" , pc, (void *) pc);#ifdef CONFIG_FUNCTION_GRAPH_TRACER if ((pc +8UL ) = (unsigned long ) &return_to_handler) { -int index = tsk->curr_ret_stack; -if (tsk->ret_stack && index >= graph) { - pc = tsk->ret_stack[index - graph].ret; +struct ftrace_ret_stack *ret_stack; + ret_stack =ftrace_graph_get_ret_stack (tsk, graph); +if (ret_stack) { + pc = ret_stack->ret;printk (" [%016lx] %pS\n" , pc, (void *) pc); graph++; }
Signature · 1 line
2.19.2
Re: [for-next][PATCH 06/24] sparc64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
From: Steven Rostedt <[email protected]>
Date: Fri, 21 Dec 2018 12:56:24 -0500
Quoted 11 lines
From: "Steven Rostedt (VMware)" <[email protected]>
The structure of the
ret_stackarray on the task struct is going to change, and accessing it directly via thecurr_ret_stackindex will no longer give theret_stackentry that holds the return address. To access that, architectures must now useftrace_graph_get_ret_stack()to get the associatedret_stackthat matches the saved return address.Cc "David S. Miller" <[email protected]>Signed-off-by Steven Rostedt (VMware) <[email protected]>
Steven Rostedt
maintainerRe: [for-next][PATCH 06/24] sparc64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
Quoted 11 lines · On Fri, 21 Dec 2018 10:24:14 -0800 (PST) David Miller <[email protected]>…
From: Steven Rostedt <[email protected]>
Date: Fri, 21 Dec 2018 12:56:24 -0500Quoted 8 lines
From: "Steven Rostedt (VMware)" <[email protected]>
The structure of the
ret_stackarray on the task struct is going to change, and accessing it directly via thecurr_ret_stackindex will no longer give theret_stackentry that holds the return address. To access that, architectures must now useftrace_graph_get_ret_stack()to get the associatedret_stackthat matches the saved return address.
Quoted 5 lines
Quoted 3 lines
Cc "David S. Miller" <[email protected]>Signed-off-by Steven Rostedt (VMware) <[email protected]>Acked-by David S. Miller <[email protected]>
Thanks David!
-- Steve
Steven Rostedt
maintainerPATCH 7/24 sh: ftrace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The structure of the ret_stack array on the task struct is going to change, and accessing it directly via the curr_ret_stack index will no longer give the ret_stack entry that holds the return address. To access that, architectures must now use ftrace_graph_get_ret_stack() to get the associated ret_stack that matches the saved return address.
--- arch/sh/kernel/dumpstack.c | 11 +++++++---- arch/sh/kernel/dwarf.c | 9 +++++---- 2 files changed, 12 insertions(+), 8 deletions(-)
arch/sh/kernel/dumpstack.c +7 -4
@@ -59,17 +59,20 @@ print_ftrace_graph_addr(unsigned long addr, void *data, struct thread_info *tinfo, int *graph) { struct task_struct *task = tinfo->task; +struct ftrace_ret_stack *ret_stack;unsigned long ret_addr; -int index = task->curr_ret_stack;if (addr != (unsigned long )return_to_handler)return ; -if (!task->ret_stack || index < *graph) +if (!task->ret_stack)return ; - index -= *graph; - ret_addr = task->ret_stack[index].ret; + ret_stack =ftrace_graph_get_ret_stack (task, *graph); +if (!ret_stack) +return ; + + ret_addr = ret_stack->ret; ops->address (data, ret_addr,1 );
arch/sh/kernel/dwarf.c +5 -4
@@ -608,17 +608,18 @@ struct dwarf_frame *dwarf_unwind_stack(unsigned long pc,* expected to find the real return address. */ if (pc = (unsigned long )&return_to_handler) { -int index = current->curr_ret_stack; +struct ftrace_ret_stack *ret_stack; + ret_stack =ftrace_graph_get_ret_stack (current,0 ); +if (ret_stack) + pc = ret_stack->ret;/* * We currently have no way of tracking how many * return_to_handler()'s we've seen. If there is more * than one patched return address on our stack, * complain loudly. */ -WARN_ON (index >0 ); - - pc = current->ret_stack[index].ret; + WARN_ON(ftrace_graph_get_ret_stack(current, 1); }#endif
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 8/24 arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The structure of the ret_stack array on the task struct is going to change, and accessing it directly via the curr_ret_stack index will no longer give the ret_stack entry that holds the return address. To access that, architectures must now use ftrace_graph_get_ret_stack() to get the associated ret_stack that matches the saved return address.
--- arch/arm64/kernel/perf_callchain.c | 2 +- arch/arm64/kernel/process.c | 2 +- arch/arm64/kernel/return_address.c | 2 +- arch/arm64/kernel/stacktrace.c | 12 +++++++----- arch/arm64/kernel/time.c | 2 +- arch/arm64/kernel/traps.c | 2 +- 6 files changed, 12 insertions(+), 10 deletions(-)
arch/arm64/kernel/perf_callchain.c +1 -1
@@ -164,7 +164,7 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry, frame.fp = regs->regs[29 ]; frame.pc = regs->pc;#ifdef CONFIG_FUNCTION_GRAPH_TRACER - frame.graph = current->curr_ret_stack; + frame.graph =0 ;#endif walk_stackframe (current, &frame, callchain_trace, entry);
arch/arm64/kernel/process.c +1 -1
@@ -459,7 +459,7 @@ unsigned long get_wchan(struct task_struct *p) frame.fp =thread_saved_fp (p); frame.pc =thread_saved_pc (p);#ifdef CONFIG_FUNCTION_GRAPH_TRACER - frame.graph = p->curr_ret_stack; + frame.graph =0 ;#endif do { if (unwind_frame(p, &frame))
arch/arm64/kernel/return_address.c +1 -1
@@ -44,7 +44,7 @@ void *return_address(unsigned int level) frame.fp = (unsigned long )__builtin_frame_address (0 ); frame.pc = (unsigned long )return_address;/* dummy */ #ifdef CONFIG_FUNCTION_GRAPH_TRACER - frame.graph = current->curr_ret_stack; + frame.graph =0 ;#endif walk_stackframe (current, &frame, save_return_addr, &data);
arch/arm64/kernel/stacktrace.c +7 -5
@@ -59,15 +59,17 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)#ifdef CONFIG_FUNCTION_GRAPH_TRACER if (tsk->ret_stack && (frame->pc == (unsigned long )return_to_handler)) { -if (WARN_ON_ONCE (frame->graph ==-1 )) -return -EINVAL ; +struct ftrace_ret_stack *ret_stack;/* * This is a case where function graph tracer has * modified a return address (LR) in a stack frame * to hook a function return. * So replace it to an original value. */ - frame->pc = tsk->ret_stack[frame->graph--].ret; + ret_stack =ftrace_graph_get_ret_stack (tsk, frame->graph++); +if (WARN_ON_ONCE (!ret_stack)) +return -EINVAL ; + frame->pc = ret_stack->ret; }#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
@@ -134,7 +136,7 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace) frame.fp = regs->regs[29 ]; frame.pc = regs->pc;#ifdef CONFIG_FUNCTION_GRAPH_TRACER - frame.graph = current->curr_ret_stack; + frame.graph =0 ;#endif walk_stackframe (current, &frame, save_trace, &data);
@@ -165,7 +167,7 @@ static noinline void __save_stack_trace(struct task_struct *tsk, frame.pc = (unsigned long )__save_stack_trace; }#ifdef CONFIG_FUNCTION_GRAPH_TRACER - frame.graph = tsk->curr_ret_stack; + frame.graph =0 ;#endif walk_stackframe (tsk, &frame, save_trace, &data);
arch/arm64/kernel/time.c +1 -1
@@ -52,7 +52,7 @@ unsigned long profile_pc(struct pt_regs *regs) frame.fp = regs->regs[29 ]; frame.pc = regs->pc;#ifdef CONFIG_FUNCTION_GRAPH_TRACER - frame.graph = current->curr_ret_stack; + frame.graph =0 ;#endif do { int ret = unwind_frame(NULL, &frame);
arch/arm64/kernel/traps.c +1 -1
@@ -122,7 +122,7 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk) frame.pc =thread_saved_pc (tsk); }#ifdef CONFIG_FUNCTION_GRAPH_TRACER - frame.graph = tsk->curr_ret_stack; + frame.graph =0 ;#endif skip = !!regs;
Signature · 8 lines
2.19.2 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Steven Rostedt
maintainerPATCH 10/24 seq_buf: Use size_t for len in seq_buf_puts()
In reply to Steven RostedtFrom: Michael Ellerman <[email protected]>
Jann Horn points out that we're using unsigned int for len in seq_buf_puts(), which could potentially overflow if we're passed a UINT_MAX sized string.
The rest of the code already uses size_t, so we should also use that in seq_buf_puts() to avoid any issues.
--- lib/seq_buf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/seq_buf.c +1 -1
@@ -140,7 +140,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)*/ int seq_buf_puts (struct seq_buf *s,const char *str) { -unsigned int len =strlen (str); +size_t len =strlen (str);WARN_ON (s->size ==0 );
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 11/24 tracing: Fix ftrace_graph_get_ret_stack() to use task and not current
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The function ftrace_graph_get_ret_stack() takes a task struct descriptor but uses current as the task to perform the operations on. In pretty much all cases the task decriptor is the same as current, so this wasn't an issue. But there is a case in the ARM architecture that passes in a task that is not current, and expects a result from that task, and this code breaks it.
--- kernel/trace/fgraph.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/trace/fgraph.c +2 -2
@@ -246,10 +246,10 @@ unsigned long ftrace_return_to_handler(unsigned long frame_pointer)struct ftrace_ret_stack *ftrace_graph_get_ret_stack (struct task_struct *task,int idx) { - idx = current->curr_ret_stack - idx; + idx = task->curr_ret_stack - idx;if (idx >=0 && idx <= task->curr_ret_stack) -return ¤t->ret_stack[idx]; +return &task->ret_stack[idx];return NULL ; }
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 12/24 tracing: Remove unnecessary hist trigger struct field
In reply to Steven RostedtFrom: Tom Zanussi <[email protected]>
hist_field.var_idx is completely unused, so remove it.
--- kernel/trace/trace_events_hist.c | 1 - 1 file changed, 1 deletion(-)
kernel/trace/trace_events_hist.c +0 -1
@@ -61,7 +61,6 @@ struct hist_field {char *system;char *event_name;char *name; -unsigned int var_idx;unsigned int var_ref_idx;bool read_once; };
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 13/24 tracing: Change strlen to sizeof for hist trigger static strings
In reply to Steven RostedtFrom: Tom Zanussi <[email protected]>
There's no need to use strlen() for static strings when the length is already known, so update trace_events_hist.c with sizeof() for those cases.
--- kernel/trace/trace_events_hist.c | 38 ++++++++++++++++---------------- 1 file changed, 19 insertions(+), 19 deletions(-)
kernel/trace/trace_events_hist.c +19 -19
@@ -507,7 +507,7 @@ static int synth_field_string_size(char *type) start =strstr (type,"char[" );if (start ==NULL )return -EINVAL ; - start +=strlen ("char[" ); + start +=sizeof ("char[" ) -1 ; end =strchr (type,']' );if (!end || end < start)
@@ -1843,8 +1843,8 @@ static int parse_action(char *str, struct hist_trigger_attrs *attrs)if (attrs->n_actions >=HIST_ACTIONS_MAX )return ret; -if ((strncmp (str,"onmatch(" ,strlen ("onmatch(" )) ==0 ) || - (strncmp (str,"onmax(" ,strlen ("onmax(" )) ==0 )) { +if ((strncmp (str,"onmatch(" ,sizeof ("onmatch(" ) -1 ) ==0 ) || + (strncmp (str,"onmax(" ,sizeof ("onmax(" ) -1 ) ==0 )) { attrs->action_str[attrs->n_actions] =kstrdup (str,GFP_KERNEL );if (!attrs->action_str[attrs->n_actions]) { ret = -ENOMEM ;
@@ -1861,34 +1861,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs) {int ret =0 ; -if ((strncmp (str,"key=" ,strlen ("key=" )) ==0 ) || - (strncmp (str,"keys=" ,strlen ("keys=" )) ==0 )) { +if ((strncmp (str,"key=" ,sizeof ("key=" ) -1 ) ==0 ) || + (strncmp (str,"keys=" ,sizeof ("keys=" ) -1 ) ==0 )) { attrs->keys_str =kstrdup (str,GFP_KERNEL );if (!attrs->keys_str) { ret = -ENOMEM ; goto out; } - }else if ((strncmp (str,"val=" ,strlen ("val=" )) ==0 ) || - (strncmp (str,"vals=" ,strlen ("vals=" )) ==0 ) || - (strncmp (str,"values=" ,strlen ("values=" )) ==0 )) { + }else if ((strncmp (str,"val=" ,sizeof ("val=" ) -1 ) ==0 ) || + (strncmp (str,"vals=" ,sizeof ("vals=" ) -1 ) ==0 ) || + (strncmp (str,"values=" ,sizeof ("values=" ) -1 ) ==0 )) { attrs->vals_str =kstrdup (str,GFP_KERNEL );if (!attrs->vals_str) { ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"sort=" ,strlen ("sort=" )) ==0 ) { + }else if (strncmp (str,"sort=" ,sizeof ("sort=" ) -1 ) ==0 ) { attrs->sort_key_str =kstrdup (str,GFP_KERNEL );if (!attrs->sort_key_str) { ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"name=" ,strlen ("name=" )) ==0 ) { + }else if (strncmp (str,"name=" ,sizeof ("name=" ) -1 ) ==0 ) { attrs->name =kstrdup (str,GFP_KERNEL );if (!attrs->name) { ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"clock=" ,strlen ("clock=" )) ==0 ) { + }else if (strncmp (str,"clock=" ,sizeof ("clock=" ) -1 ) ==0 ) {strsep (&str,"=" );if (!str) { ret = -EINVAL ;
@@ -1901,7 +1901,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs) ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"size=" ,strlen ("size=" )) ==0 ) { + }else if (strncmp (str,"size=" ,sizeof ("size=" ) -1 ) ==0 ) {int map_bits =parse_map_size (str);if (map_bits <0 ) {
@@ -3497,7 +3497,7 @@ static struct action_data *onmax_parse(char *str)if (!onmax_fn_name || !str) goto free; -if (strncmp (onmax_fn_name,"save" ,strlen ("save" )) ==0 ) { +if (strncmp (onmax_fn_name,"save" ,sizeof ("save" ) -1 ) ==0 ) {char *params =strsep (&str,")" );if (!params) {
@@ -4302,8 +4302,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)for (i =0 ; i < hist_data->attrs->n_actions; i++) { str = hist_data->attrs->action_str[i]; -if (strncmp (str,"onmatch(" ,strlen ("onmatch(" )) ==0 ) { -char *action_str = str +strlen ("onmatch(" ); +if (strncmp (str,"onmatch(" ,sizeof ("onmatch(" ) -1 ) ==0 ) { +char *action_str = str +sizeof ("onmatch(" ) -1 ; data =onmatch_parse (tr, action_str);if (IS_ERR (data)) {
@@ -4311,8 +4311,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)break ; } data->fn = action_trace; - }else if (strncmp (str,"onmax(" ,strlen ("onmax(" )) ==0 ) { -char *action_str = str +strlen ("onmax(" ); + }else if (strncmp (str,"onmax(" ,sizeof ("onmax(" ) -1 ) ==0 ) { +char *action_str = str +sizeof ("onmax(" ) -1 ; data =onmax_parse (action_str);if (IS_ERR (data)) {
@@ -5548,9 +5548,9 @@ static int event_hist_trigger_func(struct event_command *cmd_ops, p++;continue ; } -if (p >= param +strlen (param) -strlen ("if" ) -1 ) +if (p >= param +strlen (param) - (sizeof ("if" ) -1 ) -1 )return -EINVAL ; -if (*(p +strlen ("if" )) !=' ' && *(p +strlen ("if" )) !='\t' ) { +if (*(p +sizeof ("if" ) -1 ) !=' ' && *(p +sizeof ("if" ) -1 ) !='\t' ) { p++;continue ; }
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 14/24 tracing: Use var_refs[] for hist trigger reference checking
In reply to Steven RostedtFrom: Tom Zanussi <[email protected]>
Since all the variable reference hist_fields are collected into hist_data->var_refs[] array, there's no need to go through all the fields looking for them, or in separate arrays like synth_var_refs[], which will be going away soon anyway.
This also allows us to get rid of some unnecessary code and functions currently used for the same purpose.
--- kernel/trace/trace_events_hist.c | 68 ++++++-------------------------- 1 file changed, 11 insertions(+), 57 deletions(-)
kernel/trace/trace_events_hist.c +11 -57
@@ -1297,75 +1297,29 @@ check_field_for_var_ref(struct hist_field *hist_field,struct hist_trigger_data *var_data, unsigned int var_idx) { -struct hist_field *found =NULL ; - -if (hist_field && hist_field->flags &HIST_FIELD_FL_VAR_REF ) { -if (hist_field->var.idx == var_idx && - hist_field->var.hist_data == var_data) { - found = hist_field; - } - } - -return found; -} - -static struct hist_field * -check_field_for_var_refs (struct hist_trigger_data *hist_data, -struct hist_field *hist_field, -struct hist_trigger_data *var_data, -unsigned int var_idx, -unsigned int level) -{ -struct hist_field *found =NULL ; -unsigned int i; - -if (level >3 ) -return found; - -if (!hist_field) -return found; - - found =check_field_for_var_ref (hist_field, var_data, var_idx); -if (found) -return found; - -for (i =0 ; i <HIST_FIELD_OPERANDS_MAX ; i++) { -struct hist_field *operand; + WARN_ON(!(hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF)); - operand = hist_field->operands[i]; - found =check_field_for_var_refs (hist_data, operand, var_data, - var_idx, level +1 ); -if (found) -return found; - } +if (hist_field && hist_field->var.idx == var_idx && + hist_field->var.hist_data == var_data) +return hist_field; -return found; +return NULL ; }static struct hist_field *find_var_ref (struct hist_trigger_data *hist_data,struct hist_trigger_data *var_data,unsigned int var_idx) { -struct hist_field *hist_field, *found =NULL ; +struct hist_field *hist_field;unsigned int i; -for_each_hist_field (i, hist_data) { - hist_field = hist_data->fields[i]; - found =check_field_for_var_refs (hist_data, hist_field, - var_data, var_idx,0 ); -if (found) -return found; - } - -for (i =0 ; i < hist_data->n_synth_var_refs; i++) { - hist_field = hist_data->synth_var_refs[i]; - found =check_field_for_var_refs (hist_data, hist_field, - var_data, var_idx,0 ); -if (found) -return found; +for (i =0 ; i < hist_data->n_var_refs; i++) { + hist_field = hist_data->var_refs[i]; +if (check_field_for_var_ref (hist_field, var_data, var_idx)) +return hist_field; } -return found; +return NULL ; } static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 15/24 tracing: Remove open-coding of hist trigger var_ref management
In reply to Steven RostedtFrom: Tom Zanussi <[email protected]>
Have create_var_ref() manage the hist trigger's var_ref list, rather than having similar code doing it in multiple places. This cleans up the code and makes sure var_refs are always accounted properly.
Also, document the var_ref-related functions to make what their purpose clearer.
--- kernel/trace/trace_events_hist.c | 93 +++++++++++++++++++++++++------- 1 file changed, 75 insertions(+), 18 deletions(-)
kernel/trace/trace_events_hist.c +75 -18
@@ -1292,6 +1292,17 @@ static u64 hist_field_cpu(struct hist_field *hist_field,return cpu; } +/** +* check_field_for_var_ref - Check if a VAR_REF field references a variable +* @hist_field: The VAR_REF field to check +* @var_data: The hist trigger that owns the variable +* @var_idx: The trigger variable identifier +* +* Check the given VAR_REF field to see whether or not it references +* the given variable associated with the given trigger. +* +* Return: The VAR_REF field if it does reference the variable, NULL if not +*/ static struct hist_field * check_field_for_var_ref(struct hist_field *hist_field, struct hist_trigger_data *var_data,
@@ -1306,6 +1317,18 @@ check_field_for_var_ref(struct hist_field *hist_field,return NULL ; } +/** +* find_var_ref - Check if a trigger has a reference to a trigger variable +* @hist_data: The hist trigger that might have a reference to the variable +* @var_data: The hist trigger that owns the variable +* @var_idx: The trigger variable identifier +* +* Check the list of var_refs[] on the first hist trigger to see +* whether any of them are references to the variable on the second +* trigger. +* +* Return: The VAR_REF field referencing the variable if so, NULL if not +*/ static struct hist_field *find_var_ref (struct hist_trigger_data *hist_data,struct hist_trigger_data *var_data,unsigned int var_idx)
@@ -1322,6 +1345,20 @@ static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,return NULL ; } +/** +* find_any_var_ref - Check if there is a reference to a given trigger variable +* @hist_data: The hist trigger +* @var_idx: The trigger variable identifier +* +* Check to see whether the given variable is currently referenced by +* any other trigger. +* +* The trigger the variable is defined on is explicitly excluded - the +* assumption being that a self-reference doesn't prevent a trigger +* from being removed. +* +* Return: The VAR_REF field referencing the variable if so, NULL if not +*/ static struct hist_field *find_any_var_ref (struct hist_trigger_data *hist_data,unsigned int var_idx) {
@@ -1340,6 +1377,19 @@ static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,return found; } +/** +* check_var_refs - Check if there is a reference to any of trigger's variables +* @hist_data: The hist trigger +* +* A trigger can define one or more variables. If any one of them is +* currently referenced by any other trigger, this function will +* determine that. + +* Typically used to determine whether or not a trigger can be removed +* - if there are any references to a trigger's variables, it cannot. +* +* Return: True if there is a reference to any of trigger's variables +*/ static bool check_var_refs (struct hist_trigger_data *hist_data) {struct hist_field *field;
@@ -2343,7 +2393,23 @@ static int init_var_ref(struct hist_field *ref_field, goto out; } -static struct hist_field *create_var_ref (struct hist_field *var_field, +/** +* create_var_ref - Create a variable reference and attach it to trigger +* @hist_data: The trigger that will be referencing the variable +* @var_field: The VAR field to create a reference to +* @system: The optional system string +* @event_name: The optional event_name string +* +* Given a variable hist_field, create a VAR_REF hist_field that +* represents a reference to it. +* +* This function also adds the reference to the trigger that +* now references the variable. +* +* Return: The VAR_REF field if successful, NULL if not +*/ +static struct hist_field *create_var_ref (struct hist_trigger_data *hist_data, +struct hist_field *var_field,char *system,char *event_name) {unsigned long flags =HIST_FIELD_FL_VAR_REF ;
@@ -2355,6 +2421,9 @@ static struct hist_field *create_var_ref(struct hist_field *var_field,destroy_hist_field (ref_field,0 );return NULL ; } + + hist_data->var_refs[hist_data->n_var_refs] = ref_field; + ref_field->var_ref_idx = hist_data->n_var_refs++; }return ref_field;
@@ -2428,7 +2497,8 @@ static struct hist_field *parse_var_ref(struct hist_trigger_data *hist_data, var_field =find_event_var (hist_data, system, event_name, var_name);if (var_field) - ref_field =create_var_ref (var_field, system, event_name); + ref_field =create_var_ref (hist_data, var_field, + system, event_name);if (!ref_field) hist_err_event("Couldn't find variable: $",
@@ -2546,8 +2616,6 @@ static struct hist_field *parse_atom(struct hist_trigger_data *hist_data,if (!s) { hist_field =parse_var_ref (hist_data, ref_system, ref_event, ref_var);if (hist_field) { - hist_data->var_refs[hist_data->n_var_refs] = hist_field; - hist_field->var_ref_idx = hist_data->n_var_refs++;if (var_name) { hist_field =create_alias (hist_data, hist_field, var_name);if (!hist_field) {
@@ -3321,7 +3389,6 @@ static int onmax_create(struct hist_trigger_data *hist_data,unsigned int var_ref_idx = hist_data->n_var_refs;struct field_var *field_var;char *onmax_var_str, *param; -unsigned long flags;unsigned int i;int ret =0 ;
@@ -3338,18 +3405,10 @@ static int onmax_create(struct hist_trigger_data *hist_data,return -EINVAL ; } - flags =HIST_FIELD_FL_VAR_REF ; - ref_field =create_hist_field (hist_data,NULL , flags,NULL ); + ref_field =create_var_ref (hist_data, var_field,NULL ,NULL );if (!ref_field)return -ENOMEM ; -if (init_var_ref (ref_field, var_field,NULL ,NULL )) { -destroy_hist_field (ref_field,0 ); - ret = -ENOMEM ; - goto out; - } - hist_data->var_refs[hist_data->n_var_refs] = ref_field; - ref_field->var_ref_idx = hist_data->n_var_refs++; data->onmax.var = ref_field; data->fn = onmax_save;
@@ -3538,9 +3597,6 @@ static void save_synth_var_ref(struct hist_trigger_data *hist_data, struct hist_field *var_ref) { hist_data->synth_var_refs[hist_data->n_synth_var_refs++] = var_ref; - - hist_data->var_refs[hist_data->n_var_refs] = var_ref; - var_ref->var_ref_idx = hist_data->n_var_refs++; } static int check_synth_field(struct synth_event *event,
@@ -3694,7 +3750,8 @@ static int onmatch_create(struct hist_trigger_data *hist_data, }if (check_synth_field (event, hist_field, field_pos) ==0 ) { - var_ref =create_var_ref (hist_field, system, event_name); + var_ref =create_var_ref (hist_data, hist_field, + system, event_name);if (!var_ref) {kfree (p); ret = -ENOMEM ;
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 16/24 tracing: Use hist triggers var_ref array to destroy var_refs
In reply to Steven RostedtFrom: Tom Zanussi <[email protected]>
Since every var ref for a trigger has an entry in the var_ref[] array, use that to destroy the var_refs, instead of piecemeal via the field expressions.
This allows us to avoid having to keep and treat differently separate lists for the action-related references, which future patches will remove.
--- kernel/trace/trace_events_hist.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
kernel/trace/trace_events_hist.c +19 -5
@@ -2190,6 +2190,15 @@ static int contains_operator(char *str)return field_op; } +static void __destroy_hist_field (struct hist_field *hist_field) +{ +kfree (hist_field->var.name); +kfree (hist_field->name); +kfree (hist_field->type); + +kfree (hist_field); +} +static void destroy_hist_field (struct hist_field *hist_field,unsigned int level) {
@@ -2201,14 +2210,13 @@ static void destroy_hist_field(struct hist_field *hist_field,if (!hist_field)return ; +if (hist_field->flags &HIST_FIELD_FL_VAR_REF ) +return ;/* var refs will be destroyed separately */ +for (i =0 ; i <HIST_FIELD_OPERANDS_MAX ; i++)destroy_hist_field (hist_field->operands[i], level +1 ); -kfree (hist_field->var.name); -kfree (hist_field->name); -kfree (hist_field->type); - -kfree (hist_field); +__destroy_hist_field (hist_field); } static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
@@ -2335,6 +2343,12 @@ static void destroy_hist_fields(struct hist_trigger_data *hist_data) hist_data->fields[i] =NULL ; } } + +for (i =0 ; i < hist_data->n_var_refs; i++) { +WARN_ON (!(hist_data->var_refs[i]->flags &HIST_FIELD_FL_VAR_REF )); +__destroy_hist_field (hist_data->var_refs[i]); + hist_data->var_refs[i] =NULL ; + } } static int init_var_ref(struct hist_field *ref_field,
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 17/24 tracing: Remove hist trigger synth_var_refs
In reply to Steven RostedtFrom: Tom Zanussi <[email protected]>
All var_refs are now handled uniformly and there's no reason to treat the synth_refs in a special way now, so remove them and associated functions.
--- kernel/trace/trace_events_hist.c | 18 ------------------ 1 file changed, 18 deletions(-)
kernel/trace/trace_events_hist.c +0 -18
@@ -279,8 +279,6 @@ struct hist_trigger_data {struct action_data *actions[HIST_ACTIONS_MAX ];unsigned int n_actions; -struct hist_field *synth_var_refs[SYNTH_FIELDS_MAX ]; -unsigned int n_synth_var_refs;struct field_var *field_vars[SYNTH_FIELDS_MAX ];unsigned int n_field_vars;unsigned int n_field_var_str;
@@ -3599,20 +3597,6 @@ static void save_field_var(struct hist_trigger_data *hist_data, } -static void destroy_synth_var_refs (struct hist_trigger_data *hist_data) -{ -unsigned int i; - -for (i =0 ; i < hist_data->n_synth_var_refs; i++) -destroy_hist_field (hist_data->synth_var_refs[i],0 ); -} - -static void save_synth_var_ref (struct hist_trigger_data *hist_data, -struct hist_field *var_ref) -{ - hist_data->synth_var_refs[hist_data->n_synth_var_refs++] = var_ref; -} -static int check_synth_field (struct synth_event *event,struct hist_field *hist_field,unsigned int field_pos)
@@ -3772,7 +3756,6 @@ static int onmatch_create(struct hist_trigger_data *hist_data, goto err; } -save_synth_var_ref (hist_data, var_ref); field_pos++;kfree (p);continue ;
@@ -4516,7 +4499,6 @@ static void destroy_hist_data(struct hist_trigger_data *hist_data)destroy_actions (hist_data);destroy_field_vars (hist_data);destroy_field_var_hists (hist_data); -destroy_synth_var_refs (hist_data);kfree (hist_data); }
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 18/24 tracing: Add hist trigger comments for variable-related fields
In reply to Steven RostedtFrom: Tom Zanussi <[email protected]>
Add a few comments to help clarify how variable and variable reference fields are used in the code.
--- kernel/trace/trace_events_hist.c | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
kernel/trace/trace_events_hist.c +36 -0
@@ -40,6 +40,16 @@ enum field_op_id { FIELD_OP_UNARY_MINUS, }; +/* +* A hist_var (histogram variable) contains variable information for +* hist_fields having the HIST_FIELD_FL_VAR or HIST_FIELD_FL_VAR_REF +* flag set. A hist_var has a variable name e.g. ts0, and is +* associated with a given histogram trigger, as specified by +* hist_data. The hist_var idx is the unique index assigned to the +* variable by the hist trigger's tracing_map. The idx is what is +* used to set a variable's value and, by a variable reference, to +* retrieve it. +*/ struct hist_var {char *name;struct hist_trigger_data *hist_data;
@@ -56,11 +66,29 @@ struct hist_field {const char *type;struct hist_field *operands[HIST_FIELD_OPERANDS_MAX ];struct hist_trigger_data *hist_data; + +/* +* Variable fields contain variable-specific info in var. +*/ struct hist_var var;enum field_op_id operator;char *system;char *event_name; + +/* +* The name field is used for EXPR and VAR_REF fields. VAR +* fields contain the variable name in var.name. +*/ char *name; + +/* +* When a histogram trigger is hit, if it has any references +* to variables, the values of those variables are collected +* into a var_ref_vals array by resolve_var_refs(). The +* current value of each variable is read from the tracing_map +* using the hist field's hist_var.idx and entered into the +* var_ref_idx entry i.e. var_ref_vals[var_ref_idx]. +*/ unsigned int var_ref_idx;bool read_once; };
@@ -365,6 +393,14 @@ struct action_data {union {struct { +/* +* When a histogram trigger is hit, the values of any +* references to variables, including variables being passed +* as parameters to synthetic events, are collected into a +* var_ref_vals array. This var_ref_idx is the index of the +* first param in the array to be passed to the synthetic +* event invocation. +*/ unsigned int var_ref_idx;char *match_event;char *match_event_system;
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 19/24 tracing: Merge seq_print_sym_short() and seq_print_sym_offset()
In reply to Steven RostedtFrom: Rasmus Villemoes <[email protected]>
These two functions are nearly identical, so we can avoid some code duplication by moving the conditional into a common implementation.
--- kernel/trace/trace_output.c | 34 +++++++--------------------------- 1 file changed, 7 insertions(+), 27 deletions(-)
kernel/trace/trace_output.c +7 -27
@@ -339,34 +339,17 @@ static inline const char *kretprobed(const char *name)#endif /* CONFIG_KRETPROBES */ static void -seq_print_sym_short (struct trace_seq *s,const char *fmt,unsigned long address) +seq_print_sym (struct trace_seq *s,const char *fmt,unsigned long address, +bool offset) {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); -return ; - } -#endif -snprintf (str,KSYM_SYMBOL_LEN ,"0x%08lx" , address); -trace_seq_printf (s, fmt, str); -} - -static void -seq_print_sym_offset (struct trace_seq *s,const char *fmt, -unsigned long address) -{ -char str[KSYM_SYMBOL_LEN ]; -#ifdef CONFIG_KALLSYMS -const char *name; - -sprint_symbol (str, address); +if (offset) +sprint_symbol (str, address); +else +kallsyms_lookup (address,NULL ,NULL ,NULL , str); name =kretprobed (str);if (name &&strlen (name)) {
@@ -424,10 +407,7 @@ seq_print_ip_sym(struct trace_seq *s, unsigned long ip, unsigned long sym_flags) goto out; } -if (sym_flags &TRACE_ITER_SYM_OFFSET ) -seq_print_sym_offset (s,"%s" , ip); -else -seq_print_sym_short (s,"%s" , ip); +seq_print_sym (s,"%s" , ip, sym_flags &TRACE_ITER_SYM_OFFSET );if (sym_flags &TRACE_ITER_SYM_ADDR )trace_seq_printf (s," <" IP_FMT ">" , ip);
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 21/24 tracing: Simplify printfing in seq_print_sym
In reply to Steven RostedtFrom: Rasmus Villemoes <[email protected]>
trace_seq_printf(..., "%s", ...) can be done with trace_seq_puts() instead, avoiding printf overhead. In the second instance, the string we're copying was just created from an snprintf() to a stack buffer, so we might as well do that printf directly. This naturally leads to moving the declaration of the str buffer inside the CONFIG_KALLSYMS guard, which in turn will make gcc inline the function for !CONFIG_KALLSYMS (it only has a single caller, but the huge stack frame seems to make gcc not inline it for CONFIG_KALLSYMS).
--- kernel/trace/trace_output.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
kernel/trace/trace_output.c +3 -4
@@ -341,8 +341,8 @@ static inline const char *kretprobed(const char *name)static void seq_print_sym (struct trace_seq *s,unsigned long address,bool offset) { -char str[KSYM_SYMBOL_LEN ];#ifdef CONFIG_KALLSYMS +char str[KSYM_SYMBOL_LEN ];const char *name;if (offset)
@@ -352,12 +352,11 @@ seq_print_sym(struct trace_seq *s, unsigned long address, bool offset) name =kretprobed (str);if (name &&strlen (name)) { -trace_seq_printf (s,"%s" , name); +trace_seq_puts (s, name);return ; }#endif -snprintf (str,KSYM_SYMBOL_LEN ,"0x%08lx" , address); -trace_seq_printf (s,"%s" , str); +trace_seq_printf (s,"0x%08lx" , address); }#ifndef CONFIG_64BIT
Signature · 1 line
2.19.2
Steven Rostedt
maintainerPATCH 23/24 string.h: Add strncmp_prefix() helper macro
In reply to Steven RostedtFrom: Steven Rostedt <[email protected]>
A discussion came up in the trace triggers thread about converting a bunch of:
strncmp(str, "const", sizeof("const") - 1)
use cases into a helper macro. It started with:
strncmp(str, const, sizeof(const) - 1)
But then Joe Perches mentioned that if a const is not used, the sizeof() will be the size of a pointer, which can be bad. And that gcc will optimize strlen("const") into "sizeof("const") - 1".
Thinking about this more, a quick grep in the kernel tree found several (thousands!) of cases that use this construct. A quick grep also revealed that there's probably several bugs in that use case. Some are that people forgot the "- 1" (which I found) and others could be that the constant for the sizeof is different than the constant (although, I haven't found any of those, but I also didn't look hard).
I figured the best thing to do is to create a helper macro and place it into include/linux/string.h. And go around and fix all the open coded versions of it later.
I plan on only applying this patch and updating the tracing hooks for this merge window. And perhaps use it to fix some of the bugs that were found.
I was going to just use:
strncmp(str, prefix, strlen(prefix))
but then realized that "prefix" is used twice, and will break if someone does something like:
strncmp_prefix(str, ptr++);
So instead I check with __builtin_constant_p() to see if the second parameter is truly a constant, which I use sizeof() anyway (why bother gcc to optimize it, if we already know it's a constant), and copy the parameter into a local variable and use that local variable for the non constant part (with strlen).
--- include/linux/string.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
include/linux/string.h +22 -0
@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);extern void *vmemdup_user (const void __user *,size_t );extern void *memdup_user_nul (const void __user *,size_t ); +/* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use strncmp_prefix(). +*/ +#define strncmp_prefix (str, prefix) \ + ({ \ + int ____strcmp_prefix_ret____; \ + if (__builtin_constant_p(&prefix)) { \ + ____strcmp_prefix_ret____ = \ + strncmp(str, prefix, sizeof(prefix) - 1); \ + } else { \ + typeof(prefix) ____strcmp_prefix____ = prefix; \ + ____strcmp_prefix_ret____ = \ + strncmp(str, ____strcmp_prefix____, \ + strlen(____strcmp_prefix____)); \ + } \ + ____strcmp_prefix_ret____; \ + }) +/* * Include machine specific inline routines */
Signature · 1 line
2.19.2
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 8 lines · Steven Rostedt
I figured the best thing to do is to create a helper macro and place it into
include/linux/string.h. And go around and fix all the open coded versions of it later.I plan on only applying this patch and updating the tracing hooks for this merge window. And perhaps use it to fix some of the bugs that were found.
I like the helper function concept, but as they say about CS: "There is only one hard problem in computer science: naming and off-by-one errors".
And this one has that problem. The name makes absolutely no sense.
Calling it "strncmp_prefix()" when there is no "n" there makes no sense.
So drop the "n" from the name.
And honestly, maybe it would be better to use a different naming pattern entirely, and avoid the crazy non-boolean "str*cmp()" model. That thing is useful for search comparisons (where "before/after" matters), but it's horrible for the actual "is this true or not", where the code ends up being
if (!strcmp_prefix(a, "prefix")) {
// This is the "Yes, prefix matched" case, despite the"if !" syntax
which is just confusing.
So I'd really prefer more of a
#define has_prefix(str, prefix) ...
model that actually returns a boolean (true if it has a prefix, false if it doesn't), rather than use the "str*cmp" naming and return values.
(But I agree that *if* you use the "strcmp" naming, then you do need to hold to the traditional strcmp return value semantics).
Hmm?
Finally, I also suspect that your helper might be slightly fragile. Doing sizeof() seems broken. I could see somebody using some prefix define for arrays with constant sizes, and doing
#define PFX1 "cpp\0" #define PFX2 "c\0\0\0" #define PFX3 "h\0\0\0"
or similar. Also, is there a reason you use "&prefix" for the constant test? I don't see that pattern anywhere else. Plus it looks entirely wrong without the parenthesis (ie "&(prefix)" to make sure we group things right).
So a lot of small problems, but the naming one is big.
Linus
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 10 lines · On Fri, 21 Dec 2018 10:51:29 -0800 Linus Torvalds…
Quoted 8 lines · Steven Rostedt
I figured the best thing to do is to create a helper macro and place it into
include/linux/string.h. And go around and fix all the open coded versions of it later.I plan on only applying this patch and updating the tracing hooks for this merge window. And perhaps use it to fix some of the bugs that were found.
Quoted 5 lines
I like the helper function concept, but as they say about CS: "There is only one hard problem in computer science: naming and off-by-one errors".
And this one has that problem. The name makes absolutely no sense.
Good thing I rebased and put these patches at the end before running my tests :-) (Specifically because I was expecting to have feedback like this).
Quoted 4 lines
Calling it "
strncmp_prefix()" when there is no "n" there makes no sense.So drop the "n" from the name.
I originally did that, but then I thought strcmp() is a full compare, and 'n' denotes it is partial. But thinking about it more, yeah one would think it would need a parameter to represent 'n'.
Quoted 9 lines
And honestly, maybe it would be better to use a different naming pattern entirely, and avoid the crazy non-boolean "str*cmp()" model. That thing is useful for search comparisons (where "before/after" matters), but it's horrible for the actual "is this true or not", where the code ends up being
if (!strcmp_prefix(a, "prefix")) { // This is the "Yes, prefix matched" case, despite the
Quoted 7 lines
"if !" syntax
which is just confusing.
So I'd really prefer more of a
#define has_prefix(str, prefix) ...
I like changing the name.
Quoted 4 lines
model that actually returns a boolean (true if it has a prefix, false if it doesn't), rather than use the "str*cmp" naming and return values.
Actually, instead of returning a bool, it can return the length if it matches.
Reason being, there's several locations that does something like:
while (...) {
len = strlen("const");
if (strncmp(str, "const", len) == 0) {
str += len;
break;
}
}And I was having trouble thinking about how to deal with these. But if we have a has_prefix() that returns the length on match then we can do:
if ((len = has_prefix(str, "const")) {
str += len;Quoted 10 lines
(But I agree that *if* you use the "strcmp" naming, then you do need to hold to the traditional strcmp return value semantics).
Hmm?
Finally, I also suspect that your helper might be slightly fragile. Doing
sizeof()seems broken. I could see somebody using some prefix define for arrays with constant sizes, and doing
Quoted 5 lines
#define PFX1 "cpp\0" #define PFX2 "c\0\0\0" #define PFX3 "h\0\0\0"or similar. Also, is there a reason you use "&prefix" for the constant
That was left over in my original tests in userspace. When I first tried it with __builtin_constant_p() I got an error, and added the '&', but then fixed something else. The something else was what actually caused the error, but since it didn't complain (and past all my tests), I left in the '&'.
Quoted 5 lines
test? I don't see that pattern anywhere else. Plus it looks entirely wrong without the parenthesis (ie "&(prefix)" to make sure we group things right).
So a lot of small problems, but the naming one is big.
OK, what about if we just use strlen() and say that this macro is not safe for parameters with side effects.
-- Steve
include/linux/string.h +23 -0
@@ -14,6 +14,29 @@ extern void *memdup_user(const void __user *, size_t);extern void *vmemdup_user (const void __user *,size_t );extern void *memdup_user_nul (const void __user *,size_t ); +/** +* have_prefix - Test if a string has a given prefix +* @str: The string to test +* @prefix: The string to see if @str starts with +* +* IMPORTANT; @prefix must not have side effects (used more than once here) +* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use has_prefix(). +* +* Returns: 0 if @str does not start with @prefix +strlen(@prefix) if @str does start with @prefix +*/ +#define has_prefix (str, prefix) \ + ({ \ + int ____strcmp_prefix_len____ = strlen(prefix); \ + strncmp(str, prefix, ____strcmp_prefix_len____) == 0 ? \ + ____strcmp_prefix_len____ : 0; \ + }) +/* * Include machine specific inline routines */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 3 lines · Steven Rostedt
OK, what about if we just use
strlen()and say that this macro is not safe for parameters with side effects.
I think gcc follows simple assignments just fine, and does the optimized strlen() for them too.
So why not just do
#define have_prefix(str,prefix) ({ \
const char *__pfx = prefix; \
size_t __pfxlen = strlen(__pfx); \
strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); })and be done with it safely?
The above is ENTIRELY untested.
Linus
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 10 lines · On Fri, 21 Dec 2018 12:01:48 -0800 Linus Torvalds…
Quoted 3 lines · Steven Rostedt
OK, what about if we just use
strlen()and say that this macro is not safe for parameters with side effects.I think gcc follows simple assignments just fine, and does the optimized
strlen()for them too.So why not just do
Quoted 9 lines
#define have_prefix(str,prefix) ({ \ const char *__pfx = prefix; \ size_t __pfxlen = strlen(__pfx); \ strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); })and be done with it safely?
The above is ENTIRELY untested.
At first I thought this would have issues, but with a slight change...
#define have_prefix(str, prefix) ({ \
const char *__pfx = (const char *)prefix; \And the rest the same, it appears to work.
Need the cast because if for some reason someone passed in something like "const unsigned char" then it wouldn't work. But that's just a nit.
So something like this then?
-- Steve
include/linux/string.h +22 -0
@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);extern void *vmemdup_user (const void __user *,size_t );extern void *memdup_user_nul (const void __user *,size_t ); +/** +* have_prefix - Test if a string has a given prefix +* @str: The string to test +* @prefix: The string to see if @str starts with +* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use has_prefix(). +* +* Returns: 0 if @str does not start with @prefix +strlen(@prefix) if @str does start with @prefix +*/ +#define has_prefix (str, prefix) \ + ({ \ + const char *____prefix____ = (const char *)(prefix); \ + int ____len____ = strlen(____prefix____); \ + strncmp(str, ____prefix____, ____len____) == 0 ? \ + ____len____ : 0; \ + }) +/* * Include machine specific inline routines */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
include/linux/string.h
Quoted 16 lines · Steven Rostedt
At first I thought this would have issues, but with a slight change...
#define have_prefix(str, prefix) ({ \ const char *__pfx = (const char *)prefix; \And the rest the same, it appears to work.
Need the cast because if for some reason someone passed in something like "const unsigned char" then it wouldn't work. But that's just a nit.
So something like this then?
-- Steve
include/linux/string.h +0
-0
[]
include/linux/string.h extern void *memdup_user(const void __user *, size_t); · line 18
Quoted 6 lines
include/linux/string.h +2
-0@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);extern void *vmemdup_user (const void __user *,size_t );extern void *memdup_user_nul (const void __user *,size_t ); +/** +* have_prefix - Test if a string has a given prefix
There is a naming mismatch of have/has here
and has_prefix is probably too generic a name.
How about str_has_prefix?
Quoted 9 lines
include/linux/string.h +9
-0+* @str: The string to test +* @prefix: The string to see if @str starts with +* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use has_prefix(). +*
include/linux/string.h extern void *memdup_user(const void __user *, size_t); · line 37
Quoted 10 lines
include/linux/string.h +10
-0+* Returns: 0 if @str does not start with @prefix +strlen(@prefix) if @str does start with @prefix +*/ +#define has_prefix (str, prefix) \ + ({ \ + const char *____prefix____ = (const char *)(prefix); \ + int ____len____ = strlen(____prefix____); \ + strncmp(str, ____prefix____, ____len____) == 0 ? \ + ____len____ : 0; \ + })
I think all the underscores are unnecessary and confusing.
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 11 lines · On Fri, 21 Dec 2018 12:46:47 -0800 Joe Perches <[email protected]> wrote:
include/linux/string.h extern void *memdup_user(const void __user *, size_t); · line 18
Quoted 6 lines
include/linux/string.h +2
-0@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);extern void *vmemdup_user (const void __user *,size_t );extern void *memdup_user_nul (const void __user *,size_t ); +/** +* have_prefix - Test if a string has a given prefix There is a naming mismatch of have/has here
andhas_prefixis probably too generic a name.How about
str_has_prefix?
Sure.
Quoted 14 lines
Quoted 14 lines
include/linux/string.h +13
-0+* @str: The string to test +* @prefix: The string to see if @str starts with +* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use has_prefix(). +* +* Returns: 0 if @str does not start with @prefix +strlen(@prefix) if @str does start with @prefix +*/ +#define has_prefix (str, prefix) \
Quoted 9 lines
include/linux/string.h extern void *memdup_user(const void __user *, size_t); · line 37
Quoted 6 lines
include/linux/string.h +6
-0+ ({ \ +const char *____prefix____ = (const char *)(prefix); \ +int ____len____ =strlen (____prefix____); \ +strncmp (str, ____prefix____, ____len____) ==0 ? \ + ____len____ :0 ; \ + })I think all the underscores are unnecessary and confusing.
Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question.
I don't find them confusing ;-) But I tend to use a lot of macros.
-- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 26 lines · Steven Rostedt
Quoted 13 lines · On Fri, 21 Dec 2018 12:46:47 -0800 Joe Perches <[email protected]> wrote:
Quoted 13 lines
+ * @str: The string to test
+ * @prefix: The string to see if @str starts with
+ *
+ * A common way to test a prefix of a string is to do:
+ *strncmp(str, prefix, sizeof(prefix) - 1)
+ *
+ * But this can lead to bugs due to typos, or if prefix is a pointer
+ * and not a constant. Instead usehas_prefix().
+ *
+ * Returns: 0 if @str does not start with @prefixinclude/linux/string.h +3
-0+strlen(@prefix) if @str does start with @prefix +*/ +#define has_prefix (str, prefix) \Quoted 9 lines
include/linux/string.h extern void *memdup_user(const void __user *, size_t); · line 37
Quoted 6 lines
include/linux/string.h +6
-0+ ({ \ +const char *____prefix____ = (const char *)(prefix); \ +int ____len____ =strlen (____prefix____); \ +strncmp (str, ____prefix____, ____len____) ==0 ? \ + ____len____ :0 ; \ + })I think all the underscores are unnecessary and confusing.
Quoted 2 lines
Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question.
Why not make it an inline function?
Andreas.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 4 lines · On Fri, 21 Dec 2018 21:58:32 +0100 Andreas Schwab <[email protected]> wrote:
Quoted 2 lines
Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question.
Why not make it an inline function?
Matters if that removes the strlen(const) optimization. I could try it and see what happens.
-- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 11 lines · Steven Rostedt
Quoted 4 lines · On Fri, 21 Dec 2018 21:58:32 +0100 Andreas Schwab <[email protected]> wrote:
Quoted 2 lines
Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question.
Why not make it an inline function?
Matters if that removes the strlen(const) optimization. I could try it and see what happens.
Using
static inline bool str_has_prefix(const char *str, const char prefix[])
{
return !strncmp(str, prefix, strlen(prefix));
}eliminates the strlen with gcc 4.8 (oldest I still have)
$ cat lib/test_module.c * This module emits "Hello, world" on printk when loaded. * * It is designed to be used for basic evaluation of the module loading * subsystem (for example when validating module signing/verification). It * lacks any extra dependencies, and will not normally be loaded by the * system unless explicitly requested by name. */
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/init.h>
#include <linux/module.h>
#include <linux/printk.h>
static inline bool str_has_prefix(const char *str, const char prefix[])
{
return !strncmp(str, prefix, strlen(prefix));
}bool test_str_has_prefix(const char *foo)
{
return str_has_prefix("TomJonesPrefix", foo);
}bool test_str_has_prefix_TomJones(void)
{
return str_has_prefix("TomJonesPrefix", "TomJones");
}$ make CC=/usr/bin/gcc-4.8 lib/test_module.o $ objdump -d -s lib/test_module.o
lib/test_module.o: file format elf64-x86-64
Contents of section .text:
0000 534889fb e8000000 00b90f00 00004883 SH............H. 0010 f80f4889 df480f4e c848c7c6 00000000 ..H..H.N.H...... 0020 4839c9f3 a65b0f94 c0c3660f 1f440000 H9...[....f..D.. 0030 b8010000 00c3 ...... Contents of section .rodata.str1.1: 0000 546f6d4a 6f6e6573 50726566 697800 TomJonesPrefix. Contents of section .comment: 0000 00474343 3a202855 62756e74 7520342e .GCC: (Ubuntu 4. 0010 382e352d 34756275 6e747539 2920342e 8.5-4ubuntu9) 4. 0020 382e3500 8.5.
Contents of section .orc_unwind_ip:
0000 00000000 00000000 00000000 00000000 ................
0010 00000000 00000000 ........
Contents of section .orc_unwind:
0000 08000000 05001000 00000500 08000000 ................ 0010 05000000 00000000 08000000 05000000 ................ 0020 00000000 ....
Disassembly of section .text:
0000000000000000 <test_str_has_prefix>:
0: 53 push %rbx 1: 48 89 fb mov %rdi,%rbx 4: e8 00 00 00 00 callq 9 <test_str_has_prefix+0x9> 9: b9 0f 00 00 00 mov $0xf,%ecx e: 48 83 f8 0f cmp $0xf,%rax 12: 48 89 df mov %rbx,%rdi 15: 48 0f 4e c8 cmovle %rax,%rcx 19: 48 c7 c6 00 00 00 00 mov $0x0,%rsi 20: 48 39 c9 cmp %rcx,%rcx 23: f3 a6 repz cmpsb %es:(%rdi),%ds:(%rsi) 25: 5b pop %rbx 26: 0f 94 c0 sete %al 29: c3 retq 2a: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
0000000000000030 <test_str_has_prefix_TomJones>:
30: b8 01 00 00 00 mov $0x1,%eax
35: c3 retq
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 9 lines · Joe Perches
Using
static inline bool
str_has_prefix(const char *str, const char prefix[]){ return !strncmp(str, prefix, strlen(prefix)); }eliminates the strlen with gcc 4.8 (oldest I still have)
Ok, that looks like the right thing to do.
Side note: in the kernel we disable the whole "unsigned vs signed" pointer warning entirely, exactly because of the "char *" vs "unsigned char *" problem.
Linus
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 4 lines · On Fri, 21 Dec 2018 14:29:30 -0800 Linus Torvalds…
Quoted 3 lines · Joe Perches
Using
Quoted 9 lines
Quoted 6 lines
static inline bool
str_has_prefix(const char *str, const char prefix[]){ return !strncmp(str, prefix, strlen(prefix)); }eliminates the strlen with gcc 4.8 (oldest I still have)
Ok, that looks like the right thing to do.
Agreed, and I posted a new version. I can start running it through my test suit (I'll update all the instances in the tracing directory to use it), and then it will be ready for a pull request by next week.
I'll revert the top two patches from my for-next tree now.
-- Steve
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
In reply to Joe PerchesQuoted 6 lines · On Fri, 21 Dec 2018 14:20:25 -0800 Joe Perches <[email protected]> wrote:
static inline bool
str_has_prefix(const char *str, const char prefix[]){ return !strncmp(str, prefix, strlen(prefix)); }eliminates the strlen with gcc 4.8 (oldest I still have)
I tested it a bit more before posting.
I tested it against:
gcc 4.5.1, 4.5.4, 4.6.3, 6.2.0, 7.2.0 and 8.2
And the strlen was eliminated each time.
So this looks like the right approach :-)
-- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
In reply to Joe PerchesQuoted 13 lines · Joe Perches
Quoted 11 lines · Steven Rostedt
Quoted 4 lines · On Fri, 21 Dec 2018 21:58:32 +0100 Andreas Schwab <[email protected]> wrote:
Quoted 2 lines
Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question.
Why not make it an inline function?
Matters if that removes the strlen(const) optimization. I could try it and see what happens.
Quoted 7 lines
Using
static inline bool
str_has_prefix(const char *str, const char prefix[]){ return !strncmp(str, prefix, strlen(prefix)); }
We already have exactly that function, it's called strstarts().
commit 66f92cf9d415e96a5bdd6c64de8dd8418595d2fc
Author: Rusty Russell <[email protected]> Date: Tue Mar 31 13:05:36 2009 -0600
strstarts: helper function for !strncmp(str, prefix, strlen(prefix))
Please don't add a copy under another name.
As for converting existing users, go for it. FWIW, I ran a cocci script a few years ago to find suspicious strncmp() cases, and there were some (e87c3f, ca957b6), but fewer than I expected. There are some confused/confusing ones that apparently deliberately do strncmp(a, b, sizeof(b)) instead of the equivalent to strcmp(a, b) (e.g. 'strncmp(str, "hwc", 4) == 0')
Rasmus
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 16 lines · On Sun, 23 Dec 2018 23:01:52 +0100 Rasmus Villemoes <[email protected]>…
Quoted 15 lines · Joe Perches
Quoted 11 lines · Steven Rostedt
Quoted 4 lines · On Fri, 21 Dec 2018 21:58:32 +0100 Andreas Schwab <[email protected]> wrote:
Quoted 2 lines
Well, perhaps I can just remove the ending ones. I get paranoid with macro variables, and tend to over do it so that there's no question.
Why not make it an inline function?
Matters if that removes the strlen(const) optimization. I could try it and see what happens.
Using
Quoted 7 lines
Quoted 5 lines
static inline bool
str_has_prefix(const char *str, const char prefix[]){ return !strncmp(str, prefix, strlen(prefix)); }We already have exactly that function, it's called
strstarts().
It's not exact.
Quoted 7 lines
commit
66f92cf9d415e96a5bdd6c64de8dd8418595d2fcAuthor: Rusty Russell <[email protected]> Date: Tue Mar 31 13:05:36 2009 -0600strstarts: helper function for !strncmp(str, prefix, strlen(prefix))
Quoted 8 lines
Please don't add a copy under another name.
As for converting existing users, go for it. FWIW, I ran a cocci script a few years ago to find suspicious
strncmp()cases, and there were some (e87c3f, ca957b6), but fewer than I expected. There are some confused/confusing ones that apparently deliberately dostrncmp(a, b, sizeof(b))instead of the equivalent tostrcmp(a, b)(e.g. 'strncmp(str, "hwc", 4)== 0')
Well, one thing that str_has_prefix() does that strstarts() does not, is to return the prefix length which gets rid of the duplication.
Would it be OK to convert strstarts() to return the length of prefix?
-- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 13 lines · Steven Rostedt
Quoted 10 lines · On Sun, 23 Dec 2018 23:01:52 +0100 Rasmus Villemoes <[email protected]>…
Quoted 8 lines · Joe Perches
Using
static inline bool
str_has_prefix(const char *str, const char prefix[]){ return !strncmp(str, prefix, strlen(prefix)); }
Quoted 3 lines
Quoted 1 line
We already have exactly that function, it's called
strstarts().It's not exact.
Huh? The str_has_prefix() I quoted is exactly strstarts().
Quoted 3 lines
Well, one thing that
str_has_prefix()does thatstrstarts()does not, is to return the prefix length which gets rid of the duplication.
I hadn't seen the patches containing that version of str_has_prefix(). Anyway, I just wanted to point out that strstarts() exists, so that we at least do not add a copy of that.
Quoted 1 line
Would it be OK to convert
strstarts()to return the length of prefix?
Dunno. By far, most users of the strncmp() idiom only seem to be interested in the boolean result. It's true that there are some that then want to skip the prefix and do further parsing, and I can see how avoiding duplicating the prefix length is useful. But the mathematician in me can't help consider the corner case of 'the empty string is always a prefix of any other string', and having str_has_prefix(str, "") be false seems wrong - obviously, nobody would ever use a literal "" there, but nothing in str_has_prefix() _requires_ the prefix to be a constant.
Maybe 'bool str_skip_prefix(const char *s, const char *p, const char **out)' where *out is set to s+len on success, and set to s on failure (just to allow passing &s and continue parsing in elseifs)? That would make your 4/5 "tracing: Have the historgram use the result of str_has_prefix() for len of prefix" do
if (str_skip_prefix(str, "onmatch(", &action_str)) {hoisting the action_str declaration to the top, replacing the len variable?
Rasmus
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 14 lines · On Mon, 24 Dec 2018 00:52:13 +0100 Rasmus Villemoes <[email protected]>…
Quoted 13 lines · Steven Rostedt
Quoted 10 lines · On Sun, 23 Dec 2018 23:01:52 +0100 Rasmus Villemoes <[email protected]>…
Quoted 8 lines · Joe Perches
Using
static inline bool
str_has_prefix(const char *str, const char prefix[]){ return !strncmp(str, prefix, strlen(prefix)); }
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
We already have exactly that function, it's called
strstarts().It's not exact.
Huh? The
str_has_prefix()I quoted is exactlystrstarts().
The the implemented str_has_prefix() that you replied to is:
+/*
+ * A common way to test a prefix of a string is to do:
+ * strncmp(str, prefix, sizeof(prefix) - 1)
+ *
+ * But this can lead to bugs due to typos, or if prefix is a pointer
+ * and not a constant. Instead use strncmp_prefix().
+ */
+#define strncmp_prefix(str, prefix) \
+ ({ \
+ int ____strcmp_prefix_ret____; \
+ if (__builtin_constant_p(&prefix)) { \
+ ____strcmp_prefix_ret____ = \
+ strncmp(str, prefix, sizeof(prefix) - 1); \
+ } else { \
+ typeof(prefix) ____strcmp_prefix____ = prefix; \
+ ____strcmp_prefix_ret____ = \
+ strncmp(str, ____strcmp_prefix____, \
+ strlen(____strcmp_prefix____)); \
+ } \
+ ____strcmp_prefix_ret____; \
+ })
+
Note, this has turned into a nice function:
http://lkml.kernel.org/r/[email protected]
+/**
+ * str_has_prefix - Test if a string has a given prefix
+ * @str: The string to test
+ * @prefix: The string to see if @str starts with
+ *
+ * A common way to test a prefix of a string is to do:
+ * strncmp(str, prefix, sizeof(prefix) - 1)
+ *
+ * But this can lead to bugs due to typos, or if prefix is a pointer
+ * and not a constant. Instead use str_has_prefix().
+ *
+ * Returns: 0 if @str does not start with @prefix
+ strlen(@prefix) if @str does start with @prefix
+ */
+static __always_inline size_t str_has_prefix(const char *str, const char *prefix) +{
+ size_t len = strlen(prefix);
+ return strncmp(str, prefix, len) == 0 ? len : 0;
+}
+
Quoted 8 lines
Quoted 4 lines
Well, one thing that
str_has_prefix()does thatstrstarts()does not, is to return the prefix length which gets rid of the duplication.I hadn't seen the patches containing that version of
str_has_prefix(). Anyway, I just wanted to point out thatstrstarts()exists, so that we at least do not add a copy of that.
That's because you didn't read the patch that you quoted, just the change log.
Quoted 11 lines
Quoted 2 lines
Would it be OK to convert
strstarts()to return the length of prefix?Dunno. By far, most users of the
strncmp()idiom only seem to be interested in the boolean result. It's true that there are some that then want to skip the prefix and do further parsing, and I can see how avoiding duplicating the prefix length is useful. But the mathematician in me can't help consider the corner case of 'the empty string is always a prefix of any other string', and havingstr_has_prefix(str, "")be false seems wrong - obviously, nobody would ever use a literal "" there, but nothing instr_has_prefix()_requires_ the prefix to be a constant.
Which would be a useless use case. And if you define that it returns the length of prefix on return, then it both matches and doesn't match ;-)
Quoted 11 lines
Maybe 'bool
str_skip_prefix(const char *s, const char *p, const char **out)' where *out is set to s+len on success, and set to s on failure (just to allow passing &s and continue parsing in elseifs)? That would make your 4/5 "tracing: Have the historgram use the result ofstr_has_prefix()for len of prefix" doif (str_skip_prefix(str, "onmatch(", &action_str)) {hoisting the
action_strdeclaration to the top, replacing the len variable?
The use cases I've used in the final patch series uses the len for indexing and other cases.
I think I'm keeping the str_has_prefix() and change the other users to use it in the kernel. Most of the git grep strstarts() is tools and scripts anyway.
-- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
In reply to Rasmus VillemoesQuoted 1 line · Rasmus Villemoes
On 21/12/2018 23.20, Joe Perches wrote:
[]
Quoted 1 line
Quoted 1 line
static inline bool
str_has_prefix(const char *str, const char prefix[])
[]
Quoted 1 line
We already have exactly that function, it's called
strstarts().
Heh. Thanks Rasmus. I didn't remember that one.
I think the 'int str_has_prefix' naming and returning the prefix length may be a bit better use than 'bool strstarts' and perhaps a treewide conversion of the existing strstarts to str_has_prefix would be OK as there aren't that many.
$ git grep -w strstarts | wc -l 91
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
In reply to Joe PerchesQuoted 1 line · On Fri, 21 Dec 2018 12:46:47 -0800 Joe Perches <[email protected]> wrote:
I think all the underscores are unnecessary and confusing.
Here. I removed a beginning and ending underscore from each variable ;-)
-- Steve
include/linux/string.h +22 -0
@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);extern void *vmemdup_user (const void __user *,size_t );extern void *memdup_user_nul (const void __user *,size_t ); +/** +* str_has_prefix - Test if a string has a given prefix +* @str: The string to test +* @prefix: The string to see if @str starts with +* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use str_has_prefix(). +* +* Returns: 0 if @str does not start with @prefix +strlen(@prefix) if @str does start with @prefix +*/ +#define str_has_prefix (str, prefix) \ + ({ \ + const char *___prefix___ = (const char *)(prefix); \ + int ___len___ = strlen(___prefix___); \ + strncmp(str, ___prefix___, ___len___) == 0 ? \ + ___len___ : 0; \ + }) +/* * Include machine specific inline routines */
A message that is not in the archive (sent off-list or not yet received): CAHk-=wjtkvFUuRNZU67KccuUKYHw=pYoDMQJ_9OVDFxOwmK9zQ@mail.gmail.com
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 1 line · On Fri, 21 Dec 2018 12:41:17 -0800 Linus Torvalds…
Parentheses....
?
-- Steve
Quoted 77 lines · On Fri, Dec 21, 2018, 12:35 Steven Rostedt <[email protected] wrote:
Quoted 75 lines
Quoted 1 line · On Fri, 21 Dec 2018 12:01:48 -0800 Linus Torvalds…
On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt <[email protected]>
Quoted 9 lines · wrote:
Quoted 3 lines
OK, what about if we just use
strlen()and say that this macro is not safe for parameters with side effects.I think gcc follows simple assignments just fine, and does the optimized
strlen()for them too.So why not just do
Quoted 9 lines
#define have_prefix(str,prefix) ({ \ const char *__pfx = prefix; \ size_t __pfxlen = strlen(__pfx); \ strncmp(str, __pfx, __pfxlen) ? 0 : __pfxlen); })and be done with it safely?
The above is ENTIRELY untested.
At first I thought this would have issues, but with a slight change...
#define have_prefix(str, prefix) ({ \ const char *__pfx = (const char *)prefix; \And the rest the same, it appears to work.
Need the cast because if for some reason someone passed in something like "const unsigned char" then it wouldn't work. But that's just a nit.
So something like this then?
-- Steve
include/linux/string.h +22
-0@@ -14,6 +14,28 @@ extern void *memdup_user(const void __user *, size_t);extern void *vmemdup_user (const void __user *,size_t );extern void *memdup_user_nul (const void __user *,size_t ); +/** +* have_prefix - Test if a string has a given prefix +* @str: The string to test +* @prefix: The string to see if @str starts with +* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use has_prefix(). +* +* Returns: 0 if @str does not start with @prefix +strlen(@prefix) if @str does start with @prefix +*/ +#define has_prefix (str, prefix) \ + ({ \ +const char *____prefix____ = (const char *)(prefix); \ +int ____len____ =strlen (____prefix____); \ +strncmp (str, ____prefix____, ____len____) ==0 ? \ + ____len____ :0 ; \ + }) +/* * Include machine specific inline routines */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 7 lines · Steven Rostedt
Quoted 1 line · On Fri, 21 Dec 2018 12:41:17 -0800 Linus Torvalds…
Parentheses....
?
Your patch actually had them, but in the body of your email you did
Quoted 2 lines
#define have_prefix(str, prefix) ({ \
const char *__pfx = (const char *)prefix; \which is just completely wrong.
Considering your _old_ patch had the exact same bug, I really think you need to start internalizing the whole "macro arguments *have* to be properly protected" thing.
And I agree with Joe that using a million underscores just makes code less legible. Two underscores at the beginning is the standard namespace protection. Underscores at the end do nothing. And using *more* than two is just crazy.
Finally, I think the cast is actually bogus and wrong. Why would the prefix ever be anything but "const char *"? Adding the cast only makes it more possible that somebody uses a truly wrong type ("unsigned long *" ?), and then the cast just silently hides it.
If somebody uses "unsigned char *" for this, they'd get the exact same warning if they were using strncmp and do this by hand.
So why would that helper function act any differently? Particularly when it then has the huge downside that it will also accept absolute garbage?
Anyway, that was a long and winding NAK for your patch.
Linus
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 11 lines · On Fri, 21 Dec 2018 14:08:11 -0800 Linus Torvalds…
Quoted 7 lines · Steven Rostedt
Quoted 1 line · On Fri, 21 Dec 2018 12:41:17 -0800 Linus Torvalds…
Parentheses....
?
Your patch actually had them, but in the body of your email you did
Quoted 8 lines
Quoted 2 lines
#define have_prefix(str, prefix) ({ \ const char *__pfx = (const char *)prefix; \which is just completely wrong.
Considering your _old_ patch had the exact same bug, I really think you need to start internalizing the whole "macro arguments *have* to be properly protected" thing.
Well, there's less with assignments that can go wrong than with other code. That is, there's little that can happen with "int x = arg;" where arg is the macro paramater to cause something really nasty. The chances of that happening is up there with using only two underscores causing an issue.
But they don't hurt to add.
Quoted 5 lines
And I agree with Joe that using a million underscores just makes code less legible. Two underscores at the beginning is the standard namespace protection. Underscores at the end do nothing. And using *more* than two is just crazy.
The two are standard for static variables in C code, but that makes me worry about macros. I usually do at least three for macros. The underscores at the end, to me, make it more balanced and easier to read:
strncmp(str, ___prefix___, ___len___);
To me looks better than:
strncmp(str, ___prefix, ___len);
But again, no reason to fight this bikeshed.
Quoted 5 lines
Finally, I think the cast is actually bogus and wrong. Why would the prefix ever be anything but "const char *"? Adding the cast only makes it more possible that somebody uses a truly wrong type ("unsigned long *" ?), and then the cast just silently hides it.
Actually after I sent the email, I was thinking the same thing, that the original strncmp() would probably complain about that too, and we should keep it the same. Not sure why I even suggested that. Perhaps, I tested too many use cases :-/
Quoted 9 lines
If somebody uses "unsigned char *" for this, they'd get the exact same warning if they were using strncmp and do this by hand.
So why would that helper function act any differently? Particularly when it then has the huge downside that it will also accept absolute garbage?
Anyway, that was a long and winding NAK for your patch.
But after all that and said. I tested it as a static __always_inline, and guess what. It also optimizes out the strlen() for constants!!!
-- Steve
include/linux/string.h +21 -0
@@ -456,4 +456,25 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len, memcpy(dest, src, dest_len); } +/** +* str_has_prefix - Test if a string has a given prefix +* @str: The string to test +* @prefix: The string to see if @str starts with +* +* A common way to test a prefix of a string is to do: +* strncmp(str, prefix, sizeof(prefix) - 1) +* +* But this can lead to bugs due to typos, or if prefix is a pointer +* and not a constant. Instead use str_has_prefix(). +* +* Returns: 0 if @str does not start with @prefix +strlen(@prefix) if @str does start with @prefix +*/ +static __always_inline int str_has_prefix(const char *str, const char *prefix) +{ +int len =strlen (prefix); + +return strncmp (str, prefix, len) ==0 ? len :0 ; +} +#endif /* _LINUX_STRING_H_ */
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 8 lines · Steven Rostedt
Quoted 8 lines
Your patch actually had them, but in the body of your email you did
Quoted 2 lines
#define have_prefix(str, prefix) ({ \ const char *__pfx = (const char *)prefix; \which is just completely wrong.
Quoted 7 lines
Quoted 3 lines
Considering your _old_ patch had the exact same bug, I really think you need to start internalizing the whole "macro arguments *have* to be properly protected" thing.
Well, there's less with assignments that can go wrong than with other code. That is, there's little that can happen with "int x = arg;" where arg is the macro paramater to cause something really nasty.
What's wrong, Steven?
The assignment is entirely irrelevant.
The problem is the cast.
A type cast has a very high priority, and so if you do
(const char *)prefix
it breaks completely if you might have something like"a+6" as the argument.
Think about what if "a" is of type "unsigned long", for example?
Linus
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 13 lines · On Fri, 21 Dec 2018 14:57:13 -0800 Linus Torvalds…
Quoted 12 lines · Steven Rostedt
Quoted 11 lines
Your patch actually had them, but in the body of your email you did
Quoted 2 lines
#define have_prefix(str, prefix) ({ \ const char *__pfx = (const char *)prefix; \which is just completely wrong.
Considering your _old_ patch had the exact same bug, I really think you need to start internalizing the whole "macro arguments *have* to be properly protected" thing.
Quoted 5 lines
Quoted 3 lines
Well, there's less with assignments that can go wrong than with other code. That is, there's little that can happen with "int x = arg;" where arg is the macro paramater to cause something really nasty.
What's wrong, Steven?
We are miscommunicating here...
I was talking about the missing parenthesis on the original patch, which you stated was missing as well. And the original patch didn't have the typecast.
Quoted 5 lines
The assignment is entirely irrelevant.
The problem is the cast.
Quoted 7 lines
A type cast has a very high priority, and so if you do
(const char *)prefix
it breaks completely if you might have something like"a+6" as the argument.
Think about what if "a" is of type "unsigned long", for example?
Yes, when writing the real code, I noticed that the typecast could cause issues without the parenthesis, and added them.
The email you are replying to was saying there's not much that can go wrong with:
#define MACRO(x) { \
int __p = x; \I definitely can see something wrong with:
#define MACRO(x) { \
int __p = (int)x; \because exactly what you stated.
There's nothing wrong with adding (x) for the first one, but it's unlikely anything will cause it harm. The second one the (x) *is* most definitely required.
This is a long winded "I agree with you" ;-)
-- Steve
Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
In reply to Linus TorvaldsQuoted 14 lines · Linus Torvalds
Quoted 8 lines · Steven Rostedt
I figured the best thing to do is to create a helper macro and place it into
include/linux/string.h. And go around and fix all the open coded versions of it later.I plan on only applying this patch and updating the tracing hooks for this merge window. And perhaps use it to fix some of the bugs that were found.
I like the helper function concept, but as they say about CS: "There is only one hard problem in computer science: naming and off-by-one errors".
Quoted 6 lines
And this one has that problem. The name makes absolutely no sense.
Calling it "
strncmp_prefix()" when there is no "n" there makes no sense.So drop the "n" from the name.
Being British the 'n' implies 'and' and still being interpreted Boolean.
strncmp = string and compare
Like other of our words Fish'n'Chips, Salt'n'Shake.
I don't think these abbreviations exist in American English.
Steven Rostedt
maintainerRe: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro
Quoted 11 lines · On Sat, 22 Dec 2018 10:20:02 +0000 Malcolm Priestley <[email protected]> wrote:
Quoted 4 lines
Calling it "
strncmp_prefix()" when there is no "n" there makes no sense.So drop the "n" from the name.
Being British the 'n' implies 'and' and still being interpreted Boolean.
strncmp = string and compare
Like other of our words Fish'n'Chips, Salt'n'Shake.
I don't think these abbreviations exist in American English.
No, no, they actually do exist in American English.
But when people ask me which language is my mother tongue, I simply reply "C". In which case, 'n' means number.
-- Steve
Steven Rostedt
maintainerPATCH 24/24 tracing: Use strncmp_prefix() helper for histogram code
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
The tracing histogram code contains a lot of instances of the construct:
strncmp(str, "const", sizeof("const") - 1)
This can be prone to bugs due to typos or bad cut and paste. Use the strncmp_prefix() helper macro instead that removes the need for having two copies of the constant string.
--- kernel/trace/trace_events_hist.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
kernel/trace/trace_events_hist.c +14 -14
@@ -1881,8 +1881,8 @@ static int parse_action(char *str, struct hist_trigger_attrs *attrs)if (attrs->n_actions >=HIST_ACTIONS_MAX )return ret; -if ((strncmp (str,"onmatch(" ,sizeof ("onmatch(" ) -1 ) ==0 ) || - (strncmp (str,"onmax(" ,sizeof ("onmax(" ) -1 ) ==0 )) { +if ((strncmp_prefix (str,"onmatch(" ) ==0 ) || + (strncmp_prefix (str,"onmax(" ) ==0 )) { attrs->action_str[attrs->n_actions] =kstrdup (str,GFP_KERNEL );if (!attrs->action_str[attrs->n_actions]) { ret = -ENOMEM ;
@@ -1899,34 +1899,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs) {int ret =0 ; -if ((strncmp (str,"key=" ,sizeof ("key=" ) -1 ) ==0 ) || - (strncmp (str,"keys=" ,sizeof ("keys=" ) -1 ) ==0 )) { +if ((strncmp_prefix (str,"key=" ) ==0 ) || + (strncmp_prefix (str,"keys=" ) ==0 )) { attrs->keys_str =kstrdup (str,GFP_KERNEL );if (!attrs->keys_str) { ret = -ENOMEM ; goto out; } - }else if ((strncmp (str,"val=" ,sizeof ("val=" ) -1 ) ==0 ) || - (strncmp (str,"vals=" ,sizeof ("vals=" ) -1 ) ==0 ) || - (strncmp (str,"values=" ,sizeof ("values=" ) -1 ) ==0 )) { + }else if ((strncmp_prefix (str,"val=" ) ==0 ) || + (strncmp_prefix (str,"vals=" ) ==0 ) || + (strncmp_prefix (str,"values=" ) ==0 )) { attrs->vals_str =kstrdup (str,GFP_KERNEL );if (!attrs->vals_str) { ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"sort=" ,sizeof ("sort=" ) -1 ) ==0 ) { + }else if (strncmp_prefix (str,"sort=" ) ==0 ) { attrs->sort_key_str =kstrdup (str,GFP_KERNEL );if (!attrs->sort_key_str) { ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"name=" ,sizeof ("name=" ) -1 ) ==0 ) { + }else if (strncmp_prefix (str,"name=" ) ==0 ) { attrs->name =kstrdup (str,GFP_KERNEL );if (!attrs->name) { ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"clock=" ,sizeof ("clock=" ) -1 ) ==0 ) { + }else if (strncmp_prefix (str,"clock=" ) ==0 ) {strsep (&str,"=" );if (!str) { ret = -EINVAL ;
@@ -1939,7 +1939,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs) ret = -ENOMEM ; goto out; } - }else if (strncmp (str,"size=" ,sizeof ("size=" ) -1 ) ==0 ) { + }else if (strncmp_prefix (str,"size=" ) ==0 ) {int map_bits =parse_map_size (str);if (map_bits <0 ) {
@@ -3558,7 +3558,7 @@ static struct action_data *onmax_parse(char *str)if (!onmax_fn_name || !str) goto free; -if (strncmp (onmax_fn_name,"save" ,sizeof ("save" ) -1 ) ==0 ) { +if (strncmp_prefix (onmax_fn_name,"save" ) ==0 ) {char *params =strsep (&str,")" );if (!params) {
@@ -4346,7 +4346,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)for (i =0 ; i < hist_data->attrs->n_actions; i++) { str = hist_data->attrs->action_str[i]; -if (strncmp (str,"onmatch(" ,sizeof ("onmatch(" ) -1 ) ==0 ) { +if (strncmp_prefix (str,"onmatch(" ) ==0 ) {char *action_str = str +sizeof ("onmatch(" ) -1 ; data =onmatch_parse (tr, action_str);
@@ -4355,7 +4355,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)break ; } data->fn = action_trace; - }else if (strncmp (str,"onmax(" ,sizeof ("onmax(" ) -1 ) ==0 ) { + }else if (strncmp_prefix (str,"onmax(" ) ==0 ) {char *action_str = str +sizeof ("onmax(" ) -1 ; data =onmax_parse (action_str);
Signature · 1 line
2.19.2
A message that is not in the archive (sent off-list or not yet received): [email protected]
Steven Rostedt
maintainerRe: [for-next][PATCH 09/24] seq_buf: Make seq_buf_puts() null-terminate the buffer
Quoted 16 lines · On Fri, 21 Dec 2018 12:56:27 -0500 Steven Rostedt <[email protected]> wrote:
From: Michael Ellerman <[email protected]>
Currently
seq_buf_puts()will happily create a non null-terminated string for you in the buffer. This is particularly dangerous if the buffer is on the stack.For example:
char buf[8]; char secret = "secret"; struct seq_buf s; seq_buf_init(&s, buf, sizeof(buf)); seq_buf_puts(&s, "foo"); printk("Message is %s\n", buf);
Quoted 3 lines
Can result in:
Message is fooªªªªªsecret
Sending this via quilt, and that we have non UTF8 characters causes LKML to blow up.
There's a couple more patches with this issue. I'm going to fix up the change logs and rebase them.
-- Steve
Quoted 38 lines
We could require all users to
memset()their buffer to zero before use. But that seems likely to be forgotten and lead to bugs.Instead we can change
seq_buf_puts()to always leave the buffer in a null-terminated state.The only downside is that this makes the buffer 1 character smaller for
seq_buf_puts(), but that seems like a good trade off.Acked-by Kees Cook <[email protected]>Signed-off-by Michael Ellerman <[email protected]>Signed-off-by Steven Rostedt (VMware) <[email protected]>--- lib/seq_buf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)lib/seq_buf.c +5
-1@@ -144,9 +144,13 @@ int seq_buf_puts(struct seq_buf *s, const char *str)WARN_ON (s->size ==0 ); +/* Add 1 to len for the trailing null byte which must be there */ + len +=1 ; +if (seq_buf_can_fit (s, len)) {memcpy (s->buffer + s->len, str, len); - s->len += len; +/* Don't count the trailing null byte against the capacity */ + s->len += len -1 ;return 0 ; }seq_buf_set_overflow (s);
Steven Rostedt
maintainerRe: [for-next][PATCH 09/24] seq_buf: Make seq_buf_puts() null-terminate the buffer
Quoted 9 lines · On Fri, 21 Dec 2018 13:00:21 -0500 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines
Can result in:
Message is fooªªªªªsecret
Sending this via quilt, and that we have non UTF8 characters causes LKML to blow up.
There's a couple more patches with this issue. I'm going to fix up the change logs and rebase them.
Actually, this change log should have them. Bah, I'll just ignore it. The repo that gets pulled is fine.
-- Steve
Michael Ellerman
reviewerRe: [for-next][PATCH 09/24] seq_buf: Make seq_buf_puts() null-terminate the buffer
Quoted 8 lines · Steven Rostedt
Quoted 4 lines · On Fri, 21 Dec 2018 13:00:21 -0500 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines
Can result in:
Message is fooªªªªªsecret
Quoted 8 lines
Quoted 5 lines
Sending this via quilt, and that we have non UTF8 characters causes LKML to blow up.
There's a couple more patches with this issue. I'm going to fix up the change logs and rebase them.
Actually, this change log should have them. Bah, I'll just ignore it. The repo that gets pulled is fine.
Thanks, sorry for the bother. It doesn't really matter in this case what's displayed, it's just an example.
cheers