take kprobe event as example, when create a group of events,
p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
according to this format, we must input EVENT name,
this change allow only GRP/ input, EVENT name auto generate from KSYM,
p[:[GRP/][EVENT]] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
siliar change apply to eprobe and uprobe.
V1: https://lore.kernel.org/lkml/[email protected]/
V2: fix remove comment in V1 patch1,
remove v1 patch2 as it is NACK.
v3: (v2 link: https://lore.kernel.org/lkml/[email protected]/)
add selftest cases for kprobe and eprobe event,
remove macro used in v1,v2,
change location to generate eprobe event name.
v4: (v3 link: https://lore.kernel.org/lkml/[email protected]/)
fix comment of kprobe/eprobe test case.
v5: (v4: https://lore.kernel.org/lkml/[email protected]/)
for eprobe, when only input a "SYSTEM.", it is invalid.
add Acked-by from Masami Hiramatsu (Google) <[email protected]>
Linyu Yuan (3):
tracing: eprobe: remove duplicate is_good_name() operation
tracing: auto generate event name when create a group of events
selftests/ftrace: add test case for GRP/ only input
Documentation/trace/kprobetrace.rst | 8 +++----
Documentation/trace/uprobetracer.rst | 8 +++----
kernel/trace/trace.c | 8 +++----
kernel/trace/trace_dynevent.c | 2 +-
kernel/trace/trace_eprobe.c | 27 +++++++++++-----------
kernel/trace/trace_kprobe.c | 16 ++++++++-----
kernel/trace/trace_probe.c | 6 +++++
kernel/trace/trace_uprobe.c | 12 ++++++----
.../ftrace/test.d/dynevent/add_remove_eprobe.tc | 9 +++++++-
.../ftrace/test.d/dynevent/add_remove_kprobe.tc | 7 ++++++
10 files changed, 65 insertions(+), 38 deletions(-)
--
2.7.4
Currently when create a specific group of trace events,
take kprobe event as example, user must use the following format:
p:GRP/EVENT [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
which means user must enter EVENT name, one example is: echo
'p:usb_gadget/config_usb_cfg_link config_usb_cfg_link $arg1' >>
kprobe_events, it is not simple if there are too many entries
because the event name is same as symbol name.
This change allows user to specify no EVENT name, format changed as:
p:GRP/ [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS],
it will generate event name automatically and one example is:
echo 'p:usb_gadget/ config_usb_cfg_link $arg1' >> kprobe_events.
Acked-by: Masami Hiramatsu (Google) <[email protected]>
Signed-off-by: Linyu Yuan <[email protected]>
---
v2: fix review comments in V1:
change TP_ENAME_EMPTTY to TP_ENAME_EMPTY,
add some space,
document the macros return by traceprobe_parse_event_name(),
updatea commit description.
v3: remove TP_ENAME_* macro which suggest by Masami Hiramatsu,
allow generate event name from SYSTEM/EVENT for eprobe.
v4: no change
v5: when eprobe input "SYSTEM." is invalid,
add Acked-by tag
Documentation/trace/kprobetrace.rst | 8 ++++----
Documentation/trace/uprobetracer.rst | 8 ++++----
kernel/trace/trace.c | 8 ++++----
kernel/trace/trace_dynevent.c | 2 +-
kernel/trace/trace_eprobe.c | 23 +++++++++++++----------
kernel/trace/trace_kprobe.c | 16 ++++++++++------
kernel/trace/trace_probe.c | 6 ++++++
kernel/trace/trace_uprobe.c | 12 ++++++++----
8 files changed, 50 insertions(+), 33 deletions(-)
diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
index b175d88..4274cc6 100644
--- a/Documentation/trace/kprobetrace.rst
+++ b/Documentation/trace/kprobetrace.rst
@@ -28,10 +28,10 @@ Synopsis of kprobe_events
-------------------------
::
- p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe
- r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe
- p:[GRP/]EVENT] [MOD:]SYM[+0]%return [FETCHARGS] : Set a return probe
- -:[GRP/]EVENT : Clear a probe
+ p[:[GRP/][EVENT]] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe
+ r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe
+ p[:[GRP/][EVENT]] [MOD:]SYM[+0]%return [FETCHARGS] : Set a return probe
+ -:[GRP/][EVENT] : Clear a probe
GRP : Group name. If omitted, use "kprobes" for it.
EVENT : Event name. If omitted, the event name is generated
diff --git a/Documentation/trace/uprobetracer.rst b/Documentation/trace/uprobetracer.rst
index a8e5938..3a1797d7 100644
--- a/Documentation/trace/uprobetracer.rst
+++ b/Documentation/trace/uprobetracer.rst
@@ -26,10 +26,10 @@ Synopsis of uprobe_tracer
-------------------------
::
- p[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a uprobe
- r[:[GRP/]EVENT] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
- p[:[GRP/]EVENT] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
- -:[GRP/]EVENT : Clear uprobe or uretprobe event
+ p[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a uprobe
+ r[:[GRP/][EVENT]] PATH:OFFSET [FETCHARGS] : Set a return uprobe (uretprobe)
+ p[:[GRP/][EVENT]] PATH:OFFSET%return [FETCHARGS] : Set a return uprobe (uretprobe)
+ -:[GRP/][EVENT] : Clear uprobe or uretprobe event
GRP : Group name. If omitted, "uprobes" is the default value.
EVENT : Event name. If omitted, the event name is generated based
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f400800b..76274c6 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5575,13 +5575,13 @@ static const char readme_msg[] =
#endif
#if defined(CONFIG_KPROBE_EVENTS) || defined(CONFIG_UPROBE_EVENTS)
"\t accepts: event-definitions (one definition per line)\n"
- "\t Format: p[:[<group>/]<event>] <place> [<args>]\n"
- "\t r[maxactive][:[<group>/]<event>] <place> [<args>]\n"
+ "\t Format: p[:[<group>/][<event>]] <place> [<args>]\n"
+ "\t r[maxactive][:[<group>/][<event>]] <place> [<args>]\n"
#ifdef CONFIG_HIST_TRIGGERS
"\t s:[synthetic/]<event> <field> [<field>]\n"
#endif
- "\t e[:[<group>/]<event>] <attached-group>.<attached-event> [<args>]\n"
- "\t -:[<group>/]<event>\n"
+ "\t e[:[<group>/][<event>]] <attached-group>.<attached-event> [<args>]\n"
+ "\t -:[<group>/][<event>]\n"
#ifdef CONFIG_KPROBE_EVENTS
"\t place: [<module>:]<symbol>[+<offset>]|<memaddr>\n"
"place (kretprobe): [<module>:]<symbol>[+<offset>]%return|<memaddr>\n"
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
index 076b447..1549966 100644
--- a/kernel/trace/trace_dynevent.c
+++ b/kernel/trace/trace_dynevent.c
@@ -101,7 +101,7 @@ int dyn_event_release(const char *raw_command, struct dyn_event_operations *type
event = p + 1;
*p = '\0';
}
- if (event[0] == '\0') {
+ if (!system && event[0] == '\0') {
ret = -EINVAL;
goto out;
}
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 17d64e3..200f226 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -125,6 +125,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
* We match the following:
* event only - match all eprobes with event name
* system and event only - match all system/event probes
+ * system only - match all system probes
*
* The below has the above satisfied with more arguments:
*
@@ -143,7 +144,7 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
return false;
/* Must match the event name */
- if (strcmp(trace_probe_name(&ep->tp), event) != 0)
+ if (event[0] != '\0' && strcmp(trace_probe_name(&ep->tp), event) != 0)
return false;
/* No arguments match all */
@@ -848,7 +849,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
{
/*
* Argument syntax:
- * e[:[GRP/]ENAME] SYSTEM.EVENT [FETCHARGS]
+ * e[:[GRP/][ENAME]] SYSTEM.EVENT [FETCHARGS]
* Fetch args:
* <name>=$<field>[:TYPE]
*/
@@ -858,6 +859,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
struct trace_eprobe *ep = NULL;
char buf1[MAX_EVENT_NAME_LEN];
char buf2[MAX_EVENT_NAME_LEN];
+ char gbuf[MAX_EVENT_NAME_LEN];
int ret = 0;
int i;
@@ -869,22 +871,23 @@ static int __trace_eprobe_create(int argc, const char *argv[])
event = strchr(&argv[0][1], ':');
if (event) {
event++;
- ret = traceprobe_parse_event_name(&event, &group, buf1,
+ ret = traceprobe_parse_event_name(&event, &group, gbuf,
event - argv[0]);
if (ret)
goto parse_error;
- } else {
- strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN);
- sanitize_event_name(buf1);
- event = buf1;
}
sys_event = argv[1];
- ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
- sys_event - argv[1]);
- if (ret || !sys_name)
+ ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
+ if (!sys_event || !sys_name)
goto parse_error;
+ if (!event) {
+ snprintf(buf1, MAX_EVENT_NAME_LEN, "%s_%s", sys_name, sys_event);
+ sanitize_event_name(buf1);
+ event = buf1;
+ }
+
mutex_lock(&event_mutex);
event_call = find_and_get_event(sys_name, sys_event);
ep = alloc_event_probe(group, event, event_call, argc - 2);
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 9350733..2c36a71 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -163,7 +163,8 @@ static bool trace_kprobe_match(const char *system, const char *event,
{
struct trace_kprobe *tk = to_trace_kprobe(ev);
- return strcmp(trace_probe_name(&tk->tp), event) == 0 &&
+ return (event[0] == '\0' ||
+ strcmp(trace_probe_name(&tk->tp), event) == 0) &&
(!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) &&
trace_kprobe_match_command_head(tk, argc, argv);
}
@@ -708,11 +709,11 @@ static int __trace_kprobe_create(int argc, const char *argv[])
/*
* Argument syntax:
* - Add kprobe:
- * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
+ * p[:[GRP/][EVENT]] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS]
* - Add kretprobe:
- * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS]
+ * r[MAXACTIVE][:[GRP/][EVENT]] [MOD:]KSYM[+0] [FETCHARGS]
* Or
- * p:[GRP/]EVENT] [MOD:]KSYM[+0]%return [FETCHARGS]
+ * p[:[GRP/][EVENT]] [MOD:]KSYM[+0]%return [FETCHARGS]
*
* Fetch args:
* $retval : fetch return value
@@ -739,6 +740,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
long offset = 0;
void *addr = NULL;
char buf[MAX_EVENT_NAME_LEN];
+ char gbuf[MAX_EVENT_NAME_LEN];
unsigned int flags = TPARG_FL_KERNEL;
switch (argv[0][0]) {
@@ -833,11 +835,13 @@ static int __trace_kprobe_create(int argc, const char *argv[])
trace_probe_log_set_index(0);
if (event) {
- ret = traceprobe_parse_event_name(&event, &group, buf,
+ ret = traceprobe_parse_event_name(&event, &group, gbuf,
event - argv[0]);
if (ret)
goto parse_error;
- } else {
+ }
+
+ if (!event) {
/* Make a new event name */
if (symbol)
snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 80863c6..b788183 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -257,6 +257,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
}
len = strlen(event);
if (len == 0) {
+ if (slash) {
+ *pevent = NULL;
+ return 0;
+ }
+
trace_probe_log_err(offset, NO_EVENT_NAME);
return -EINVAL;
} else if (len > MAX_EVENT_NAME_LEN) {
@@ -267,6 +272,7 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
trace_probe_log_err(offset, BAD_EVENT_NAME);
return -EINVAL;
}
+
return 0;
}
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 9711589..5af0e85 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -312,7 +312,8 @@ static bool trace_uprobe_match(const char *system, const char *event,
{
struct trace_uprobe *tu = to_trace_uprobe(ev);
- return strcmp(trace_probe_name(&tu->tp), event) == 0 &&
+ return (event[0] == '\0' ||
+ strcmp(trace_probe_name(&tu->tp), event) == 0) &&
(!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) &&
trace_uprobe_match_command_head(tu, argc, argv);
}
@@ -532,7 +533,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
/*
* Argument syntax:
- * - Add uprobe: p|r[:[GRP/]EVENT] PATH:OFFSET[%return][(REF)] [FETCHARGS]
+ * - Add uprobe: p|r[:[GRP/][EVENT]] PATH:OFFSET[%return][(REF)] [FETCHARGS]
*/
static int __trace_uprobe_create(int argc, const char **argv)
{
@@ -540,6 +541,7 @@ static int __trace_uprobe_create(int argc, const char **argv)
const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
char *arg, *filename, *rctr, *rctr_end, *tmp;
char buf[MAX_EVENT_NAME_LEN];
+ char gbuf[MAX_EVENT_NAME_LEN];
enum probe_print_type ptype;
struct path path;
unsigned long offset, ref_ctr_offset;
@@ -645,11 +647,13 @@ static int __trace_uprobe_create(int argc, const char **argv)
/* setup a probe */
trace_probe_log_set_index(0);
if (event) {
- ret = traceprobe_parse_event_name(&event, &group, buf,
+ ret = traceprobe_parse_event_name(&event, &group, gbuf,
event - argv[0]);
if (ret)
goto fail_address_parse;
- } else {
+ }
+
+ if (!event) {
char *tail;
char *ptr;
--
2.7.4
add kprobe and eprobe event test for new GRP/ only format.
Acked-by: Masami Hiramatsu (Google) <[email protected]>
Signed-off-by: Linyu Yuan <[email protected]>
---
v3: first add in this version
v4: remove restriction of test case
v5: add Acked-by tag
.../selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc | 9 ++++++++-
.../selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc | 7 +++++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc
index 60c02b4..c300eb0 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc
@@ -1,7 +1,7 @@
#!/bin/sh
# SPDX-License-Identifier: GPL-2.0
# description: Generic dynamic event - add/remove eprobe events
-# requires: dynamic_events events/syscalls/sys_enter_openat "e[:[<group>/]<event>] <attached-group>.<attached-event> [<args>]":README
+# requires: dynamic_events events/syscalls/sys_enter_openat "<attached-group>.<attached-event> [<args>]":README
echo 0 > events/enable
@@ -87,4 +87,11 @@ echo "-:eprobes/$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
! grep -q "$EPROBE" dynamic_events
! test -d events/eprobes/$EPROBE
+if grep -q "e\[:\[<group>/]\[<event>]]" README; then
+ echo "e:mygroup/ $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+ test -d events/mygroup
+ echo "-:mygroup/" >> dynamic_events
+ ! test -d events/mygroup
+fi
+
clear_trace
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
index b4da41d..13d43f4 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
@@ -23,4 +23,11 @@ grep -q myevent1 dynamic_events
echo > dynamic_events
+if grep -q "p\[:\[<group>/]\[<event>]]" README; then
+ echo "p:mygroup/ $PLACE" >> dynamic_events
+ test -d events/mygroup
+ echo "-:mygroup/" >> dynamic_events
+ ! test -d events/mygroup
+fi
+
clear_trace
--
2.7.4
traceprobe_parse_event_name() already validate group and event name,
there is no need to call is_good_name() after it.
Acked-by: Masami Hiramatsu (Google) <[email protected]>
Signed-off-by: Linyu Yuan <[email protected]>
---
v2: drop v1 change as it is NACK.
add it to remove duplicate is_good_name().
v3: move it as first patch.
v4: no change
v5: add Acked-by tag
kernel/trace/trace_eprobe.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 7d44785..17d64e3 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc, const char *argv[])
sanitize_event_name(buf1);
event = buf1;
}
- if (!is_good_name(event) || !is_good_name(group))
- goto parse_error;
sys_event = argv[1];
ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
sys_event - argv[1]);
if (ret || !sys_name)
goto parse_error;
- if (!is_good_name(sys_event) || !is_good_name(sys_name))
- goto parse_error;
mutex_lock(&event_mutex);
event_call = find_and_get_event(sys_name, sys_event);
--
2.7.4
On Tue, 14 Jun 2022 09:04:56 +0800
Linyu Yuan <[email protected]> wrote:
> traceprobe_parse_event_name() already validate group and event name,
> there is no need to call is_good_name() after it.
>
> Acked-by: Masami Hiramatsu (Google) <[email protected]>
> Signed-off-by: Linyu Yuan <[email protected]>
> ---
> v2: drop v1 change as it is NACK.
> add it to remove duplicate is_good_name().
> v3: move it as first patch.
> v4: no change
> v5: add Acked-by tag
>
> kernel/trace/trace_eprobe.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 7d44785..17d64e3 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc, const char *argv[])
> sanitize_event_name(buf1);
> event = buf1;
> }
> - if (!is_good_name(event) || !is_good_name(group))
> - goto parse_error;
>
Tom replied that the above may be an issue. You ignored his response.
-- Steve
> sys_event = argv[1];
> ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
> sys_event - argv[1]);
> if (ret || !sys_name)
> goto parse_error;
> - if (!is_good_name(sys_event) || !is_good_name(sys_name))
> - goto parse_error;
>
> mutex_lock(&event_mutex);
> event_call = find_and_get_event(sys_name, sys_event);
hi tom,
On 6/18/2022 9:49 AM, Steven Rostedt wrote:
> On Tue, 14 Jun 2022 09:04:56 +0800
> Linyu Yuan <[email protected]> wrote:
>
>> traceprobe_parse_event_name() already validate group and event name,
>> there is no need to call is_good_name() after it.
>>
>> Acked-by: Masami Hiramatsu (Google) <[email protected]>
>> Signed-off-by: Linyu Yuan <[email protected]>
>> ---
>> v2: drop v1 change as it is NACK.
>> add it to remove duplicate is_good_name().
>> v3: move it as first patch.
>> v4: no change
>> v5: add Acked-by tag
>>
>> kernel/trace/trace_eprobe.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
>> index 7d44785..17d64e3 100644
>> --- a/kernel/trace/trace_eprobe.c
>> +++ b/kernel/trace/trace_eprobe.c
>> @@ -878,16 +878,12 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>> sanitize_event_name(buf1);
>> event = buf1;
>> }
>> - if (!is_good_name(event) || !is_good_name(group))
>> - goto parse_error;
>>
> Tom replied that the above may be an issue. You ignored his response.
>
> -- Steve
>
i reply his mail in V4
https://lore.kernel.org/lkml/[email protected]/,
form my view, i think it is safe, in !event case Tom mentioned, we will
generate event from SYSTEM.EVENT
which is verified by traceprobe_parse_event_name().
Tom, could you review it again ?
>> sys_event = argv[1];
>> ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2,
>> sys_event - argv[1]);
>> if (ret || !sys_name)
>> goto parse_error;
>> - if (!is_good_name(sys_event) || !is_good_name(sys_name))
>> - goto parse_error;
>>
>> mutex_lock(&event_mutex);
>> event_call = find_and_get_event(sys_name, sys_event);