* [PATCH 0/2] tracing/probes: allow no event name input when create group
@ 2022-05-01 9:34 Linyu Yuan
2022-05-01 9:34 ` [PATCH 1/2] tracing/probes: auto generate events name when create a group of events Linyu Yuan
2022-05-01 9:34 ` [PATCH 2/2] tracing/probes: make match function safe Linyu Yuan
0 siblings, 2 replies; 9+ messages in thread
From: Linyu Yuan @ 2022-05-01 9:34 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar; +Cc: linux-kernel, Linyu Yuan
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.
Linyu Yuan (2):
tracing/probes: auto generate events name when create a group of
events
tracing/probes: make match function safe
Documentation/trace/kprobetrace.rst | 8 +++---
Documentation/trace/uprobetracer.rst | 8 +++---
kernel/trace/trace_dynevent.c | 2 +-
kernel/trace/trace_eprobe.c | 50 ++++++++++++++++++++++++------------
kernel/trace/trace_kprobe.c | 21 ++++++++++-----
kernel/trace/trace_probe.c | 9 ++++++-
kernel/trace/trace_probe.h | 4 +++
kernel/trace/trace_uprobe.c | 17 ++++++++----
8 files changed, 81 insertions(+), 38 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] tracing/probes: auto generate events name when create a group of events 2022-05-01 9:34 [PATCH 0/2] tracing/probes: allow no event name input when create group Linyu Yuan @ 2022-05-01 9:34 ` Linyu Yuan 2022-05-24 22:31 ` Steven Rostedt 2022-05-01 9:34 ` [PATCH 2/2] tracing/probes: make match function safe Linyu Yuan 1 sibling, 1 reply; 9+ messages in thread From: Linyu Yuan @ 2022-05-01 9:34 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar; +Cc: linux-kernel, Linyu Yuan change traceprobe_parse_event_name() and return GRP/EVENT types, TP_ENAME_GROUP_EVENT means user input GRP/EVENT format, TP_ENAME_EVENT means user only input EVENT format, TP_ENAME_GROUP means user only input GRP/ format, it allow no event name input when create a group of events, and will auto generate event name according second argument. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- Documentation/trace/kprobetrace.rst | 8 ++++---- Documentation/trace/uprobetracer.rst | 8 ++++---- kernel/trace/trace_dynevent.c | 2 +- kernel/trace/trace_eprobe.c | 19 +++++++++++-------- kernel/trace/trace_kprobe.c | 18 +++++++++++------- kernel/trace/trace_probe.c | 9 ++++++++- kernel/trace/trace_probe.h | 4 ++++ kernel/trace/trace_uprobe.c | 14 +++++++++----- 8 files changed, 52 insertions(+), 30 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_dynevent.c b/kernel/trace/trace_dynevent.c index e34e8182..033248d 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 7d44785..b16e067 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 grp_buf[MAX_EVENT_NAME_LEN]; int ret = 0; int i; @@ -866,14 +868,16 @@ static int __trace_eprobe_create(int argc, const char *argv[]) trace_probe_log_init("event_probe", argc, argv); + ret = TP_ENAME_EMPTTY; event = strchr(&argv[0][1], ':'); if (event) { event++; - ret = traceprobe_parse_event_name(&event, &group, buf1, + ret = traceprobe_parse_event_name(&event, &group, grp_buf, event - argv[0]); - if (ret) + if (ret < 0) goto parse_error; - } else { + } + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN); sanitize_event_name(buf1); event = buf1; @@ -882,9 +886,8 @@ static int __trace_eprobe_create(int argc, const char *argv[]) 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) + ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0); + if (ret != TP_ENAME_GROUP_EVENT) goto parse_error; if (!is_good_name(sys_event) || !is_good_name(sys_name)) goto parse_error; diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 47cebef..2cd8ef9 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 grp_buf[MAX_EVENT_NAME_LEN]; unsigned int flags = TPARG_FL_KERNEL; switch (argv[0][0]) { @@ -832,12 +834,14 @@ static int __trace_kprobe_create(int argc, const char *argv[]) } trace_probe_log_set_index(0); + ret = TP_ENAME_EMPTTY; if (event) { - ret = traceprobe_parse_event_name(&event, &group, buf, + ret = traceprobe_parse_event_name(&event, &group, grp_buf, event - argv[0]); - if (ret) + if (ret < 0) goto parse_error; - } else { + } + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { /* 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..7fd50ab 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, } len = strlen(event); if (len == 0) { + if (slash) + return TP_ENAME_GROUP; + trace_probe_log_err(offset, NO_EVENT_NAME); return -EINVAL; } else if (len > MAX_EVENT_NAME_LEN) { @@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, trace_probe_log_err(offset, BAD_EVENT_NAME); return -EINVAL; } - return 0; + + if (slash) + return TP_ENAME_GROUP_EVENT; + + return TP_ENAME_EVENT; } #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long)) diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h index 92cc149..d726769 100644 --- a/kernel/trace/trace_probe.h +++ b/kernel/trace/trace_probe.h @@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg); extern int traceprobe_split_symbol_offset(char *symbol, long *offset); int traceprobe_parse_event_name(const char **pevent, const char **pgroup, char *buf, int offset); +#define TP_ENAME_GROUP_EVENT 0 +#define TP_ENAME_EVENT 1 +#define TP_ENAME_GROUP 2 +#define TP_ENAME_EMPTTY 3 enum probe_print_type { PROBE_PRINT_NORMAL, diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index 9711589..a935c08 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 grp_buf[MAX_EVENT_NAME_LEN]; enum probe_print_type ptype; struct path path; unsigned long offset, ref_ctr_offset; @@ -644,12 +646,14 @@ static int __trace_uprobe_create(int argc, const char **argv) /* setup a probe */ trace_probe_log_set_index(0); + ret = TP_ENAME_EMPTTY; if (event) { - ret = traceprobe_parse_event_name(&event, &group, buf, + ret = traceprobe_parse_event_name(&event, &group, grp_buf, event - argv[0]); - if (ret) + if (ret < 0) goto fail_address_parse; - } else { + } + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { char *tail; char *ptr; -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing/probes: auto generate events name when create a group of events 2022-05-01 9:34 ` [PATCH 1/2] tracing/probes: auto generate events name when create a group of events Linyu Yuan @ 2022-05-24 22:31 ` Steven Rostedt 2022-05-25 1:33 ` Linyu Yuan 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2022-05-24 22:31 UTC (permalink / raw) To: Linyu Yuan; +Cc: Ingo Molnar, linux-kernel, Masami Hiramatsu, Tom Zanussi [ Adding Masami (kprobe maintainer) and Tom (dynamic event maintainer) ] On Sun, 1 May 2022 17:34:10 +0800 Linyu Yuan <quic_linyyuan@quicinc.com> wrote: > change traceprobe_parse_event_name() and return GRP/EVENT types, > TP_ENAME_GROUP_EVENT means user input GRP/EVENT format, > TP_ENAME_EVENT means user only input EVENT format, > TP_ENAME_GROUP means user only input GRP/ format, > > it allow no event name input when create a group of events, > and will auto generate event name according second argument. Sorry, but I can't understand the above. Can you reword it, and perhaps include an example of what this patch does. After reading the code, I guess you are trying to make it so that you can leave out the event name and only have the group name, if you add the slash. echo 'p:group/ schedule' > kprobe_events And that will create a kprobe "schedule" event under the "group" system? Also, for some reason, patch 2 of the series never came to me. But it is in my LKML folder :-/ > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > Documentation/trace/kprobetrace.rst | 8 ++++---- > Documentation/trace/uprobetracer.rst | 8 ++++---- > kernel/trace/trace_dynevent.c | 2 +- > kernel/trace/trace_eprobe.c | 19 +++++++++++-------- > kernel/trace/trace_kprobe.c | 18 +++++++++++------- > kernel/trace/trace_probe.c | 9 ++++++++- > kernel/trace/trace_probe.h | 4 ++++ > kernel/trace/trace_uprobe.c | 14 +++++++++----- > 8 files changed, 52 insertions(+), 30 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_dynevent.c b/kernel/trace/trace_dynevent.c > index e34e8182..033248d 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 7d44785..b16e067 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 grp_buf[MAX_EVENT_NAME_LEN]; > int ret = 0; > int i; > > @@ -866,14 +868,16 @@ static int __trace_eprobe_create(int argc, const char *argv[]) > > trace_probe_log_init("event_probe", argc, argv); > > + ret = TP_ENAME_EMPTTY; EMPTY is spelled wrong. > event = strchr(&argv[0][1], ':'); > if (event) { > event++; > - ret = traceprobe_parse_event_name(&event, &group, buf1, > + ret = traceprobe_parse_event_name(&event, &group, grp_buf, > event - argv[0]); > - if (ret) > + if (ret < 0) > goto parse_error; > - } else { > + } add space. > + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { > strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN); > sanitize_event_name(buf1); > event = buf1; I'd like to see Masami's and Tom's comments on this one. -- Steve > @@ -882,9 +886,8 @@ static int __trace_eprobe_create(int argc, const char *argv[]) > 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) > + ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0); > + if (ret != TP_ENAME_GROUP_EVENT) > goto parse_error; > if (!is_good_name(sys_event) || !is_good_name(sys_name)) > goto parse_error; > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 47cebef..2cd8ef9 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 grp_buf[MAX_EVENT_NAME_LEN]; > unsigned int flags = TPARG_FL_KERNEL; > > switch (argv[0][0]) { > @@ -832,12 +834,14 @@ static int __trace_kprobe_create(int argc, const char *argv[]) > } > > trace_probe_log_set_index(0); > + ret = TP_ENAME_EMPTTY; > if (event) { > - ret = traceprobe_parse_event_name(&event, &group, buf, > + ret = traceprobe_parse_event_name(&event, &group, grp_buf, > event - argv[0]); > - if (ret) > + if (ret < 0) > goto parse_error; > - } else { > + } > + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { > /* 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..7fd50ab 100644 > --- a/kernel/trace/trace_probe.c > +++ b/kernel/trace/trace_probe.c > @@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, > } > len = strlen(event); > if (len == 0) { > + if (slash) > + return TP_ENAME_GROUP; > + > trace_probe_log_err(offset, NO_EVENT_NAME); > return -EINVAL; > } else if (len > MAX_EVENT_NAME_LEN) { > @@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, > trace_probe_log_err(offset, BAD_EVENT_NAME); > return -EINVAL; > } > - return 0; > + > + if (slash) > + return TP_ENAME_GROUP_EVENT; > + > + return TP_ENAME_EVENT; > } > > #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long)) > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > index 92cc149..d726769 100644 > --- a/kernel/trace/trace_probe.h > +++ b/kernel/trace/trace_probe.h > @@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg); > extern int traceprobe_split_symbol_offset(char *symbol, long *offset); > int traceprobe_parse_event_name(const char **pevent, const char **pgroup, > char *buf, int offset); > +#define TP_ENAME_GROUP_EVENT 0 > +#define TP_ENAME_EVENT 1 > +#define TP_ENAME_GROUP 2 > +#define TP_ENAME_EMPTTY 3 > > enum probe_print_type { > PROBE_PRINT_NORMAL, > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index 9711589..a935c08 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 grp_buf[MAX_EVENT_NAME_LEN]; > enum probe_print_type ptype; > struct path path; > unsigned long offset, ref_ctr_offset; > @@ -644,12 +646,14 @@ static int __trace_uprobe_create(int argc, const char **argv) > > /* setup a probe */ > trace_probe_log_set_index(0); > + ret = TP_ENAME_EMPTTY; > if (event) { > - ret = traceprobe_parse_event_name(&event, &group, buf, > + ret = traceprobe_parse_event_name(&event, &group, grp_buf, > event - argv[0]); > - if (ret) > + if (ret < 0) > goto fail_address_parse; > - } else { > + } > + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { > char *tail; > char *ptr; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing/probes: auto generate events name when create a group of events 2022-05-24 22:31 ` Steven Rostedt @ 2022-05-25 1:33 ` Linyu Yuan 2022-05-26 23:36 ` Masami Hiramatsu 0 siblings, 1 reply; 9+ messages in thread From: Linyu Yuan @ 2022-05-25 1:33 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, linux-kernel, Masami Hiramatsu, Tom Zanussi On 5/25/2022 6:31 AM, Steven Rostedt wrote: > [ Adding Masami (kprobe maintainer) and Tom (dynamic event maintainer) ] > > On Sun, 1 May 2022 17:34:10 +0800 > Linyu Yuan <quic_linyyuan@quicinc.com> wrote: > >> change traceprobe_parse_event_name() and return GRP/EVENT types, >> TP_ENAME_GROUP_EVENT means user input GRP/EVENT format, >> TP_ENAME_EVENT means user only input EVENT format, >> TP_ENAME_GROUP means user only input GRP/ format, >> >> it allow no event name input when create a group of events, >> and will auto generate event name according second argument. > Sorry, but I can't understand the above. Can you reword it, and perhaps > include an example of what this patch does. thanks, will try to update after Masai and Tom comment > > After reading the code, I guess you are trying to make it so that you can > leave out the event name and only have the group name, if you add the slash. > > echo 'p:group/ schedule' > kprobe_events > > And that will create a kprobe "schedule" event under the "group" system? that's right > > Also, for some reason, patch 2 of the series never came to me. But it is in > my LKML folder :-/ > >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >> --- >> Documentation/trace/kprobetrace.rst | 8 ++++---- >> Documentation/trace/uprobetracer.rst | 8 ++++---- >> kernel/trace/trace_dynevent.c | 2 +- >> kernel/trace/trace_eprobe.c | 19 +++++++++++-------- >> kernel/trace/trace_kprobe.c | 18 +++++++++++------- >> kernel/trace/trace_probe.c | 9 ++++++++- >> kernel/trace/trace_probe.h | 4 ++++ >> kernel/trace/trace_uprobe.c | 14 +++++++++----- >> 8 files changed, 52 insertions(+), 30 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_dynevent.c b/kernel/trace/trace_dynevent.c >> index e34e8182..033248d 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 7d44785..b16e067 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 grp_buf[MAX_EVENT_NAME_LEN]; >> int ret = 0; >> int i; >> >> @@ -866,14 +868,16 @@ static int __trace_eprobe_create(int argc, const char *argv[]) >> >> trace_probe_log_init("event_probe", argc, argv); >> >> + ret = TP_ENAME_EMPTTY; > EMPTY is spelled wrong. sorry, will wait comment from maintainer of V1, will fix it in V2 if V1 is not reject. >> event = strchr(&argv[0][1], ':'); >> if (event) { >> event++; >> - ret = traceprobe_parse_event_name(&event, &group, buf1, >> + ret = traceprobe_parse_event_name(&event, &group, grp_buf, >> event - argv[0]); >> - if (ret) >> + if (ret < 0) >> goto parse_error; >> - } else { >> + } > add space. sure, thanks. > >> + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { >> strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN); >> sanitize_event_name(buf1); >> event = buf1; > I'd like to see Masami's and Tom's comments on this one. > > -- Steve > >> @@ -882,9 +886,8 @@ static int __trace_eprobe_create(int argc, const char *argv[]) >> 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) >> + ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0); >> + if (ret != TP_ENAME_GROUP_EVENT) >> goto parse_error; >> if (!is_good_name(sys_event) || !is_good_name(sys_name)) >> goto parse_error; >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c >> index 47cebef..2cd8ef9 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 grp_buf[MAX_EVENT_NAME_LEN]; >> unsigned int flags = TPARG_FL_KERNEL; >> >> switch (argv[0][0]) { >> @@ -832,12 +834,14 @@ static int __trace_kprobe_create(int argc, const char *argv[]) >> } >> >> trace_probe_log_set_index(0); >> + ret = TP_ENAME_EMPTTY; >> if (event) { >> - ret = traceprobe_parse_event_name(&event, &group, buf, >> + ret = traceprobe_parse_event_name(&event, &group, grp_buf, >> event - argv[0]); >> - if (ret) >> + if (ret < 0) >> goto parse_error; >> - } else { >> + } >> + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { >> /* 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..7fd50ab 100644 >> --- a/kernel/trace/trace_probe.c >> +++ b/kernel/trace/trace_probe.c >> @@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, >> } >> len = strlen(event); >> if (len == 0) { >> + if (slash) >> + return TP_ENAME_GROUP; >> + >> trace_probe_log_err(offset, NO_EVENT_NAME); >> return -EINVAL; >> } else if (len > MAX_EVENT_NAME_LEN) { >> @@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, >> trace_probe_log_err(offset, BAD_EVENT_NAME); >> return -EINVAL; >> } >> - return 0; >> + >> + if (slash) >> + return TP_ENAME_GROUP_EVENT; >> + >> + return TP_ENAME_EVENT; >> } >> >> #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long)) >> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h >> index 92cc149..d726769 100644 >> --- a/kernel/trace/trace_probe.h >> +++ b/kernel/trace/trace_probe.h >> @@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg); >> extern int traceprobe_split_symbol_offset(char *symbol, long *offset); >> int traceprobe_parse_event_name(const char **pevent, const char **pgroup, >> char *buf, int offset); >> +#define TP_ENAME_GROUP_EVENT 0 >> +#define TP_ENAME_EVENT 1 >> +#define TP_ENAME_GROUP 2 >> +#define TP_ENAME_EMPTTY 3 >> >> enum probe_print_type { >> PROBE_PRINT_NORMAL, >> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c >> index 9711589..a935c08 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 grp_buf[MAX_EVENT_NAME_LEN]; >> enum probe_print_type ptype; >> struct path path; >> unsigned long offset, ref_ctr_offset; >> @@ -644,12 +646,14 @@ static int __trace_uprobe_create(int argc, const char **argv) >> >> /* setup a probe */ >> trace_probe_log_set_index(0); >> + ret = TP_ENAME_EMPTTY; >> if (event) { >> - ret = traceprobe_parse_event_name(&event, &group, buf, >> + ret = traceprobe_parse_event_name(&event, &group, grp_buf, >> event - argv[0]); >> - if (ret) >> + if (ret < 0) >> goto fail_address_parse; >> - } else { >> + } >> + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { >> char *tail; >> char *ptr; >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] tracing/probes: auto generate events name when create a group of events 2022-05-25 1:33 ` Linyu Yuan @ 2022-05-26 23:36 ` Masami Hiramatsu 0 siblings, 0 replies; 9+ messages in thread From: Masami Hiramatsu @ 2022-05-26 23:36 UTC (permalink / raw) To: Linyu Yuan Cc: Steven Rostedt, Ingo Molnar, linux-kernel, Masami Hiramatsu, Tom Zanussi Hi Linyu, On Wed, 25 May 2022 09:33:57 +0800 Linyu Yuan <quic_linyyuan@quicinc.com> wrote: > > On 5/25/2022 6:31 AM, Steven Rostedt wrote: > > [ Adding Masami (kprobe maintainer) and Tom (dynamic event maintainer) ] > > > > On Sun, 1 May 2022 17:34:10 +0800 > > Linyu Yuan <quic_linyyuan@quicinc.com> wrote: > > > >> change traceprobe_parse_event_name() and return GRP/EVENT types, > >> TP_ENAME_GROUP_EVENT means user input GRP/EVENT format, > >> TP_ENAME_EVENT means user only input EVENT format, > >> TP_ENAME_GROUP means user only input GRP/ format, > >> > >> it allow no event name input when create a group of events, > >> and will auto generate event name according second argument. > > Sorry, but I can't understand the above. Can you reword it, and perhaps > > include an example of what this patch does. > > thanks, will try to update after Masai and Tom comment > > > > > After reading the code, I guess you are trying to make it so that you can > > leave out the event name and only have the group name, if you add the slash. > > > > echo 'p:group/ schedule' > kprobe_events > > > > And that will create a kprobe "schedule" event under the "group" system? > > that's right Hmm, it is an interesting idea :) Please update the patch description, and avoid explaining internal implementation details. Those flag explanation should be commented in the code. (And Cc to me.) Thank you, > > > > > Also, for some reason, patch 2 of the series never came to me. But it is in > > my LKML folder :-/ > > > >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > >> --- > >> Documentation/trace/kprobetrace.rst | 8 ++++---- > >> Documentation/trace/uprobetracer.rst | 8 ++++---- > >> kernel/trace/trace_dynevent.c | 2 +- > >> kernel/trace/trace_eprobe.c | 19 +++++++++++-------- > >> kernel/trace/trace_kprobe.c | 18 +++++++++++------- > >> kernel/trace/trace_probe.c | 9 ++++++++- > >> kernel/trace/trace_probe.h | 4 ++++ > >> kernel/trace/trace_uprobe.c | 14 +++++++++----- > >> 8 files changed, 52 insertions(+), 30 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_dynevent.c b/kernel/trace/trace_dynevent.c > >> index e34e8182..033248d 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 7d44785..b16e067 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 grp_buf[MAX_EVENT_NAME_LEN]; > >> int ret = 0; > >> int i; > >> > >> @@ -866,14 +868,16 @@ static int __trace_eprobe_create(int argc, const char *argv[]) > >> > >> trace_probe_log_init("event_probe", argc, argv); > >> > >> + ret = TP_ENAME_EMPTTY; > > EMPTY is spelled wrong. > > > sorry, will wait comment from maintainer of V1, will fix it in V2 if > V1 is not reject. > > > >> event = strchr(&argv[0][1], ':'); > >> if (event) { > >> event++; > >> - ret = traceprobe_parse_event_name(&event, &group, buf1, > >> + ret = traceprobe_parse_event_name(&event, &group, grp_buf, > >> event - argv[0]); > >> - if (ret) > >> + if (ret < 0) > >> goto parse_error; > >> - } else { > >> + } > > add space. > > sure, thanks. > > > > >> + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { > >> strscpy(buf1, argv[1], MAX_EVENT_NAME_LEN); > >> sanitize_event_name(buf1); > >> event = buf1; > > I'd like to see Masami's and Tom's comments on this one. > > > > -- Steve > > > >> @@ -882,9 +886,8 @@ static int __trace_eprobe_create(int argc, const char *argv[]) > >> 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) > >> + ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0); > >> + if (ret != TP_ENAME_GROUP_EVENT) > >> goto parse_error; > >> if (!is_good_name(sys_event) || !is_good_name(sys_name)) > >> goto parse_error; > >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > >> index 47cebef..2cd8ef9 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 grp_buf[MAX_EVENT_NAME_LEN]; > >> unsigned int flags = TPARG_FL_KERNEL; > >> > >> switch (argv[0][0]) { > >> @@ -832,12 +834,14 @@ static int __trace_kprobe_create(int argc, const char *argv[]) > >> } > >> > >> trace_probe_log_set_index(0); > >> + ret = TP_ENAME_EMPTTY; > >> if (event) { > >> - ret = traceprobe_parse_event_name(&event, &group, buf, > >> + ret = traceprobe_parse_event_name(&event, &group, grp_buf, > >> event - argv[0]); > >> - if (ret) > >> + if (ret < 0) > >> goto parse_error; > >> - } else { > >> + } > >> + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { > >> /* 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..7fd50ab 100644 > >> --- a/kernel/trace/trace_probe.c > >> +++ b/kernel/trace/trace_probe.c > >> @@ -257,6 +257,9 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, > >> } > >> len = strlen(event); > >> if (len == 0) { > >> + if (slash) > >> + return TP_ENAME_GROUP; > >> + > >> trace_probe_log_err(offset, NO_EVENT_NAME); > >> return -EINVAL; > >> } else if (len > MAX_EVENT_NAME_LEN) { > >> @@ -267,7 +270,11 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup, > >> trace_probe_log_err(offset, BAD_EVENT_NAME); > >> return -EINVAL; > >> } > >> - return 0; > >> + > >> + if (slash) > >> + return TP_ENAME_GROUP_EVENT; > >> + > >> + return TP_ENAME_EVENT; > >> } > >> > >> #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long)) > >> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > >> index 92cc149..d726769 100644 > >> --- a/kernel/trace/trace_probe.h > >> +++ b/kernel/trace/trace_probe.h > >> @@ -364,6 +364,10 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg); > >> extern int traceprobe_split_symbol_offset(char *symbol, long *offset); > >> int traceprobe_parse_event_name(const char **pevent, const char **pgroup, > >> char *buf, int offset); > >> +#define TP_ENAME_GROUP_EVENT 0 > >> +#define TP_ENAME_EVENT 1 > >> +#define TP_ENAME_GROUP 2 > >> +#define TP_ENAME_EMPTTY 3 > >> > >> enum probe_print_type { > >> PROBE_PRINT_NORMAL, > >> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > >> index 9711589..a935c08 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 grp_buf[MAX_EVENT_NAME_LEN]; > >> enum probe_print_type ptype; > >> struct path path; > >> unsigned long offset, ref_ctr_offset; > >> @@ -644,12 +646,14 @@ static int __trace_uprobe_create(int argc, const char **argv) > >> > >> /* setup a probe */ > >> trace_probe_log_set_index(0); > >> + ret = TP_ENAME_EMPTTY; > >> if (event) { > >> - ret = traceprobe_parse_event_name(&event, &group, buf, > >> + ret = traceprobe_parse_event_name(&event, &group, grp_buf, > >> event - argv[0]); > >> - if (ret) > >> + if (ret < 0) > >> goto fail_address_parse; > >> - } else { > >> + } > >> + if (ret == TP_ENAME_EMPTTY || ret == TP_ENAME_GROUP) { > >> char *tail; > >> char *ptr; > >> -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] tracing/probes: make match function safe 2022-05-01 9:34 [PATCH 0/2] tracing/probes: allow no event name input when create group Linyu Yuan 2022-05-01 9:34 ` [PATCH 1/2] tracing/probes: auto generate events name when create a group of events Linyu Yuan @ 2022-05-01 9:34 ` Linyu Yuan 2022-05-24 22:34 ` Steven Rostedt 1 sibling, 1 reply; 9+ messages in thread From: Linyu Yuan @ 2022-05-01 9:34 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar; +Cc: linux-kernel, Linyu Yuan When delete one kprobe/uprobe/eprobe event entry using /sys/kernel/debug/tracing/dynamic_events file, it will loop all dynamic envent entries, as user will not input dyn_event_operations type, when call the match function of kprobe/uprobe/eprobe, the dynamic event may have different dyn_event_operations type, but currently match function may return a match. Fix by check dyn_event_operations type first. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- kernel/trace/trace_eprobe.c | 31 +++++++++++++++++++++++-------- kernel/trace/trace_kprobe.c | 3 +++ kernel/trace/trace_uprobe.c | 3 +++ 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c index b16e067..0029840 100644 --- a/kernel/trace/trace_eprobe.c +++ b/kernel/trace/trace_eprobe.c @@ -19,6 +19,21 @@ #define EPROBE_EVENT_SYSTEM "eprobes" +static int eprobe_dyn_event_create(const char *raw_command); +static int eprobe_dyn_event_show(struct seq_file *m, struct dyn_event *ev); +static bool eprobe_dyn_event_is_busy(struct dyn_event *ev); +static int eprobe_dyn_event_release(struct dyn_event *ev); +static bool eprobe_dyn_event_match(const char *system, const char *event, + int argc, const char **argv, struct dyn_event *ev); + +static struct dyn_event_operations eprobe_dyn_event_ops = { + .create = eprobe_dyn_event_create, + .show = eprobe_dyn_event_show, + .is_busy = eprobe_dyn_event_is_busy, + .free = eprobe_dyn_event_release, + .match = eprobe_dyn_event_match, +}; + struct trace_eprobe { /* tracepoint system */ const char *event_system; @@ -39,6 +54,11 @@ struct eprobe_data { static int __trace_eprobe_create(int argc, const char *argv[]); +static bool is_trace_eprobe(struct dyn_event *ev) +{ + return ev->ops == &eprobe_dyn_event_ops; +} + static void trace_event_probe_cleanup(struct trace_eprobe *ep) { if (!ep) @@ -121,6 +141,9 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, struct trace_eprobe *ep = to_trace_eprobe(ev); const char *slash; + if (!is_trace_eprobe(ev)) + return false; + /* * We match the following: * event only - match all eprobes with event name @@ -174,14 +197,6 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, return trace_probe_match_command_args(&ep->tp, argc, argv); } -static struct dyn_event_operations eprobe_dyn_event_ops = { - .create = eprobe_dyn_event_create, - .show = eprobe_dyn_event_show, - .is_busy = eprobe_dyn_event_is_busy, - .free = eprobe_dyn_event_release, - .match = eprobe_dyn_event_match, -}; - static struct trace_eprobe *alloc_event_probe(const char *group, const char *this_event, struct trace_event_call *event, diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 2cd8ef9..f63abfa 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -163,6 +163,9 @@ static bool trace_kprobe_match(const char *system, const char *event, { struct trace_kprobe *tk = to_trace_kprobe(ev); + if (!is_trace_kprobe(ev)) + return false; + return (event[0] == '\0' || strcmp(trace_probe_name(&tk->tp), event) == 0) && (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) && diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index a935c08..ee55ed0 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -312,6 +312,9 @@ static bool trace_uprobe_match(const char *system, const char *event, { struct trace_uprobe *tu = to_trace_uprobe(ev); + if (!is_trace_uprobe(ev)) + return false; + return (event[0] == '\0' || strcmp(trace_probe_name(&tu->tp), event) == 0) && (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) && -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tracing/probes: make match function safe 2022-05-01 9:34 ` [PATCH 2/2] tracing/probes: make match function safe Linyu Yuan @ 2022-05-24 22:34 ` Steven Rostedt 2022-05-26 16:03 ` Masami Hiramatsu 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2022-05-24 22:34 UTC (permalink / raw) To: Linyu Yuan; +Cc: Ingo Molnar, linux-kernel, Masami Hiramatsu, Tom Zanussi Masami and Tom, what are you thoughts on this? -- Steve On Sun, May 01, 2022 at 05:34:11PM +0800, Linyu Yuan wrote: > When delete one kprobe/uprobe/eprobe event entry > using /sys/kernel/debug/tracing/dynamic_events file, > it will loop all dynamic envent entries, > as user will not input dyn_event_operations type, > when call the match function of kprobe/uprobe/eprobe, > the dynamic event may have different dyn_event_operations type, > but currently match function may return a match. > > Fix by check dyn_event_operations type first. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > kernel/trace/trace_eprobe.c | 31 +++++++++++++++++++++++-------- > kernel/trace/trace_kprobe.c | 3 +++ > kernel/trace/trace_uprobe.c | 3 +++ > 3 files changed, 29 insertions(+), 8 deletions(-) > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > index b16e067..0029840 100644 > --- a/kernel/trace/trace_eprobe.c > +++ b/kernel/trace/trace_eprobe.c > @@ -19,6 +19,21 @@ > > #define EPROBE_EVENT_SYSTEM "eprobes" > > +static int eprobe_dyn_event_create(const char *raw_command); > +static int eprobe_dyn_event_show(struct seq_file *m, struct dyn_event *ev); > +static bool eprobe_dyn_event_is_busy(struct dyn_event *ev); > +static int eprobe_dyn_event_release(struct dyn_event *ev); > +static bool eprobe_dyn_event_match(const char *system, const char *event, > + int argc, const char **argv, struct dyn_event *ev); > + > +static struct dyn_event_operations eprobe_dyn_event_ops = { > + .create = eprobe_dyn_event_create, > + .show = eprobe_dyn_event_show, > + .is_busy = eprobe_dyn_event_is_busy, > + .free = eprobe_dyn_event_release, > + .match = eprobe_dyn_event_match, > +}; > + > struct trace_eprobe { > /* tracepoint system */ > const char *event_system; > @@ -39,6 +54,11 @@ struct eprobe_data { > > static int __trace_eprobe_create(int argc, const char *argv[]); > > +static bool is_trace_eprobe(struct dyn_event *ev) > +{ > + return ev->ops == &eprobe_dyn_event_ops; > +} > + > static void trace_event_probe_cleanup(struct trace_eprobe *ep) > { > if (!ep) > @@ -121,6 +141,9 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, > struct trace_eprobe *ep = to_trace_eprobe(ev); > const char *slash; > > + if (!is_trace_eprobe(ev)) > + return false; > + > /* > * We match the following: > * event only - match all eprobes with event name > @@ -174,14 +197,6 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, > return trace_probe_match_command_args(&ep->tp, argc, argv); > } > > -static struct dyn_event_operations eprobe_dyn_event_ops = { > - .create = eprobe_dyn_event_create, > - .show = eprobe_dyn_event_show, > - .is_busy = eprobe_dyn_event_is_busy, > - .free = eprobe_dyn_event_release, > - .match = eprobe_dyn_event_match, > -}; > - > static struct trace_eprobe *alloc_event_probe(const char *group, > const char *this_event, > struct trace_event_call *event, > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 2cd8ef9..f63abfa 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -163,6 +163,9 @@ static bool trace_kprobe_match(const char *system, const char *event, > { > struct trace_kprobe *tk = to_trace_kprobe(ev); > > + if (!is_trace_kprobe(ev)) > + return false; > + > return (event[0] == '\0' || > strcmp(trace_probe_name(&tk->tp), event) == 0) && > (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) && > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index a935c08..ee55ed0 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -312,6 +312,9 @@ static bool trace_uprobe_match(const char *system, const char *event, > { > struct trace_uprobe *tu = to_trace_uprobe(ev); > > + if (!is_trace_uprobe(ev)) > + return false; > + > return (event[0] == '\0' || > strcmp(trace_probe_name(&tu->tp), event) == 0) && > (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) && > -- > 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tracing/probes: make match function safe 2022-05-24 22:34 ` Steven Rostedt @ 2022-05-26 16:03 ` Masami Hiramatsu 2022-05-29 2:27 ` Linyu Yuan 0 siblings, 1 reply; 9+ messages in thread From: Masami Hiramatsu @ 2022-05-26 16:03 UTC (permalink / raw) To: Steven Rostedt Cc: Linyu Yuan, Ingo Molnar, linux-kernel, Masami Hiramatsu, Tom Zanussi On Tue, 24 May 2022 18:34:03 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > Masami and Tom, what are you thoughts on this? > > -- Steve Thanks for forwarding. > > On Sun, May 01, 2022 at 05:34:11PM +0800, Linyu Yuan wrote: > > When delete one kprobe/uprobe/eprobe event entry > > using /sys/kernel/debug/tracing/dynamic_events file, > > it will loop all dynamic envent entries, > > as user will not input dyn_event_operations type, > > when call the match function of kprobe/uprobe/eprobe, > > the dynamic event may have different dyn_event_operations type, > > but currently match function may return a match. > > > > Fix by check dyn_event_operations type first. Sorry, NACK. That check is not necessary, because the 'match' operation is chosen by each event::ops as below. int dyn_event_release(const char *raw_command, struct dyn_event_operations *type) { struct dyn_event *pos, *n; ... mutex_lock(&event_mutex); for_each_dyn_event_safe(pos, n) { if (type && type != pos->ops) continue; if (!pos->ops->match(system, event, argc - 1, (const char **)argv + 1, pos)) continue; ... The @pos is dyn_event. Thus @pos->ops must be the appropriate dyn_event_operations for that event. For example, if there is an eprobe event @ev, then @ev->ops must be eprobe_dyn_event_ops and @ev->ops->match must be eprobe_dyn_event_match() (unless the match function is shared with another dyn_event_operations.) Thank you, > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > --- > > kernel/trace/trace_eprobe.c | 31 +++++++++++++++++++++++-------- > > kernel/trace/trace_kprobe.c | 3 +++ > > kernel/trace/trace_uprobe.c | 3 +++ > > 3 files changed, 29 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > > index b16e067..0029840 100644 > > --- a/kernel/trace/trace_eprobe.c > > +++ b/kernel/trace/trace_eprobe.c > > @@ -19,6 +19,21 @@ > > > > #define EPROBE_EVENT_SYSTEM "eprobes" > > > > +static int eprobe_dyn_event_create(const char *raw_command); > > +static int eprobe_dyn_event_show(struct seq_file *m, struct dyn_event *ev); > > +static bool eprobe_dyn_event_is_busy(struct dyn_event *ev); > > +static int eprobe_dyn_event_release(struct dyn_event *ev); > > +static bool eprobe_dyn_event_match(const char *system, const char *event, > > + int argc, const char **argv, struct dyn_event *ev); > > + > > +static struct dyn_event_operations eprobe_dyn_event_ops = { > > + .create = eprobe_dyn_event_create, > > + .show = eprobe_dyn_event_show, > > + .is_busy = eprobe_dyn_event_is_busy, > > + .free = eprobe_dyn_event_release, > > + .match = eprobe_dyn_event_match, > > +}; > > + > > struct trace_eprobe { > > /* tracepoint system */ > > const char *event_system; > > @@ -39,6 +54,11 @@ struct eprobe_data { > > > > static int __trace_eprobe_create(int argc, const char *argv[]); > > > > +static bool is_trace_eprobe(struct dyn_event *ev) > > +{ > > + return ev->ops == &eprobe_dyn_event_ops; > > +} > > + > > static void trace_event_probe_cleanup(struct trace_eprobe *ep) > > { > > if (!ep) > > @@ -121,6 +141,9 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, > > struct trace_eprobe *ep = to_trace_eprobe(ev); > > const char *slash; > > > > + if (!is_trace_eprobe(ev)) > > + return false; > > + > > /* > > * We match the following: > > * event only - match all eprobes with event name > > @@ -174,14 +197,6 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, > > return trace_probe_match_command_args(&ep->tp, argc, argv); > > } > > > > -static struct dyn_event_operations eprobe_dyn_event_ops = { > > - .create = eprobe_dyn_event_create, > > - .show = eprobe_dyn_event_show, > > - .is_busy = eprobe_dyn_event_is_busy, > > - .free = eprobe_dyn_event_release, > > - .match = eprobe_dyn_event_match, > > -}; > > - > > static struct trace_eprobe *alloc_event_probe(const char *group, > > const char *this_event, > > struct trace_event_call *event, > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 2cd8ef9..f63abfa 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -163,6 +163,9 @@ static bool trace_kprobe_match(const char *system, const char *event, > > { > > struct trace_kprobe *tk = to_trace_kprobe(ev); > > > > + if (!is_trace_kprobe(ev)) > > + return false; > > + > > return (event[0] == '\0' || > > strcmp(trace_probe_name(&tk->tp), event) == 0) && > > (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) && > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > > index a935c08..ee55ed0 100644 > > --- a/kernel/trace/trace_uprobe.c > > +++ b/kernel/trace/trace_uprobe.c > > @@ -312,6 +312,9 @@ static bool trace_uprobe_match(const char *system, const char *event, > > { > > struct trace_uprobe *tu = to_trace_uprobe(ev); > > > > + if (!is_trace_uprobe(ev)) > > + return false; > > + > > return (event[0] == '\0' || > > strcmp(trace_probe_name(&tu->tp), event) == 0) && > > (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) && > > -- > > 2.7.4 -- Masami Hiramatsu (Google) <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] tracing/probes: make match function safe 2022-05-26 16:03 ` Masami Hiramatsu @ 2022-05-29 2:27 ` Linyu Yuan 0 siblings, 0 replies; 9+ messages in thread From: Linyu Yuan @ 2022-05-29 2:27 UTC (permalink / raw) To: Masami Hiramatsu (Google), Steven Rostedt Cc: Ingo Molnar, linux-kernel, Tom Zanussi On 5/27/2022 12:03 AM, Masami Hiramatsu (Google) wrote: > On Tue, 24 May 2022 18:34:03 -0400 > Steven Rostedt <rostedt@goodmis.org> wrote: > >> Masami and Tom, what are you thoughts on this? >> >> -- Steve > Thanks for forwarding. > >> On Sun, May 01, 2022 at 05:34:11PM +0800, Linyu Yuan wrote: >>> When delete one kprobe/uprobe/eprobe event entry >>> using /sys/kernel/debug/tracing/dynamic_events file, >>> it will loop all dynamic envent entries, >>> as user will not input dyn_event_operations type, >>> when call the match function of kprobe/uprobe/eprobe, >>> the dynamic event may have different dyn_event_operations type, >>> but currently match function may return a match. >>> >>> Fix by check dyn_event_operations type first. > Sorry, NACK. That check is not necessary, because the 'match' operation > is chosen by each event::ops as below. > > int dyn_event_release(const char *raw_command, struct dyn_event_operations *type) > { > struct dyn_event *pos, *n; > ... > mutex_lock(&event_mutex); > for_each_dyn_event_safe(pos, n) { > if (type && type != pos->ops) > continue; > if (!pos->ops->match(system, event, > argc - 1, (const char **)argv + 1, pos)) > continue; > ... > > The @pos is dyn_event. Thus @pos->ops must be the appropriate > dyn_event_operations for that event. For example, if there is an > eprobe event @ev, then @ev->ops must be eprobe_dyn_event_ops and > @ev->ops->match must be eprobe_dyn_event_match() (unless the match > function is shared with another dyn_event_operations.) > > Thank you, > thanks, yes, there is no need to add this match, if two events have same name and different group, when user delete event, if only input event name, it will delete two events. if delete a specific event, it need input group name, it will not delete event in another group. >>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >>> --- >>> kernel/trace/trace_eprobe.c | 31 +++++++++++++++++++++++-------- >>> kernel/trace/trace_kprobe.c | 3 +++ >>> kernel/trace/trace_uprobe.c | 3 +++ >>> 3 files changed, 29 insertions(+), 8 deletions(-) >>> >>> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c >>> index b16e067..0029840 100644 >>> --- a/kernel/trace/trace_eprobe.c >>> +++ b/kernel/trace/trace_eprobe.c >>> @@ -19,6 +19,21 @@ >>> >>> #define EPROBE_EVENT_SYSTEM "eprobes" >>> >>> +static int eprobe_dyn_event_create(const char *raw_command); >>> +static int eprobe_dyn_event_show(struct seq_file *m, struct dyn_event *ev); >>> +static bool eprobe_dyn_event_is_busy(struct dyn_event *ev); >>> +static int eprobe_dyn_event_release(struct dyn_event *ev); >>> +static bool eprobe_dyn_event_match(const char *system, const char *event, >>> + int argc, const char **argv, struct dyn_event *ev); >>> + >>> +static struct dyn_event_operations eprobe_dyn_event_ops = { >>> + .create = eprobe_dyn_event_create, >>> + .show = eprobe_dyn_event_show, >>> + .is_busy = eprobe_dyn_event_is_busy, >>> + .free = eprobe_dyn_event_release, >>> + .match = eprobe_dyn_event_match, >>> +}; >>> + >>> struct trace_eprobe { >>> /* tracepoint system */ >>> const char *event_system; >>> @@ -39,6 +54,11 @@ struct eprobe_data { >>> >>> static int __trace_eprobe_create(int argc, const char *argv[]); >>> >>> +static bool is_trace_eprobe(struct dyn_event *ev) >>> +{ >>> + return ev->ops == &eprobe_dyn_event_ops; >>> +} >>> + >>> static void trace_event_probe_cleanup(struct trace_eprobe *ep) >>> { >>> if (!ep) >>> @@ -121,6 +141,9 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, >>> struct trace_eprobe *ep = to_trace_eprobe(ev); >>> const char *slash; >>> >>> + if (!is_trace_eprobe(ev)) >>> + return false; >>> + >>> /* >>> * We match the following: >>> * event only - match all eprobes with event name >>> @@ -174,14 +197,6 @@ static bool eprobe_dyn_event_match(const char *system, const char *event, >>> return trace_probe_match_command_args(&ep->tp, argc, argv); >>> } >>> >>> -static struct dyn_event_operations eprobe_dyn_event_ops = { >>> - .create = eprobe_dyn_event_create, >>> - .show = eprobe_dyn_event_show, >>> - .is_busy = eprobe_dyn_event_is_busy, >>> - .free = eprobe_dyn_event_release, >>> - .match = eprobe_dyn_event_match, >>> -}; >>> - >>> static struct trace_eprobe *alloc_event_probe(const char *group, >>> const char *this_event, >>> struct trace_event_call *event, >>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c >>> index 2cd8ef9..f63abfa 100644 >>> --- a/kernel/trace/trace_kprobe.c >>> +++ b/kernel/trace/trace_kprobe.c >>> @@ -163,6 +163,9 @@ static bool trace_kprobe_match(const char *system, const char *event, >>> { >>> struct trace_kprobe *tk = to_trace_kprobe(ev); >>> >>> + if (!is_trace_kprobe(ev)) >>> + return false; >>> + >>> return (event[0] == '\0' || >>> strcmp(trace_probe_name(&tk->tp), event) == 0) && >>> (!system || strcmp(trace_probe_group_name(&tk->tp), system) == 0) && >>> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c >>> index a935c08..ee55ed0 100644 >>> --- a/kernel/trace/trace_uprobe.c >>> +++ b/kernel/trace/trace_uprobe.c >>> @@ -312,6 +312,9 @@ static bool trace_uprobe_match(const char *system, const char *event, >>> { >>> struct trace_uprobe *tu = to_trace_uprobe(ev); >>> >>> + if (!is_trace_uprobe(ev)) >>> + return false; >>> + >>> return (event[0] == '\0' || >>> strcmp(trace_probe_name(&tu->tp), event) == 0) && >>> (!system || strcmp(trace_probe_group_name(&tu->tp), system) == 0) && >>> -- >>> 2.7.4 > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-29 2:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-01 9:34 [PATCH 0/2] tracing/probes: allow no event name input when create group Linyu Yuan 2022-05-01 9:34 ` [PATCH 1/2] tracing/probes: auto generate events name when create a group of events Linyu Yuan 2022-05-24 22:31 ` Steven Rostedt 2022-05-25 1:33 ` Linyu Yuan 2022-05-26 23:36 ` Masami Hiramatsu 2022-05-01 9:34 ` [PATCH 2/2] tracing/probes: make match function safe Linyu Yuan 2022-05-24 22:34 ` Steven Rostedt 2022-05-26 16:03 ` Masami Hiramatsu 2022-05-29 2:27 ` Linyu Yuan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®