[PATCH v4 0/3] tracing/probes: allow no event name input when create group

[PATCH v4 0/3] tracing/probes: allow no event name input when create group

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

V1: https://lore.kernel.org/lkml/1651397651-30454-1-git-send-email-quic_linyyuan@quicinc.com/

V2: fix remove comment in V1 patch1,
    remove v1 patch2 as it is NACK.

v3: (v2 link: https://lore.kernel.org/lkml/1653795294-19764-1-git-send-email-quic_linyyuan@quicinc.com/)
    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/1653978552-18637-1-git-send-email-quic_linyyuan@quicinc.com/)
    fix comment of kprobe/eprobe test case.

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                        | 25 +++++++++++-----------
 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, 64 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH v4 1/3] tracing: eprobe: remove duplicate is_good_name() operation

[Linyu Yuan]

traceprobe_parse_event_name() already validate group and event name,
there is no need to call is_good_name() after it.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
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

 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

[PATCH v4 2/3] tracing: auto generate event name when create a group of events

[Linyu Yuan]

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.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
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

 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          | 21 ++++++++++++---------
 kernel/trace/trace_kprobe.c          | 16 ++++++++++------
 kernel/trace/trace_probe.c           |  6 ++++++
 kernel/trace/trace_uprobe.c          | 12 ++++++++----
 8 files changed, 49 insertions(+), 32 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..be9a94d 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]);
+	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
 	if (ret || !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

[PATCH v4 3/3] selftests/ftrace: add test case for GRP/ only input

[Linyu Yuan]

add kprobe and eprobe event test for new GRP/ only format.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: first add in this version
v4: remove restriction of test case

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

Re: [PATCH v4 0/3] tracing/probes: allow no event name input when create group

[Masami Hiramatsu]

Hi Linyu,

On Thu, 2 Jun 2022 20:10:58 +0800
Linyu Yuan <quic_linyyuan@quicinc.com> wrote:

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

Thanks for updating the series. This looks godd to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

for this series.

Thank you!

> 
> V1: https://lore.kernel.org/lkml/1651397651-30454-1-git-send-email-quic_linyyuan@quicinc.com/
> 
> V2: fix remove comment in V1 patch1,
>     remove v1 patch2 as it is NACK.
> 
> v3: (v2 link: https://lore.kernel.org/lkml/1653795294-19764-1-git-send-email-quic_linyyuan@quicinc.com/)
>     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/1653978552-18637-1-git-send-email-quic_linyyuan@quicinc.com/)
>     fix comment of kprobe/eprobe test case.
> 
> 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                        | 25 +++++++++++-----------
>  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, 64 insertions(+), 37 deletions(-)
> 
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v4 1/3] tracing: eprobe: remove duplicate is_good_name() operation

[Tom Zanussi]

Hi Linhu,

On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
> traceprobe_parse_event_name() already validate group and event name,
> there is no need to call is_good_name() after it.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> 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
> 
>  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;

traceprobe_parse_event_name() is only called if (event).  In the
!event case, wouldn't the is_good_name() checks still be needed (since
in that case buf1 is assigned to event)?

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

I agree this one isn't needed.

Thanks,

Tom

>  
>         mutex_lock(&event_mutex);
>         event_call = find_and_get_event(sys_name, sys_event);

Re: [PATCH v4 1/3] tracing: eprobe: remove duplicate is_good_name() operation

[Linyu Yuan]

hi Tom,

On 6/14/2022 5:01 AM, Tom Zanussi wrote:
> Hi Linhu,
>
> On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
>> traceprobe_parse_event_name() already validate group and event name,
>> there is no need to call is_good_name() after it.
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>> 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
>>
>>   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;
> traceprobe_parse_event_name() is only called if (event).  In the
> !event case, wouldn't the is_good_name() checks still be needed (since
> in that case buf1 is assigned to event)?

when user input no  event name, it will generate event name from second  
SYSTEM.EVENT,

and it will validate with following traceprobe_parse_event_name().


(

if you agree, i will send a new version to update a minor issue in 
second patch,


         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;

)

>
>>   
>>          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;
> I agree this one isn't needed.
>
> Thanks,
>
> Tom
>
>>   
>>          mutex_lock(&event_mutex);
>>          event_call = find_and_get_event(sys_name, sys_event);

Re: [PATCH v4 1/3] tracing: eprobe: remove duplicate is_good_name() operation

[Tom Zanussi]

Hi Linyu,

On Tue, 2022-06-14 at 08:48 +0800, Linyu Yuan wrote:
> hi Tom,
> 
> On 6/14/2022 5:01 AM, Tom Zanussi wrote:
> > Hi Linhu,
> > 
> > On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
> > > traceprobe_parse_event_name() already validate group and event
> > > name,
> > > there is no need to call is_good_name() after it.
> > > 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > 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
> > > 
> > >   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;
> > traceprobe_parse_event_name() is only called if (event).  In the
> > !event case, wouldn't the is_good_name() checks still be needed
> > (since
> > in that case buf1 is assigned to event)?
> 
> when user input no  event name, it will generate event name from
> second  
> SYSTEM.EVENT,
> 
> and it will validate with following traceprobe_parse_event_name().
> 
> 

Right, but that happens in your second patch '[PATCH v5 2/3] tracing:
auto generate event name when create a group of events', not this one.
 
So if you apply only this patch, the !event case will assign event but
it will remain unchecked when used later in this function.

It would make more sense to remove this check in patch 2/3 along with
the code that does the generating...

> (
> 
> if you agree, i will send a new version to update a minor issue in 
> second patch,
> 
> 
>          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;
> 
> )
> 
> > 
> > >   
> > >          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;
> > I agree this one isn't needed.

But keep this one in this patch, since it's useful on its own as a
standalone cleanup regardless of whether or not patch 2/3 gets merged.

Tom

> > Thanks,
> > 
> > Tom
> > 
> > >   
> > >          mutex_lock(&event_mutex);
> > >          event_call = find_and_get_event(sys_name, sys_event);

Re: [PATCH v4 1/3] tracing: eprobe: remove duplicate is_good_name() operation

[Linyu Yuan]

hi Tom,

On 6/21/2022 2:38 AM, Tom Zanussi wrote:
> Hi Linyu,
>
> On Tue, 2022-06-14 at 08:48 +0800, Linyu Yuan wrote:
>> hi Tom,
>>
>> On 6/14/2022 5:01 AM, Tom Zanussi wrote:
>>> Hi Linhu,
>>>
>>> On Thu, 2022-06-02 at 20:10 +0800, Linyu Yuan wrote:
>>>> traceprobe_parse_event_name() already validate group and event
>>>> name,
>>>> there is no need to call is_good_name() after it.
>>>>
>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>>> ---
>>>> 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
>>>>
>>>>    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;
>>> traceprobe_parse_event_name() is only called if (event).  In the
>>> !event case, wouldn't the is_good_name() checks still be needed
>>> (since
>>> in that case buf1 is assigned to event)?
>> when user input no  event name, it will generate event name from
>> second
>> SYSTEM.EVENT,
>>
>> and it will validate with following traceprobe_parse_event_name().
>>
>>
> Right, but that happens in your second patch '[PATCH v5 2/3] tracing:
> auto generate event name when create a group of events', not this one.
>   
> So if you apply only this patch, the !event case will assign event but
> it will remain unchecked when used later in this function.
>
> It would make more sense to remove this check in patch 2/3 along with
> the code that does the generating...
thanks, will do like this.
>
>> (
>>
>> if you agree, i will send a new version to update a minor issue in
>> second patch,
>>
>>
>>           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;
>>
>> )
>>
>>>>    
>>>>           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;
>>> I agree this one isn't needed.
> But keep this one in this patch, since it's useful on its own as a
> standalone cleanup regardless of whether or not patch 2/3 gets merged.
>
> Tom
>
>>> Thanks,
>>>
>>> Tom
>>>
>>>>    
>>>>           mutex_lock(&event_mutex);
>>>>           event_call = find_and_get_event(sys_name, sys_event);