[for-next][PATCH 00/24] tracing: Updates for the next (coming soon) merge window

[for-next][PATCH 00/24] tracing: Updates for the next (coming soon) merge window

[Steven Rostedt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2967 bytes --]

  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(-)

[for-next][PATCH 01/24] ftrace: Allow ftrace_replace_code() to be schedulable

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Link: http://lkml.kernel.org/r/20181204192903.8193-1-anders.roxell@linaro.org
Link: http://lkml.kernel.org/r/20181205183303.828422192@goodmis.org

Reported-by: Anders Roxell <anders.roxell@linaro.org>
Tested-by: Anders Roxell <anders.roxell@linaro.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/ftrace.h |  1 +
 kernel/trace/ftrace.c  | 19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 98e141c71ad0..13485a19e964 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -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),
 };
 
 /*
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 8ef9fc226037..ab3e8b995e12 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -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;
-- 
2.19.2

[for-next][PATCH 02/24] arm64: ftrace: Set FTRACE_MAY_SLEEP before ftrace_modify_all_code()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Link: http://lkml.kernel.org/r/20181204192903.8193-1-anders.roxell@linaro.org
Link: http://lkml.kernel.org/r/20181205183304.000714627@goodmis.org

Acked-by: Will Deacon <will.deacon@arm.com>
Reported-by: Anders Roxell <anders.roxell@linaro.org>
Tested-by: Anders Roxell <anders.roxell@linaro.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/arm64/kernel/ftrace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 57e962290df3..57d4e936a176 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -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);
 }
 
-- 
2.19.2

[for-next][PATCH 03/24] fgraph: Add comment to describe ftrace_graph_get_ret_stack

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/fgraph.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index a3704ec8b599..d4f04f0ca646 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -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)
 {
-- 
2.19.2

[for-next][PATCH 04/24] x86/ftrace: Do not call function graph from dynamic trampolines

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/kernel/ftrace.c    | 41 ++++++++++++++++++++-----------------
 arch/x86/kernel/ftrace_64.S |  8 ++++----
 2 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 7ee8067cbf45..8257a59704ae 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -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)
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index 91b2cff4b79a..75f2b36b41a6 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -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)
-- 
2.19.2

[for-next][PATCH 05/24] powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/powerpc/kernel/process.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 96f34730010f..ce393df243aa 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -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)
-- 
2.19.2

Re: [for-next][PATCH 05/24] powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[Michael Ellerman]

Steven Rostedt <rostedt@goodmis.org> writes:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> 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.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  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).

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

Re: [for-next][PATCH 05/24] powerpc/frace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[Steven Rostedt]

On Sat, 22 Dec 2018 20:51:21 +1100
Michael Ellerman <mpe@ellerman.id.au> 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).
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

Thanks! I'll add your ack.

-- Steve

[for-next][PATCH 06/24] sparc64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Cc: sparclinux@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 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(-)

diff --git a/arch/sparc/kernel/perf_event.c b/arch/sparc/kernel/perf_event.c
index 47c871394ccb..6de7c684c29f 100644
--- a/arch/sparc/kernel/perf_event.c
+++ b/arch/sparc/kernel/perf_event.c
@@ -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++;
 			}
diff --git a/arch/sparc/kernel/stacktrace.c b/arch/sparc/kernel/stacktrace.c
index be4c14cccc05..dd654e651500 100644
--- a/arch/sparc/kernel/stacktrace.c
+++ b/arch/sparc/kernel/stacktrace.c
@@ -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;
diff --git a/arch/sparc/kernel/traps_64.c b/arch/sparc/kernel/traps_64.c
index aa624ed79db1..0cd02a64a451 100644
--- a/arch/sparc/kernel/traps_64.c
+++ b/arch/sparc/kernel/traps_64.c
@@ -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++;
 			}
-- 
2.19.2

Re: [for-next][PATCH 06/24] sparc64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Fri, 21 Dec 2018 12:56:24 -0500

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 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.
> 
> Cc: sparclinux@vger.kernel.org
> Cc: "David S. Miller" <davem@davemloft.net>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [for-next][PATCH 06/24] sparc64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[Steven Rostedt]

On Fri, 21 Dec 2018 10:24:14 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> Date: Fri, 21 Dec 2018 12:56:24 -0500
> 
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > 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.
> > 
> > Cc: sparclinux@vger.kernel.org
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>  
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Thanks David!

-- Steve

[for-next][PATCH 07/24] sh: ftrace: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Cc: linux-sh@vger.kernel.org
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Rich Felker <dalias@libc.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/sh/kernel/dumpstack.c | 11 +++++++----
 arch/sh/kernel/dwarf.c     |  9 +++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/sh/kernel/dumpstack.c b/arch/sh/kernel/dumpstack.c
index b564b1eae4ae..2c2e151bf39e 100644
--- a/arch/sh/kernel/dumpstack.c
+++ b/arch/sh/kernel/dumpstack.c
@@ -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);
 
diff --git a/arch/sh/kernel/dwarf.c b/arch/sh/kernel/dwarf.c
index bb511e2d9d68..df0fd6efe758 100644
--- a/arch/sh/kernel/dwarf.c
+++ b/arch/sh/kernel/dwarf.c
@@ -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
 
-- 
2.19.2

[for-next][PATCH 08/24] arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Cc: linux-arm-kernel@lists.infradead.org
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 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(-)

diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index bcafd7dcfe8b..1b792b46604e 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -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);
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index d9a4c2d6dd8b..37a66394b07d 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -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))
diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index 933adbc0f654..53c40196b607 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -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);
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 7723dadf25be..1a29f2695ff2 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -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);
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index f258636273c9..a777ae90044d 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -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);
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 5f4d9acb32f5..49ebf3771391 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -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;
-- 
2.19.2

[for-next][PATCH 10/24] seq_buf: Use size_t for len in seq_buf_puts()

[Steven Rostedt]

From: Michael Ellerman <mpe@ellerman.id.au>

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.

Link: http://lkml.kernel.org/r/20181019042109.8064-2-mpe@ellerman.id.au

Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/seq_buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 6aabb609dd87..bd807f545a9d 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -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);
 
-- 
2.19.2

[for-next][PATCH 11/24] tracing: Fix ftrace_graph_get_ret_stack() to use task and not current

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Fixes: 51584396cff5 ("arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack")
Reported-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/fgraph.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index d4f04f0ca646..8dfd5021b933 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -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 &current->ret_stack[idx];
+		return &task->ret_stack[idx];
 
 	return NULL;
 }
-- 
2.19.2

[for-next][PATCH 12/24] tracing: Remove unnecessary hist trigger struct field

[Steven Rostedt]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

hist_field.var_idx is completely unused, so remove it.

Link: http://lkml.kernel.org/r/d4e066c0f509f5f13ad3babc8c33ca6e7ddc439a.1545161087.git.tom.zanussi@linux.intel.com

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 82e72c48a5a9..d29bf8a8e1dd 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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;
 };
-- 
2.19.2

[for-next][PATCH 13/24] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

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.

Link: http://lkml.kernel.org/r/e3e754f2bd18e56eaa8baf79bee619316ebf4cfc.1545161087.git.tom.zanussi@linux.intel.com

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 38 ++++++++++++++++----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index d29bf8a8e1dd..25d06b3ae1f6 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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;
 		}
-- 
2.19.2

[for-next][PATCH 14/24] tracing: Use var_refs[] for hist trigger reference checking

[Steven Rostedt]

From: Tom Zanussi <tzanussi@gmail.com>

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.

Link: http://lkml.kernel.org/r/1545246556.4239.7.camel@gmail.com

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 68 ++++++--------------------------
 1 file changed, 11 insertions(+), 57 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 25d06b3ae1f6..f3caf6e484f4 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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,
-- 
2.19.2

[for-next][PATCH 15/24] tracing: Remove open-coding of hist trigger var_ref management

[Steven Rostedt]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

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.

Link: http://lkml.kernel.org/r/05ddae93ff514e66fc03897d6665231892939913.1545161087.git.tom.zanussi@linux.intel.com

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 93 +++++++++++++++++++++++++-------
 1 file changed, 75 insertions(+), 18 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index f3caf6e484f4..6309f4dbfb9c 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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;
-- 
2.19.2

[for-next][PATCH 16/24] tracing: Use hist triggers var_ref array to destroy var_refs

[Steven Rostedt]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

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.

Link: http://lkml.kernel.org/r/fad1a164f0e257c158e70d6eadbf6c586e04b2a2.1545161087.git.tom.zanussi@linux.intel.com

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 6309f4dbfb9c..923c572ee68d 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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,
-- 
2.19.2

[for-next][PATCH 17/24] tracing: Remove hist trigger synth_var_refs

[Steven Rostedt]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

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.

Link: http://lkml.kernel.org/r/b4d3470526b8f0426dcec125399dad9ad9b8589d.1545161087.git.tom.zanussi@linux.intel.com

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 923c572ee68d..1b1aee944588 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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);
 }
-- 
2.19.2

[for-next][PATCH 18/24] tracing: Add hist trigger comments for variable-related fields

[Steven Rostedt]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Add a few comments to help clarify how variable and variable reference
fields are used in the code.

Link: http://lkml.kernel.org/r/ea857ce948531d7bec712bbb0f38360aa1d378ec.1545161087.git.tom.zanussi@linux.intel.com

Acked-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 1b1aee944588..c5448c103770 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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;
-- 
2.19.2

[for-next][PATCH 19/24] tracing: Merge seq_print_sym_short() and seq_print_sym_offset()

[Steven Rostedt]

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>

These two functions are nearly identical, so we can avoid some code
duplication by moving the conditional into a common implementation.

Link: http://lkml.kernel.org/r/20181029223542.26175-2-linux@rasmusvillemoes.dk

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_output.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 6e6cc64faa38..85ecd061c7be 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -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);
-- 
2.19.2

[for-next][PATCH 21/24] tracing: Simplify printfing in seq_print_sym

[Steven Rostedt]

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>

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).

Link: http://lkml.kernel.org/r/20181029223542.26175-4-linux@rasmusvillemoes.dk

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_output.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index f06fb899b746..54373d93e251 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -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
-- 
2.19.2

[for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

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).

Link: http://lkml.kernel.org/r/e3e754f2bd18e56eaa8baf79bee619316ebf4cfc.1545161087.git.tom.zanussi@linux.intel.com
Link: http://lkml.kernel.org/r/20181219211615.2298e781@gandalf.local.home

Cc: Joe Perches <joe@perches.com>
Cc: Tom Zanussi <zanussi@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/string.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..a998a53333ef 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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
  */
-- 
2.19.2

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Linus Torvalds]

On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> 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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 10:51:29 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > 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.

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).

> 
> 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'.

> 
> 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) ...

I like changing the name.

> 
> 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;



> 
> (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

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 '&'.

> 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

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..f9d274a81276 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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

[Linus Torvalds]

On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> 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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 12:01:48 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > 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.
>

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

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..4586fee60194 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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

[Joe Perches]

On Fri, 2018-12-21 at 15:35 -0500, Steven Rostedt wrote:
> 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
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
[]
> @@ -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?

> + * @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;				\
> +	}) 

I think all the underscores are unnecessary and confusing.

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 12:46:47 -0800
Joe Perches <joe@perches.com> wrote:


> > @@ -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?

Sure.

> 
> > + * @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;				\
> > +	})   
> 
> 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

[Andreas Schwab]

On Dez 21 2018, Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 21 Dec 2018 12:46:47 -0800
> Joe Perches <joe@perches.com> wrote:
>
>> > + * @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;				\
>> > +	})   
>> 
>> 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.

Why not make it an inline function?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 21:58:32 +0100
Andreas Schwab <schwab@linux-m68k.org> wrote:


> > 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

[Joe Perches]

On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote:
> On Fri, 21 Dec 2018 21:58:32 +0100
> Andreas Schwab <schwab@linux-m68k.org> wrote:
> 
> 
> > > 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

[Linus Torvalds]

On Fri, Dec 21, 2018 at 2:20 PM Joe Perches <joe@perches.com> wrote:
>
> 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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 14:29:30 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Dec 21, 2018 at 2:20 PM Joe Perches <joe@perches.com> wrote:
> >
> > 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.
> 

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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 14:20:25 -0800
Joe Perches <joe@perches.com> 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

[Rasmus Villemoes]

On 21/12/2018 23.20, Joe Perches wrote:
> On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote:
>> On Fri, 21 Dec 2018 21:58:32 +0100
>> Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>>
>>>> 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));
> }
> 

We already have exactly that function, it's called strstarts().

commit 66f92cf9d415e96a5bdd6c64de8dd8418595d2fc
Author: Rusty Russell <rusty@rustcorp.com.au>
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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Sun, 23 Dec 2018 23:01:52 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 21/12/2018 23.20, Joe Perches wrote:
> > On Fri, 2018-12-21 at 16:08 -0500, Steven Rostedt wrote:  
> >> On Fri, 21 Dec 2018 21:58:32 +0100
> >> Andreas Schwab <schwab@linux-m68k.org> wrote:
> >>
> >>  
> >>>> 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));
> > }
> >   
> 
> We already have exactly that function, it's called strstarts().

It's not exact.

> 
> commit 66f92cf9d415e96a5bdd6c64de8dd8418595d2fc
> Author: Rusty Russell <rusty@rustcorp.com.au>
> 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')

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

[Rasmus Villemoes]

On 23/12/2018 23.56, Steven Rostedt wrote:
> On Sun, 23 Dec 2018 23:01:52 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
>> On 21/12/2018 23.20, Joe Perches wrote:
>>>
>>> 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().
> 
> It's not exact.

Huh? The str_has_prefix() I quoted is exactly strstarts().

> 
> Well, one thing that str_has_prefix() does that strstarts() 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.

> 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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Mon, 24 Dec 2018 00:52:13 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 23/12/2018 23.56, Steven Rostedt wrote:
> > On Sun, 23 Dec 2018 23:01:52 +0100
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >   
> >> On 21/12/2018 23.20, Joe Perches wrote:  
> >>>
> >>> 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().  
> > 
> > It's not exact.  
> 
> Huh? The str_has_prefix() I quoted is exactly strstarts().

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/20181222162856.518489380@goodmis.org

+/**
+ * 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;
+}
+


> 
> > 
> > Well, one thing that str_has_prefix() does that strstarts() 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.

That's because you didn't read the patch that you quoted, just the
change log.

> 
> > 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.

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 ;-)

> 
> 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?
> 

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

[Joe Perches]

On Sun, 2018-12-23 at 23:01 +0100, Rasmus Villemoes wrote:
> On 21/12/2018 23.20, Joe Perches wrote:
[]
> > static inline bool str_has_prefix(const char *str, const char prefix[])
[]
> 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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 12:46:47 -0800
Joe Perches <joe@perches.com> wrote:

> I think all the underscores are unnecessary and confusing.

Here. I removed a beginning and ending underscore from each variable ;-)

-- Steve

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..7f88444471f7 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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
  */

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 12:41:17 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Parentheses....

?

-- Steve

> 
> On Fri, Dec 21, 2018, 12:35 Steven Rostedt <rostedt@goodmis.org wrote:
> 
> > On Fri, 21 Dec 2018 12:01:48 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> > > On Fri, Dec 21, 2018 at 11:40 AM Steven Rostedt <rostedt@goodmis.org>
> > wrote:
> > > >
> > > > 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.
> > >
> >
> > 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
> >
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 27d0482e5e05..4586fee60194 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -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

[Linus Torvalds]

On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 21 Dec 2018 12:41:17 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > Parentheses....
>
> ?

Your patch actually had them, but in the body of your email you did

> #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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 14:08:11 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Fri, 21 Dec 2018 12:41:17 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >  
> > > Parentheses....  
> >
> > ?  
> 
> Your patch actually had them, but in the body of your email you did
> 
> > #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.

> 
> 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.

> 
> 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 :-/

> 
> 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!!!

Suggested-by: Andreas Schwab <schwab@linux-m68k.org>

-- Steve

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..538469dfb458 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -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

[Linus Torvalds]

On Fri, Dec 21, 2018 at 2:48 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Your patch actually had them, but in the body of your email you did
> >
> > > #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.

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

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 14:57:13 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Dec 21, 2018 at 2:48 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >  
> > > Your patch actually had them, but in the body of your email you did
> > >  
> > > > #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.  
> 
> 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.

> 
> 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?

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

[Malcolm Priestley]

On 21/12/2018 18:51, Linus Torvalds wrote:
> On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>>
>> 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.
> 
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.

Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Sat, 22 Dec 2018 10:20:02 +0000
Malcolm Priestley <tvboxspy@gmail.com> wrote:

> > 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

[for-next][PATCH 24/24] tracing: Use strncmp_prefix() helper for histogram code

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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.

Cc: Tom Zanussi <zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c5448c103770..a11653ac7087 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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);
-- 
2.19.2

Re: [for-next][PATCH 09/24] seq_buf: Make seq_buf_puts() null-terminate the buffer

[Steven Rostedt]

On Fri, 21 Dec 2018 12:56:27 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Michael Ellerman <mpe@ellerman.id.au>
> 
> 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);
> 
> 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

> 
> 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.
> 
> Link: http://lkml.kernel.org/r/20181019042109.8064-1-mpe@ellerman.id.au
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  lib/seq_buf.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
> index 11f2ae0f9099..6aabb609dd87 100644
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c
> @@ -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);

Re: [for-next][PATCH 09/24] seq_buf: Make seq_buf_puts() null-terminate the buffer

[Steven Rostedt]

On Fri, 21 Dec 2018 13:00:21 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:


> > 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

Re: [for-next][PATCH 09/24] seq_buf: Make seq_buf_puts() null-terminate the buffer

[Michael Ellerman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Fri, 21 Dec 2018 13:00:21 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
>> > 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.

Thanks, sorry for the bother. It doesn't really matter in this case
what's displayed, it's just an example.

cheers