[for-next][PATCH 0/8] tracing: Final updates before sending to Linus

[for-next][PATCH 0/8] tracing: Final updates before sending to Linus

[Steven Rostedt]

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
for-next

Head SHA1: b78b94b82122208902c0f83805e614e1239f9893


Andy Shevchenko (1):
      tracing: Be more clever when dumping hex in __print_hex()

Changbin Du (1):
      ftrace: Simplify ftrace hash lookup code in clear_func_from_hash()

Masami Hiramatsu (4):
      tracing/kprobe: Fix NULL pointer access in trace_porbe_unlink()
      tracing/probe: Fix to allow user to enable events on unloaded modules
      tracing/probe: Reject exactly same probe event
      selftests/ftrace: Update kprobe event error testcase

Steven Rostedt (VMware) (1):
      selftests/ftrace: Select an existing function in kprobe_eventname test

Tom Zanussi (1):
      tracing: Make sure variable reference alias has correct var_ref_idx

----
 kernel/trace/ftrace.c                              |  6 +-
 kernel/trace/trace_events_hist.c                   |  2 +
 kernel/trace/trace_kprobe.c                        | 69 +++++++++++++++-------
 kernel/trace/trace_output.c                        |  6 +-
 kernel/trace/trace_probe.c                         | 11 ++--
 kernel/trace/trace_probe.h                         |  3 +-
 kernel/trace/trace_uprobe.c                        | 52 +++++++++++++---
 .../ftrace/test.d/kprobe/kprobe_eventname.tc       | 16 ++++-
 .../ftrace/test.d/kprobe/kprobe_syntax_errors.tc   |  1 +
 9 files changed, 123 insertions(+), 43 deletions(-)

[for-next][PATCH 1/8] ftrace: Simplify ftrace hash lookup code in clear_func_from_hash()

[Steven Rostedt]

From: Changbin Du <changbin.du@gmail.com>

Function ftrace_lookup_ip() will check empty hash table. So we don't
need extra check outside.

Link: http://lkml.kernel.org/r/20190910143336.13472-1-changbin.du@gmail.com

Signed-off-by: Changbin Du <changbin.du@gmail.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f9821a3374e9..c4cc048eb594 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6036,11 +6036,7 @@ clear_func_from_hash(struct ftrace_init_func *func, struct ftrace_hash *hash)
 {
 	struct ftrace_func_entry *entry;
 
-	if (ftrace_hash_empty(hash))
-		return;
-
-	entry = __ftrace_lookup_ip(hash, func->ip);
-
+	entry = ftrace_lookup_ip(hash, func->ip);
 	/*
 	 * Do not allow this rec to match again.
 	 * Yeah, it may waste some memory, but will be removed
-- 
2.20.1

[for-next][PATCH 2/8] tracing: Be more clever when dumping hex in __print_hex()

[Steven Rostedt]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Hex dump as many as 16 bytes at once in trace_print_hex_seq()
instead of byte-by-byte approach.

Link: http://lkml.kernel.org/r/20190806151543.86061-1-andriy.shevchenko@linux.intel.com

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_output.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index cab4a5398f1d..d54ce252b05a 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -219,10 +219,10 @@ trace_print_hex_seq(struct trace_seq *p, const unsigned char *buf, int buf_len,
 {
 	int i;
 	const char *ret = trace_seq_buffer_ptr(p);
+	const char *fmt = concatenate ? "%*phN" : "%*ph";
 
-	for (i = 0; i < buf_len; i++)
-		trace_seq_printf(p, "%s%2.2x", concatenate || i == 0 ? "" : " ",
-				 buf[i]);
+	for (i = 0; i < buf_len; i += 16)
+		trace_seq_printf(p, fmt, min(buf_len - i, 16), &buf[i]);
 	trace_seq_putc(p, 0);
 
 	return ret;
-- 
2.20.1

[for-next][PATCH 3/8] tracing: Make sure variable reference alias has correct var_ref_idx

[Steven Rostedt]

From: Tom Zanussi <zanussi@kernel.org>

Original changelog from Steve Rostedt (except last sentence which
explains the problem, and the Fixes: tag):

I performed a three way histogram with the following commands:

echo 'irq_lat u64 lat pid_t pid' > synthetic_events
echo 'wake_lat u64 lat u64 irqlat pid_t pid' >> synthetic_events
echo 'hist:keys=common_pid:irqts=common_timestamp.usecs if function == 0xffffffff81200580' > events/timer/hrtimer_start/trigger
echo 'hist:keys=common_pid:lat=common_timestamp.usecs-$irqts:onmatch(timer.hrtimer_start).irq_lat($lat,pid) if common_flags & 1' > events/sched/sched_waking/trigger
echo 'hist:keys=pid:wakets=common_timestamp.usecs,irqlat=lat' > events/synthetic/irq_lat/trigger
echo 'hist:keys=next_pid:lat=common_timestamp.usecs-$wakets,irqlat=$irqlat:onmatch(synthetic.irq_lat).wake_lat($lat,$irqlat,next_pid)' > events/sched/sched_switch/trigger
echo 1 > events/synthetic/wake_lat/enable

Basically I wanted to see:

 hrtimer_start (calling function tick_sched_timer)

Note:

  # grep tick_sched_timer /proc/kallsyms
ffffffff81200580 t tick_sched_timer

And save the time of that, and then record sched_waking if it is called
in interrupt context and with the same pid as the hrtimer_start, it
will record the latency between that and the waking event.

I then look at when the task that is woken is scheduled in, and record
the latency between the wakeup and the task running.

At the end, the wake_lat synthetic event will show the wakeup to
scheduled latency, as well as the irq latency in from hritmer_start to
the wakeup. The problem is that I found this:

          <idle>-0     [007] d...   190.485261: wake_lat: lat=27 irqlat=190485230 pid=698
          <idle>-0     [005] d...   190.485283: wake_lat: lat=40 irqlat=190485239 pid=10
          <idle>-0     [002] d...   190.488327: wake_lat: lat=56 irqlat=190488266 pid=335
          <idle>-0     [005] d...   190.489330: wake_lat: lat=64 irqlat=190489262 pid=10
          <idle>-0     [003] d...   190.490312: wake_lat: lat=43 irqlat=190490265 pid=77
          <idle>-0     [005] d...   190.493322: wake_lat: lat=54 irqlat=190493262 pid=10
          <idle>-0     [005] d...   190.497305: wake_lat: lat=35 irqlat=190497267 pid=10
          <idle>-0     [005] d...   190.501319: wake_lat: lat=50 irqlat=190501264 pid=10

The irqlat seemed quite large! Investigating this further, if I had
enabled the irq_lat synthetic event, I noticed this:

          <idle>-0     [002] d.s.   249.429308: irq_lat: lat=164968 pid=335
          <idle>-0     [002] d...   249.429369: wake_lat: lat=55 irqlat=249429308 pid=335

Notice that the timestamp of the irq_lat "249.429308" is awfully
similar to the reported irqlat variable. In fact, all instances were
like this. It appeared that:

  irqlat=$irqlat

Wasn't assigning the old $irqlat to the new irqlat variable, but
instead was assigning the $irqts to it.

The issue is that assigning the old $irqlat to the new irqlat variable
creates a variable reference alias, but the alias creation code
forgets to make sure the alias uses the same var_ref_idx to access the
reference.

Link: http://lkml.kernel.org/r/1567375321.5282.12.camel@kernel.org

Cc: Linux Trace Devel <linux-trace-devel@vger.kernel.org>
Cc: linux-rt-users <linux-rt-users@vger.kernel.org>
Cc: stable@vger.kernel.org
Fixes: 7e8b88a30b085 ("tracing: Add hist trigger support for variable reference aliases")
Reported-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 3a6e42aa08e6..9468bd8d44a2 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2804,6 +2804,8 @@ static struct hist_field *create_alias(struct hist_trigger_data *hist_data,
 		return NULL;
 	}
 
+	alias->var_ref_idx = var_ref->var_ref_idx;
+
 	return alias;
 }
 
-- 
2.20.1

[for-next][PATCH 4/8] tracing/kprobe: Fix NULL pointer access in trace_porbe_unlink()

[Steven Rostedt]

From: Masami Hiramatsu <mhiramat@kernel.org>

Fix NULL pointer access in trace_probe_unlink() by initializing
trace_probe.list correctly in trace_probe_init().

In the error case of trace_probe_init(), it can call trace_probe_unlink()
before initializing trace_probe.list member. This causes NULL pointer
dereference at list_del_init() in trace_probe_unlink().

Syzbot reported :

kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 8633 Comm: syz-executor797 Not tainted 5.3.0-rc8-next-20190915
#0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:__list_del_entry_valid+0x85/0xf5 lib/list_debug.c:51
Code: 0f 84 e1 00 00 00 48 b8 22 01 00 00 00 00 ad de 49 39 c4 0f 84 e2 00
00 00 48 b8 00 00 00 00 00 fc ff df 4c 89 e2 48 c1 ea 03 <80> 3c 02 00 75
53 49 8b 14 24 4c 39 f2 0f 85 99 00 00 00 49 8d 7d
RSP: 0018:ffff888090a7f9d8 EFLAGS: 00010246
RAX: dffffc0000000000 RBX: ffff88809b6f90c0 RCX: ffffffff817c0ca9
RDX: 0000000000000000 RSI: ffffffff817c0a73 RDI: ffff88809b6f90c8
RBP: ffff888090a7f9f0 R08: ffff88809a04e600 R09: ffffed1015d26aed
R10: ffffed1015d26aec R11: ffff8880ae935763 R12: 0000000000000000
R13: 0000000000000000 R14: ffff88809b6f90c0 R15: ffff88809b6f90d0
FS:  0000555556f99880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000006cc090 CR3: 00000000962b2000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
  __list_del_entry include/linux/list.h:131 [inline]
  list_del_init include/linux/list.h:190 [inline]
  trace_probe_unlink+0x1f/0x200 kernel/trace/trace_probe.c:959
  trace_probe_cleanup+0xd3/0x110 kernel/trace/trace_probe.c:973
  trace_probe_init+0x3f2/0x510 kernel/trace/trace_probe.c:1011
  alloc_trace_uprobe+0x5e/0x250 kernel/trace/trace_uprobe.c:353
  create_local_trace_uprobe+0x109/0x4a0 kernel/trace/trace_uprobe.c:1508
  perf_uprobe_init+0x131/0x210 kernel/trace/trace_event_perf.c:314
  perf_uprobe_event_init+0x106/0x1a0 kernel/events/core.c:8898
  perf_try_init_event+0x135/0x590 kernel/events/core.c:10184
  perf_init_event kernel/events/core.c:10228 [inline]
  perf_event_alloc.part.0+0x1b89/0x33d0 kernel/events/core.c:10505
  perf_event_alloc kernel/events/core.c:10887 [inline]
  __do_sys_perf_event_open+0xa2d/0x2d00 kernel/events/core.c:10989
  __se_sys_perf_event_open kernel/events/core.c:10871 [inline]
  __x64_sys_perf_event_open+0xbe/0x150 kernel/events/core.c:10871
  do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Link: http://lkml.kernel.org/r/156869709721.22406.5153754822203046939.stgit@devnote2

Reported-by: syzbot+2f807f4d3a2a4e87f18f@syzkaller.appspotmail.com
Fixes: ca89bc071d5e ("tracing/kprobe: Add multi-probe per event support")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_probe.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 1e67fef06e53..baf58a3612c0 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -986,6 +986,12 @@ int trace_probe_init(struct trace_probe *tp, const char *event,
 	if (!tp->event)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&tp->event->files);
+	INIT_LIST_HEAD(&tp->event->class.fields);
+	INIT_LIST_HEAD(&tp->event->probes);
+	INIT_LIST_HEAD(&tp->list);
+	list_add(&tp->event->probes, &tp->list);
+
 	call = trace_probe_event_call(tp);
 	call->class = &tp->event->class;
 	call->name = kstrdup(event, GFP_KERNEL);
@@ -999,11 +1005,6 @@ int trace_probe_init(struct trace_probe *tp, const char *event,
 		ret = -ENOMEM;
 		goto error;
 	}
-	INIT_LIST_HEAD(&tp->event->files);
-	INIT_LIST_HEAD(&tp->event->class.fields);
-	INIT_LIST_HEAD(&tp->event->probes);
-	INIT_LIST_HEAD(&tp->list);
-	list_add(&tp->event->probes, &tp->list);
 
 	return 0;
 
-- 
2.20.1

[for-next][PATCH 5/8] selftests/ftrace: Select an existing function in kprobe_eventname test

[Steven Rostedt]

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

Running the ftrace selftests on the latest kernel caused the
kprobe_eventname test to fail. It was due to the test that searches for
a function with at "dot" in the name and adding a probe to that.
Unfortunately, for this test, it picked:

 optimize_nops.isra.2.cold.4

Which happens to be marked as "__init", which means it no longer exists
in the kernel! (kallsyms keeps those function names around for tracing
purposes)

As only functions that still exist are in the
available_filter_functions file, as they are removed when the functions
are freed at boot or module exit, have the test search for a function
with ".isra." in the name as well as being in the
available_filter_functions (if the file exists).

Link: http://lkml.kernel.org/r/20190322150923.1b58eca5@gandalf.local.home

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 .../ftrace/test.d/kprobe/kprobe_eventname.tc     | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
index 3fb70e01b1fe..3ff236719b6e 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_eventname.tc
@@ -24,7 +24,21 @@ test -d events/kprobes2/event2 || exit_failure
 
 :;: "Add an event on dot function without name" ;:
 
-FUNC=`grep -m 10 " [tT] .*\.isra\..*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "`
+find_dot_func() {
+	if [ ! -f available_filter_functions ]; then
+		grep -m 10 " [tT] .*\.isra\..*$" /proc/kallsyms | tail -n 1 | cut -f 3 -d " "
+		return;
+	fi
+
+	grep " [tT] .*\.isra\..*" /proc/kallsyms | cut -f 3 -d " " | while read f; do
+		if grep -s $f available_filter_functions; then
+			echo $f
+			break
+		fi
+	done
+}
+
+FUNC=`find_dot_func | tail -n 1`
 [ "x" != "x$FUNC" ] || exit_unresolved
 echo "p $FUNC" > kprobe_events
 EVENT=`grep $FUNC kprobe_events | cut -f 1 -d " " | cut -f 2 -d:`
-- 
2.20.1

[for-next][PATCH 6/8] tracing/probe: Fix to allow user to enable events on unloaded modules

[Steven Rostedt]

From: Masami Hiramatsu <mhiramat@kernel.org>

Fix to allow user to enable probe events on unloaded modules.

This operations was allowed before commit 60d53e2c3b75 ("tracing/probe:
Split trace_event related data from trace_probe"), because if users
need to probe module init functions, they have to enable those probe
events before loading module.

Link: http://lkml.kernel.org/r/156879693733.31056.9331322616994665167.stgit@devnote2

Cc: stable@vger.kernel.org
Fixes: 60d53e2c3b75 ("tracing/probe: Split trace_event related data from trace_probe")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_kprobe.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 7579c53bb053..0ba3239c0270 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -371,31 +371,24 @@ static int enable_trace_kprobe(struct trace_event_call *call,
 	if (enabled)
 		return 0;
 
-	enabled = false;
 	list_for_each_entry(pos, trace_probe_probe_list(tp), list) {
 		tk = container_of(pos, struct trace_kprobe, tp);
 		if (trace_kprobe_has_gone(tk))
 			continue;
 		ret = __enable_trace_kprobe(tk);
-		if (ret) {
-			if (enabled) {
-				__disable_trace_kprobe(tp);
-				enabled = false;
-			}
+		if (ret)
 			break;
-		}
 		enabled = true;
 	}
 
-	if (!enabled) {
-		/* No probe is enabled. Roll back */
+	if (ret) {
+		/* Failed to enable one of them. Roll back all */
+		if (enabled)
+			__disable_trace_kprobe(tp);
 		if (file)
 			trace_probe_remove_file(tp, file);
 		else
 			trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
-		if (!ret)
-			/* Since all probes are gone, this is not available */
-			ret = -EADDRNOTAVAIL;
 	}
 
 	return ret;
-- 
2.20.1

[for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event

[Steven Rostedt]

From: Masami Hiramatsu <mhiramat@kernel.org>

Reject exactly same probe events as existing probes.

Multiprobe allows user to define multiple probes on same
event. If user appends a probe which exactly same definition
(same probe address and same arguments) on existing event,
the event will record same probe information twice.
That can be confusing users, so reject it.

Link: http://lkml.kernel.org/r/156879694602.31056.5533024778165036763.stgit@devnote2

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_kprobe.c | 52 +++++++++++++++++++++++++++++++------
 kernel/trace/trace_probe.h  |  3 ++-
 kernel/trace/trace_uprobe.c | 52 +++++++++++++++++++++++++++++++------
 3 files changed, 90 insertions(+), 17 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 0ba3239c0270..a6697e28ddda 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -528,10 +528,53 @@ static int unregister_trace_kprobe(struct trace_kprobe *tk)
 	return 0;
 }
 
+static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig,
+					 struct trace_kprobe *comp)
+{
+	struct trace_probe_event *tpe = orig->tp.event;
+	struct trace_probe *pos;
+	int i;
+
+	list_for_each_entry(pos, &tpe->probes, list) {
+		orig = container_of(pos, struct trace_kprobe, tp);
+		if (strcmp(trace_kprobe_symbol(orig),
+			   trace_kprobe_symbol(comp)) ||
+		    trace_kprobe_offset(orig) != trace_kprobe_offset(comp))
+			continue;
+
+		/*
+		 * trace_probe_compare_arg_type() ensured that nr_args and
+		 * each argument name and type are same. Let's compare comm.
+		 */
+		for (i = 0; i < orig->tp.nr_args; i++) {
+			if (strcmp(orig->tp.args[i].comm,
+				   comp->tp.args[i].comm))
+				continue;
+		}
+
+		return true;
+	}
+
+	return false;
+}
+
 static int append_trace_kprobe(struct trace_kprobe *tk, struct trace_kprobe *to)
 {
 	int ret;
 
+	ret = trace_probe_compare_arg_type(&tk->tp, &to->tp);
+	if (ret) {
+		/* Note that argument starts index = 2 */
+		trace_probe_log_set_index(ret + 1);
+		trace_probe_log_err(0, DIFF_ARG_TYPE);
+		return -EEXIST;
+	}
+	if (trace_kprobe_has_same_kprobe(to, tk)) {
+		trace_probe_log_set_index(0);
+		trace_probe_log_err(0, SAME_PROBE);
+		return -EEXIST;
+	}
+
 	/* Append to existing event */
 	ret = trace_probe_append(&tk->tp, &to->tp);
 	if (ret)
@@ -568,14 +611,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
 			trace_probe_log_err(0, DIFF_PROBE_TYPE);
 			ret = -EEXIST;
 		} else {
-			ret = trace_probe_compare_arg_type(&tk->tp, &old_tk->tp);
-			if (ret) {
-				/* Note that argument starts index = 2 */
-				trace_probe_log_set_index(ret + 1);
-				trace_probe_log_err(0, DIFF_ARG_TYPE);
-				ret = -EEXIST;
-			} else
-				ret = append_trace_kprobe(tk, old_tk);
+			ret = append_trace_kprobe(tk, old_tk);
 		}
 		goto end;
 	}
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index f805cc4cbe7c..4ee703728aec 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -436,7 +436,8 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(BAD_INSN_BNDRY,	"Probe point is not an instruction boundary"),\
 	C(FAIL_REG_PROBE,	"Failed to register probe event"),\
 	C(DIFF_PROBE_TYPE,	"Probe type is different from existing probe"),\
-	C(DIFF_ARG_TYPE,	"Argument type or name is different from existing probe"),
+	C(DIFF_ARG_TYPE,	"Argument type or name is different from existing probe"),\
+	C(SAME_PROBE,		"There is already the exact same probe event"),
 
 #undef C
 #define C(a, b)		TP_ERR_##a
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index cbf4da4bf367..34dd6d0016a3 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -410,10 +410,53 @@ static int unregister_trace_uprobe(struct trace_uprobe *tu)
 	return 0;
 }
 
+static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig,
+					 struct trace_uprobe *comp)
+{
+	struct trace_probe_event *tpe = orig->tp.event;
+	struct trace_probe *pos;
+	struct inode *comp_inode = d_real_inode(comp->path.dentry);
+	int i;
+
+	list_for_each_entry(pos, &tpe->probes, list) {
+		orig = container_of(pos, struct trace_uprobe, tp);
+		if (comp_inode != d_real_inode(orig->path.dentry) ||
+		    comp->offset != orig->offset)
+			continue;
+
+		/*
+		 * trace_probe_compare_arg_type() ensured that nr_args and
+		 * each argument name and type are same. Let's compare comm.
+		 */
+		for (i = 0; i < orig->tp.nr_args; i++) {
+			if (strcmp(orig->tp.args[i].comm,
+				   comp->tp.args[i].comm))
+				continue;
+		}
+
+		return true;
+	}
+
+	return false;
+}
+
 static int append_trace_uprobe(struct trace_uprobe *tu, struct trace_uprobe *to)
 {
 	int ret;
 
+	ret = trace_probe_compare_arg_type(&tu->tp, &to->tp);
+	if (ret) {
+		/* Note that argument starts index = 2 */
+		trace_probe_log_set_index(ret + 1);
+		trace_probe_log_err(0, DIFF_ARG_TYPE);
+		return -EEXIST;
+	}
+	if (trace_uprobe_has_same_uprobe(to, tu)) {
+		trace_probe_log_set_index(0);
+		trace_probe_log_err(0, SAME_PROBE);
+		return -EEXIST;
+	}
+
 	/* Append to existing event */
 	ret = trace_probe_append(&tu->tp, &to->tp);
 	if (!ret)
@@ -469,14 +512,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
 			trace_probe_log_err(0, DIFF_PROBE_TYPE);
 			ret = -EEXIST;
 		} else {
-			ret = trace_probe_compare_arg_type(&tu->tp, &old_tu->tp);
-			if (ret) {
-				/* Note that argument starts index = 2 */
-				trace_probe_log_set_index(ret + 1);
-				trace_probe_log_err(0, DIFF_ARG_TYPE);
-				ret = -EEXIST;
-			} else
-				ret = append_trace_uprobe(tu, old_tu);
+			ret = append_trace_uprobe(tu, old_tu);
 		}
 		goto end;
 	}
-- 
2.20.1

[for-next][PATCH 8/8] selftests/ftrace: Update kprobe event error testcase

[Steven Rostedt]

From: Masami Hiramatsu <mhiramat@kernel.org>

Update kprobe event error testcase to test if it correctly
finds the exact same probe event.

Link: http://lkml.kernel.org/r/156879695513.31056.1580235733738840126.stgit@devnote2

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 .../selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc       | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
index 39ef7ac1f51c..8a4025e912cb 100644
--- a/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/kprobe/kprobe_syntax_errors.tc
@@ -95,6 +95,7 @@ echo 'p:kprobes/testevent _do_fork abcd=\1' > kprobe_events
 check_error 'p:kprobes/testevent _do_fork ^bcd=\1'	# DIFF_ARG_TYPE
 check_error 'p:kprobes/testevent _do_fork ^abcd=\1:u8'	# DIFF_ARG_TYPE
 check_error 'p:kprobes/testevent _do_fork ^abcd=\"foo"'	# DIFF_ARG_TYPE
+check_error '^p:kprobes/testevent _do_fork'	# SAME_PROBE
 fi
 
 exit 0
-- 
2.20.1

Re: [for-next][PATCH 3/8] tracing: Make sure variable reference alias has correct var_ref_idx

[Steven Rostedt]

On Sat, 21 Sep 2019 12:06:18 +0000
Sasha Levin <sashal@kernel.org> wrote:

> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: .
> 
> The bot has tested the following trees: v5.2.16, v4.19.74, v4.14.145, v4.9.193, v4.4.193.


The fixes tag is 7e8b88a30b085 which was added to mainline in 4.17.
According to this email, it applies fine to 5.2 and 4.19, but fails on
4.14 and earlier. As the commit was added in 4.17 that makes perfect
sense. Can you update your scripts to test when the fixes commit was
added, and not send spam about it not applying to stable trees where
it's not applicable.

On a git repo containing only Linus's tree, I have:

$ git describe --contains 7e8b88a30b085
v4.17-rc1~28^2~43

Which shows me when it was applied.

Thanks!

-- Steve



> 
> v5.2.16: Build OK!
> v4.19.74: Build OK!
> v4.14.145: Failed to apply! Possible dependencies:
>     00b4145298ae ("ring-buffer: Add interface for setting absolute time stamps")
>     067fe038e70f ("tracing: Add variable reference handling to hist triggers")
>     0d7a8325bf33 ("tracing: Clean up hist_field_flags enum")
>     100719dcef44 ("tracing: Add simple expression support to hist triggers")
>     30350d65ac56 ("tracing: Add variable support to hist triggers")
>     442c94846190 ("tracing: Add Documentation for log2 modifier")
>     5819eaddf35b ("tracing: Reimplement log2")
>     7e8b88a30b08 ("tracing: Add hist trigger support for variable reference aliases")
>     85013256cf01 ("tracing: Add hist_field_name() accessor")
>     860f9f6b02e9 ("tracing: Add usecs modifier for hist trigger timestamps")
>     8b7622bf94a4 ("tracing: Add cpu field for hist triggers")
>     ad42febe51ae ("tracing: Add hist trigger timestamp support")
>     b559d003a226 ("tracing: Add hist_data member to hist_field")
>     b8df4a3634e0 ("tracing: Move hist trigger Documentation to histogram.txt")
> 
> v4.9.193: Failed to apply! Possible dependencies:
>     00b4145298ae ("ring-buffer: Add interface for setting absolute time stamps")
>     067fe038e70f ("tracing: Add variable reference handling to hist triggers")
>     0d7a8325bf33 ("tracing: Clean up hist_field_flags enum")
>     100719dcef44 ("tracing: Add simple expression support to hist triggers")
>     30350d65ac56 ("tracing: Add variable support to hist triggers")
>     442c94846190 ("tracing: Add Documentation for log2 modifier")
>     5819eaddf35b ("tracing: Reimplement log2")
>     7e8b88a30b08 ("tracing: Add hist trigger support for variable reference aliases")
>     85013256cf01 ("tracing: Add hist_field_name() accessor")
>     860f9f6b02e9 ("tracing: Add usecs modifier for hist trigger timestamps")
>     8b7622bf94a4 ("tracing: Add cpu field for hist triggers")
>     ad42febe51ae ("tracing: Add hist trigger timestamp support")
>     b559d003a226 ("tracing: Add hist_data member to hist_field")
>     b8df4a3634e0 ("tracing: Move hist trigger Documentation to histogram.txt")
> 
> v4.4.193: Failed to apply! Possible dependencies:
>     08d43a5fa063 ("tracing: Add lock-free tracing_map")
>     0c4a6b4666e8 ("tracing: Add hist trigger 'hex' modifier for displaying numeric fields")
>     0fc3813ce103 ("tracing: Add 'hist' trigger Documentation")
>     52a7f16dedff ("tracing: Add support for multiple hist triggers per event")
>     5463bfda327b ("tracing: Add support for named hist triggers")
>     76a3b0c8ac34 ("tracing: Add hist trigger support for compound keys")
>     7e8b88a30b08 ("tracing: Add hist trigger support for variable reference aliases")
>     7ef224d1d0e3 ("tracing: Add 'hist' event trigger command")
>     83e99914c9e2 ("tracing: Add hist trigger support for pausing and continuing a trace")
>     8b7622bf94a4 ("tracing: Add cpu field for hist triggers")
>     b8df4a3634e0 ("tracing: Move hist trigger Documentation to histogram.txt")
>     c6afad49d127 ("tracing: Add hist trigger 'sym' and 'sym-offset' modifiers")
>     e62347d24534 ("tracing: Add hist trigger support for user-defined sorting ('sort=' param)")
>     f2606835d70d ("tracing: Add hist trigger support for multiple values ('vals=' param)")
> 
> 
> NOTE: The patch will not be queued to stable trees until it is upstream.
> 
> How should we proceed with this patch?
> 
> --
> Thanks,
> Sasha

Re: [for-next][PATCH 3/8] tracing: Make sure variable reference alias has correct var_ref_idx

[Sasha Levin]

On Sat, Sep 21, 2019 at 08:20:35AM -0400, Steven Rostedt wrote:
>On Sat, 21 Sep 2019 12:06:18 +0000
>Sasha Levin <sashal@kernel.org> wrote:
>
>> Hi,
>>
>> [This is an automated email]
>>
>> This commit has been processed because it contains a "Fixes:" tag,
>> fixing commit: .
>>
>> The bot has tested the following trees: v5.2.16, v4.19.74, v4.14.145, v4.9.193, v4.4.193.
>
>
>The fixes tag is 7e8b88a30b085 which was added to mainline in 4.17.
>According to this email, it applies fine to 5.2 and 4.19, but fails on
>4.14 and earlier. As the commit was added in 4.17 that makes perfect
>sense. Can you update your scripts to test when the fixes commit was
>added, and not send spam about it not applying to stable trees where
>it's not applicable.

The script already does that. What happened here is that it got confused
with your previous "Fixes:" statement in the commit message and went
haywire.

I thought that something like this shouldn't happen because I grep for
"^fixes:", but looks like something is broken. I'll go fix that...


--
Thanks,
Sasha

Re: [for-next][PATCH 3/8] tracing: Make sure variable reference alias has correct var_ref_idx

[Steven Rostedt]

On September 21, 2019 3:21:08 PM EDT, Sasha Levin <sashal@kernel.org> wrote:
>On Sat, Sep 21, 2019 at 08:20:35AM -0400, Steven Rostedt wrote:
>>On Sat, 21 Sep 2019 12:06:18 +0000
>>Sasha Levin <sashal@kernel.org> wrote:
>>
>>> Hi,
>>>
>>> [This is an automated email]
>>>
>>> This commit has been processed because it contains a "Fixes:" tag,
>>> fixing commit: .
>>>
>>> The bot has tested the following trees: v5.2.16, v4.19.74,
>v4.14.145, v4.9.193, v4.4.193.
>>
>>
>>The fixes tag is 7e8b88a30b085 which was added to mainline in 4.17.
>>According to this email, it applies fine to 5.2 and 4.19, but fails on
>>4.14 and earlier. As the commit was added in 4.17 that makes perfect
>>sense. Can you update your scripts to test when the fixes commit was
>>added, and not send spam about it not applying to stable trees where
>>it's not applicable.
>
>The script already does that. What happened here is that it got
>confused
>with your previous "Fixes:" statement in the commit message and went
>haywire.
>
>I thought that something like this shouldn't happen because I grep for
>"^fixes:", but looks like something is broken. I'll go fix that...


Thanks!

-- Steve

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity and top posting.

Re: [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event

[Srikar Dronamraju]

Hey Masami, Steven

>  
> +static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig,
> +					 struct trace_kprobe *comp)
> +{
> +	struct trace_probe_event *tpe = orig->tp.event;
> +	struct trace_probe *pos;
> +	int i;
> +
> +	list_for_each_entry(pos, &tpe->probes, list) {
> +		orig = container_of(pos, struct trace_kprobe, tp);
> +		if (strcmp(trace_kprobe_symbol(orig),
> +			   trace_kprobe_symbol(comp)) ||
> +		    trace_kprobe_offset(orig) != trace_kprobe_offset(comp))
> +			continue;
> +
> +		/*
> +		 * trace_probe_compare_arg_type() ensured that nr_args and
> +		 * each argument name and type are same. Let's compare comm.
> +		 */
> +		for (i = 0; i < orig->tp.nr_args; i++) {
> +			if (strcmp(orig->tp.args[i].comm,
> +				   comp->tp.args[i].comm))
> +				continue;

In a nested loop, *continue* is going to continue iterating through the
inner loop. In which case, continue is doing nothing here. I thought we
should have used a goto instead. No?  To me, continue as a last statement of
a for loop always looks weird.

> +		}
> +
> +		return true;
> +	}

I think we need something like this:

	list_for_each_entry(pos, &tpe->probes, list) {
		orig = container_of(pos, struct trace_kprobe, tp);
		if (strcmp(trace_kprobe_symbol(orig),
			   trace_kprobe_symbol(comp)) ||
		    trace_kprobe_offset(orig) != trace_kprobe_offset(comp))
			continue;

		/*
		 * trace_probe_compare_arg_type() ensured that nr_args and
		 * each argument name and type are same. Let's compare comm.
		 */
		for (i = 0; i < orig->tp.nr_args; i++) {
			if (strcmp(orig->tp.args[i].comm,
				   comp->tp.args[i].comm))
				goto outer_loop;

		}

		return true;
outer_loop:
	}


> +
> +	return false;
> +}
> +
>  

......

> +static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig,
> +					 struct trace_uprobe *comp)
> +{
> +	struct trace_probe_event *tpe = orig->tp.event;
> +	struct trace_probe *pos;
> +	struct inode *comp_inode = d_real_inode(comp->path.dentry);
> +	int i;
> +
> +	list_for_each_entry(pos, &tpe->probes, list) {
> +		orig = container_of(pos, struct trace_uprobe, tp);
> +		if (comp_inode != d_real_inode(orig->path.dentry) ||
> +		    comp->offset != orig->offset)
> +			continue;
> +
> +		/*
> +		 * trace_probe_compare_arg_type() ensured that nr_args and
> +		 * each argument name and type are same. Let's compare comm.
> +		 */
> +		for (i = 0; i < orig->tp.nr_args; i++) {
> +			if (strcmp(orig->tp.args[i].comm,
> +				   comp->tp.args[i].comm))
> +				continue;

Same as above.

> +		}
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +

-- 
Thanks and Regards
Srikar Dronamraju

Re: [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event

[Masami Hiramatsu]

On Mon, 23 Sep 2019 16:12:53 +0530
Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> Hey Masami, Steven
> 
> >  
> > +static bool trace_kprobe_has_same_kprobe(struct trace_kprobe *orig,
> > +					 struct trace_kprobe *comp)
> > +{
> > +	struct trace_probe_event *tpe = orig->tp.event;
> > +	struct trace_probe *pos;
> > +	int i;
> > +
> > +	list_for_each_entry(pos, &tpe->probes, list) {
> > +		orig = container_of(pos, struct trace_kprobe, tp);
> > +		if (strcmp(trace_kprobe_symbol(orig),
> > +			   trace_kprobe_symbol(comp)) ||
> > +		    trace_kprobe_offset(orig) != trace_kprobe_offset(comp))
> > +			continue;
> > +
> > +		/*
> > +		 * trace_probe_compare_arg_type() ensured that nr_args and
> > +		 * each argument name and type are same. Let's compare comm.
> > +		 */
> > +		for (i = 0; i < orig->tp.nr_args; i++) {
> > +			if (strcmp(orig->tp.args[i].comm,
> > +				   comp->tp.args[i].comm))
> > +				continue;
> 
> In a nested loop, *continue* is going to continue iterating through the
> inner loop. In which case, continue is doing nothing here. I thought we
> should have used a goto instead. No?  To me, continue as a last statement of
> a for loop always looks weird.

Oops, thanks for pointing it out!

> 
> > +		}
> > +
> > +		return true;
> > +	}
> 
> I think we need something like this:
> 
> 	list_for_each_entry(pos, &tpe->probes, list) {
> 		orig = container_of(pos, struct trace_kprobe, tp);
> 		if (strcmp(trace_kprobe_symbol(orig),
> 			   trace_kprobe_symbol(comp)) ||
> 		    trace_kprobe_offset(orig) != trace_kprobe_offset(comp))
> 			continue;
> 
> 		/*
> 		 * trace_probe_compare_arg_type() ensured that nr_args and
> 		 * each argument name and type are same. Let's compare comm.
> 		 */
> 		for (i = 0; i < orig->tp.nr_args; i++) {
> 			if (strcmp(orig->tp.args[i].comm,
> 				   comp->tp.args[i].comm))
> 				goto outer_loop;
> 
> 		}
> 
> 		return true;
> outer_loop:
> 	}

Correct, that's what I intended.
Could you make a fix patch on top of it? (or do I?)

Thank you,

> 
> 
> > +
> > +	return false;
> > +}
> > +
> >  
> 
> ......
> 
> > +static bool trace_uprobe_has_same_uprobe(struct trace_uprobe *orig,
> > +					 struct trace_uprobe *comp)
> > +{
> > +	struct trace_probe_event *tpe = orig->tp.event;
> > +	struct trace_probe *pos;
> > +	struct inode *comp_inode = d_real_inode(comp->path.dentry);
> > +	int i;
> > +
> > +	list_for_each_entry(pos, &tpe->probes, list) {
> > +		orig = container_of(pos, struct trace_uprobe, tp);
> > +		if (comp_inode != d_real_inode(orig->path.dentry) ||
> > +		    comp->offset != orig->offset)
> > +			continue;
> > +
> > +		/*
> > +		 * trace_probe_compare_arg_type() ensured that nr_args and
> > +		 * each argument name and type are same. Let's compare comm.
> > +		 */
> > +		for (i = 0; i < orig->tp.nr_args; i++) {
> > +			if (strcmp(orig->tp.args[i].comm,
> > +				   comp->tp.args[i].comm))
> > +				continue;
> 
> Same as above.
> 
> > +		}
> > +
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> 
> -- 
> Thanks and Regards
> Srikar Dronamraju
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event

[Srikar Dronamraju]

> > I think we need something like this:
> > 
> > 	list_for_each_entry(pos, &tpe->probes, list) {
> > 		orig = container_of(pos, struct trace_kprobe, tp);
> > 		if (strcmp(trace_kprobe_symbol(orig),
> > 			   trace_kprobe_symbol(comp)) ||
> > 		    trace_kprobe_offset(orig) != trace_kprobe_offset(comp))
> > 			continue;
> > 
> > 		/*
> > 		 * trace_probe_compare_arg_type() ensured that nr_args and
> > 		 * each argument name and type are same. Let's compare comm.
> > 		 */
> > 		for (i = 0; i < orig->tp.nr_args; i++) {
> > 			if (strcmp(orig->tp.args[i].comm,
> > 				   comp->tp.args[i].comm))
> > 				goto outer_loop;
> > 
> > 		}
> > 
> > 		return true;
> > outer_loop:
> > 	}
> 
> Correct, that's what I intended.
> Could you make a fix patch on top of it? (or do I?)
> 
> Thank you,

Either way is fine. I can send out a patch tomorrow. But fine if you beat
me to it.

-- 
Thanks and Regards
Srikar Dronamraju