* [for-next][PATCH 1/8] ftrace: Simplify ftrace hash lookup code in clear_func_from_hash()
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 2/8] tracing: Be more clever when dumping hex in __print_hex() Steven Rostedt
` (6 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Changbin Du
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [for-next][PATCH 2/8] tracing: Be more clever when dumping hex in __print_hex()
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 1/8] ftrace: Simplify ftrace hash lookup code in clear_func_from_hash() Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 3/8] tracing: Make sure variable reference alias has correct var_ref_idx Steven Rostedt
` (5 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Andy Shevchenko
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [for-next][PATCH 3/8] tracing: Make sure variable reference alias has correct var_ref_idx
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 1/8] ftrace: Simplify ftrace hash lookup code in clear_func_from_hash() Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 2/8] tracing: Be more clever when dumping hex in __print_hex() Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
[not found] ` <20190921120618.DF81120665@mail.kernel.org>
2019-09-19 23:23 ` [for-next][PATCH 4/8] tracing/kprobe: Fix NULL pointer access in trace_porbe_unlink() Steven Rostedt
` (4 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Linux Trace Devel, linux-rt-users,
stable, Tom Zanussi
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [for-next][PATCH 4/8] tracing/kprobe: Fix NULL pointer access in trace_porbe_unlink()
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
` (2 preceding siblings ...)
2019-09-19 23:23 ` [for-next][PATCH 3/8] tracing: Make sure variable reference alias has correct var_ref_idx Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 5/8] selftests/ftrace: Select an existing function in kprobe_eventname test Steven Rostedt
` (3 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, syzbot+2f807f4d3a2a4e87f18f,
Masami Hiramatsu
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [for-next][PATCH 5/8] selftests/ftrace: Select an existing function in kprobe_eventname test
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
` (3 preceding siblings ...)
2019-09-19 23:23 ` [for-next][PATCH 4/8] tracing/kprobe: Fix NULL pointer access in trace_porbe_unlink() Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 6/8] tracing/probe: Fix to allow user to enable events on unloaded modules Steven Rostedt
` (2 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Masami Hiramatsu
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [for-next][PATCH 6/8] tracing/probe: Fix to allow user to enable events on unloaded modules
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
` (4 preceding siblings ...)
2019-09-19 23:23 ` [for-next][PATCH 5/8] selftests/ftrace: Select an existing function in kprobe_eventname test Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event Steven Rostedt
2019-09-19 23:23 ` [for-next][PATCH 8/8] selftests/ftrace: Update kprobe event error testcase Steven Rostedt
7 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, stable, Masami Hiramatsu
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
` (5 preceding siblings ...)
2019-09-19 23:23 ` [for-next][PATCH 6/8] tracing/probe: Fix to allow user to enable events on unloaded modules Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
2019-09-23 10:42 ` Srikar Dronamraju
2019-09-19 23:23 ` [for-next][PATCH 8/8] selftests/ftrace: Update kprobe event error testcase Steven Rostedt
7 siblings, 1 reply; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Masami Hiramatsu
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event
2019-09-19 23:23 ` [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event Steven Rostedt
@ 2019-09-23 10:42 ` Srikar Dronamraju
2019-09-23 17:15 ` Masami Hiramatsu
0 siblings, 1 reply; 15+ messages in thread
From: Srikar Dronamraju @ 2019-09-23 10:42 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Masami Hiramatsu,
Naveen Rao, Ravi Bangoria
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event
2019-09-23 10:42 ` Srikar Dronamraju
@ 2019-09-23 17:15 ` Masami Hiramatsu
2019-09-23 17:42 ` Srikar Dronamraju
0 siblings, 1 reply; 15+ messages in thread
From: Masami Hiramatsu @ 2019-09-23 17:15 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Andrew Morton,
Masami Hiramatsu, Naveen Rao, Ravi Bangoria
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>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event
2019-09-23 17:15 ` Masami Hiramatsu
@ 2019-09-23 17:42 ` Srikar Dronamraju
0 siblings, 0 replies; 15+ messages in thread
From: Srikar Dronamraju @ 2019-09-23 17:42 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Andrew Morton,
Naveen Rao, Ravi Bangoria
> > 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [for-next][PATCH 8/8] selftests/ftrace: Update kprobe event error testcase
2019-09-19 23:23 [for-next][PATCH 0/8] tracing: Final updates before sending to Linus Steven Rostedt
` (6 preceding siblings ...)
2019-09-19 23:23 ` [for-next][PATCH 7/8] tracing/probe: Reject exactly same probe event Steven Rostedt
@ 2019-09-19 23:23 ` Steven Rostedt
7 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2019-09-19 23:23 UTC (permalink / raw)
To: linux-kernel; +Cc: Ingo Molnar, Andrew Morton, Masami Hiramatsu
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
^ permalink raw reply related [flat|nested] 15+ messages in thread