Skip to contentSkip to conversation

tracing: Fix removal of eprobes and add test

PATCH In review Steven Rostedt 2 participants 4 messages Oct 13, 2021

Subsystems (2)
Kselftest Tracing
lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

v2 cover tracing: Fix removal of eprobes and add test

When doing the following:

 # echo 'e:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
 # echo '-:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events

It errors out with -ENOENT. This is because the "match" function does not take into account the "timer/hrtimer_cancel" part. Fix it and also make it work more genericly like kprobes and uprobes.

v1 at: https://lore.kernel.org/all/[email protected]/

Changes since v1:
- Instead of just fixing the missing system/event, have it be more like kprobes and uprobes.

Steven Rostedt (VMware) (2):
tracing: Fix event probe removal from dynamic events selftests/ftrace: Update test for more eprobe removal process

----

 kernel/trace/trace_eprobe.c                        | 54 ++++++++++++++++++++--
 .../ftrace/test.d/dynevent/add_remove_eprobe.tc    | 54 +++++++++++++++++++++-
 2 files changed, 103 insertions(+), 5 deletions(-)
author maintainer
#1 lore raw

PATCH v2 1/2 tracing: Fix event probe removal from dynamic events

From: "Steven Rostedt (VMware)" <[email protected]>

When an event probe is to be removed via the API that created it via the dynamic events, an -ENOENT error is returned.

This is because the removal of the event probe does not expect to see the event system and name that the event probe is attached to, even though that's part of the API to create it. As the removal of probes is to use the same API as they are created.

In fact, the removal is not consistent with the kprobes and uprobes removal. Fix that by allowing various ways to remove the eprobe.

The eprobe is created with:

e:[GROUP/]NAME SYSTEM/EVENT [OPTIONS]

Have it get removed by echoing in the following into dynamic_events:

# Remove all eprobes with NAME
echo '-:NAME' >> dynamic_events

# Remove a specific eprobe
echo '-:GROUP/NAME' >> dynamic_events
echo '-:GROUP/NAME SYSTEM/EVENT' >> dynamic_events echo '-:NAME SYSTEM/EVENT' >> dynamic_events
echo '-:GROUP/NAME SYSTEM/EVENT OPTIONS' >> dynamic_events echo '-:NAME SYSTEM/EVENT OPTIONS' >> dynamic_events

Link https://lkml.kernel.org/r/[email protected] open here
Suggested-by Masami Hiramatsu <[email protected]>
Fixes 7491e2c442781 ("tracing: Add a probe that attaches to trace events")
Signed-off-by Steven Rostedt (VMware) <[email protected]>
---
 kernel/trace/trace_eprobe.c | 54 ++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)
kernel/trace/trace_eprobe.c +51 -3
index 570d081929fb..c4a15aef36af 100644
@@ -119,10 +119,58 @@ static bool eprobe_dyn_event_match(const char *system, const char *event,
 			int argc, const char **argv, struct dyn_event *ev)
 {
 	struct trace_eprobe *ep = to_trace_eprobe(ev);
+	const char *slash;
 
-	return strcmp(trace_probe_name(&ep->tp), event) == 0 &&
-	    (!system || strcmp(trace_probe_group_name(&ep->tp), system) == 0) &&
-	    trace_probe_match_command_args(&ep->tp, argc, argv);
+	/*
+	 * We match the following:
+	 *  event only			- match all eprobes with event name
+	 *  system and event only	- match all system/event probes
+	 *
+	 * The below has the above satisfied with more arguments:
+	 *
+	 *  attached system/event	- If the arg has the system and event
+	 *				  the probe is attached to, match
+	 *				  probes with the attachment.
+	 *
+	 *  If any more args are given, then it requires a full match.
+	 */
+
+	/*
+	 * If system exists, but this probe is not part of that system
+	 * do not match.
+	 */
+	if (system && strcmp(trace_probe_group_name(&ep->tp), system) != 0)
+		return false;
+
+	/* Must match the event name */
+	if (strcmp(trace_probe_name(&ep->tp), event) != 0)
+		return false;
+
+	/* No arguments match all */
+	if (argc < 1)
+		return true;
+
+	/* First argument is the system/event the probe is attached to */
+
+	slash = strchr(argv[0], '/');
+	if (!slash)
+		slash = strchr(argv[0], '.');
+	if (!slash)
+		return false;
+
+	if (strncmp(ep->event_system, argv[0], slash - argv[0]))
+		return false;
+	if (strcmp(ep->event_name, slash + 1))
+		return false;
+
+	argc--;
+	argv++;
+
+	/* If there are no other args, then match */
+	if (argc < 1)
+		return true;
+
+	return trace_probe_match_command_args(&ep->tp, argc, argv);
 }
 
 static struct dyn_event_operations eprobe_dyn_event_ops = {
Signature · 1 line
2.32.0
author maintainer
#2 lore raw

PATCH v2 2/2 selftests/ftrace: Update test for more eprobe removal process

In reply to Steven Rostedt

From: "Steven Rostedt (VMware)" <[email protected]>

The removal of eprobes was broken and missed in testing. Add various ways to remove eprobes that are considered acceptable to the testing process to catch when/if they break again.

Signed-off-by Steven Rostedt (VMware) <[email protected]>
---
 .../test.d/dynevent/add_remove_eprobe.tc      | 54 ++++++++++++++++++-
 1 file changed, 52 insertions(+), 2 deletions(-)
tools/testing/selftests/ftrace/test.d/dynevent/add_remove_eprobe.tc +52 -2
index 5f5b2ba3e557..60c02b482be8 100644
@@ -11,8 +11,8 @@ SYSTEM="syscalls"
 EVENT="sys_enter_openat"
 FIELD="filename"
 EPROBE="eprobe_open"
-
-echo "e:$EPROBE $SYSTEM/$EVENT file=+0(\$filename):ustring" >> dynamic_events
+OPTIONS="file=+0(\$filename):ustring"
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
 
 grep -q "$EPROBE" dynamic_events
 test -d events/eprobes/$EPROBE

@@ -37,4 +37,54 @@ echo "-:$EPROBE" >> dynamic_events
 ! grep -q "$EPROBE" dynamic_events
 ! test -d events/eprobes/$EPROBE
 
+# test various ways to remove the probe (already tested with just event name)
+
+# With group name
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With group name and system/event
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE $SYSTEM/$EVENT" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With just event name and system/event
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:$EPROBE $SYSTEM/$EVENT" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With just event name and system/event and options
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# With group name and system/event and options
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+grep -q "$EPROBE" dynamic_events
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
+# Finally make sure what is in the dynamic_events file clears it too
+echo "e:$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+LINE=`sed -e '/$EPROBE/s/^e/-/' < dynamic_events`
+test -d events/eprobes/$EPROBE
+echo "-:eprobes/$EPROBE $SYSTEM/$EVENT $OPTIONS" >> dynamic_events
+! grep -q "$EPROBE" dynamic_events
+! test -d events/eprobes/$EPROBE
+
 clear_trace
Signature · 1 line
2.32.0
maintainer Acked-by on the v2 series
#3 lore raw
In reply to Steven Rostedt
Quoted 9 lines · On Wed, 13 Oct 2021 16:51:11 -0400 Steven Rostedt <[email protected]> wrote:

When doing the following:

 # echo 'e:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
 # echo '-:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events

It errors out with -ENOENT. This is because the "match" function does not take into account the "timer/hrtimer_cancel" part. Fix it and also make it work more genericly like kprobes and uprobes.

Thanks for update. This series looks good to me.

Acked-by Masami Hiramatsu <[email protected]>

for the series.

Thank you!

Quoted 15 lines

v1 at: https://lore.kernel.org/all/[email protected]/

Changes since v1:
- Instead of just fixing the missing system/event, have it be more like kprobes and uprobes.

Steven Rostedt (VMware) (2):
tracing: Fix event probe removal from dynamic events selftests/ftrace: Update test for more eprobe removal process

----

 kernel/trace/trace_eprobe.c                        | 54 ++++++++++++++++++++--
 .../ftrace/test.d/dynevent/add_remove_eprobe.tc    | 54 +++++++++++++++++++++-
 2 files changed, 103 insertions(+), 5 deletions(-)
Signature · 1 line
Masami Hiramatsu <[email protected]>