[PATCH v2] tracing: Fix event probe removal from dynamic events

[PATCH v2] tracing: Fix event probe removal from dynamic events

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

When an event probe is to be removed via the API to remove dynamic events,
an -EBUSY 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, fix it by first testing if the first
parameter of the event probe to be removed matches the system and event
that the probe is attached to, and then adjust the argc and argv of the
parameters to match the rest of the syntax.

Link: https://lkml.kernel.org/r/20211011211105.48b6a5fd@oasis.local.home

Fixes: 7491e2c442781 ("tracing: Add a probe that attaches to trace events")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
Changes since v1:
   - amended the commit with the definition of "slash"

 kernel/trace/trace_eprobe.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 570d081929fb..2bcfa8da5cef 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -119,6 +119,26 @@ 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;
+
+	/* First argument is the system/event the probe is attached to */
+
+	if (argc < 1)
+		return false;
+
+	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++;
 
 	return strcmp(trace_probe_name(&ep->tp), event) == 0 &&
 	    (!system || strcmp(trace_probe_group_name(&ep->tp), system) == 0) &&
-- 
2.31.1

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Masami Hiramatsu]

On Tue, 12 Oct 2021 08:19:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> When an event probe is to be removed via the API to remove dynamic events,
> an -EBUSY 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, fix it by first testing if the first
> parameter of the event probe to be removed matches the system and event
> that the probe is attached to, and then adjust the argc and argv of the
> parameters to match the rest of the syntax.

Hmm, this seems something wrong. Via dynamic_events interface, all the
events must be parsed equaly. If you have to pass the attached "system/event"
that's something wrong. The dynamic_events interface will accept 

-:[GROUP/]EVENT [optional arguments]

Or

!e:[GROUP/]EVENT [optional arguments]

What did you expect other that these syntax?

Thank you,

> 
> Link: https://lkml.kernel.org/r/20211011211105.48b6a5fd@oasis.local.home
> 
> Fixes: 7491e2c442781 ("tracing: Add a probe that attaches to trace events")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> Changes since v1:
>    - amended the commit with the definition of "slash"
> 
>  kernel/trace/trace_eprobe.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 570d081929fb..2bcfa8da5cef 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -119,6 +119,26 @@ 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;
> +
> +	/* First argument is the system/event the probe is attached to */
> +
> +	if (argc < 1)
> +		return false;
> +
> +	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++;
>  
>  	return strcmp(trace_probe_name(&ep->tp), event) == 0 &&
>  	    (!system || strcmp(trace_probe_group_name(&ep->tp), system) == 0) &&
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Steven Rostedt]

On Tue, 12 Oct 2021 23:31:07 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hmm, this seems something wrong. Via dynamic_events interface, all the
> events must be parsed equaly. If you have to pass the attached "system/event"
> that's something wrong. The dynamic_events interface will accept 
> 
> -:[GROUP/]EVENT [optional arguments]
> 
> Or
> 
> !e:[GROUP/]EVENT [optional arguments]
> 
> What did you expect other that these syntax?

But there are non "optional arguments".

To create the event probe, we need to send:

  e:[GROUP/]EVENT system/event [optional arguments]

Where the "system/event" is what we attach to. Similar to adding a function
or address to kprobes. Do you not need to add that for deleting a kprobe?

That is, if I create a kprobe with:

  p:myprobe schedule > dynamic_events

To remove it, don't we need to have:

  -:myprobe schedule >> dynamic_events

?

-- Steve

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Masami Hiramatsu]

On Tue, 12 Oct 2021 12:03:10 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 12 Oct 2021 23:31:07 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hmm, this seems something wrong. Via dynamic_events interface, all the
> > events must be parsed equaly. If you have to pass the attached "system/event"
> > that's something wrong. The dynamic_events interface will accept 
> > 
> > -:[GROUP/]EVENT [optional arguments]
> > 
> > Or
> > 
> > !e:[GROUP/]EVENT [optional arguments]
> > 
> > What did you expect other that these syntax?
> 
> But there are non "optional arguments".
> 
> To create the event probe, we need to send:
> 
>   e:[GROUP/]EVENT system/event [optional arguments]
> 
> Where the "system/event" is what we attach to. Similar to adding a function
> or address to kprobes. Do you not need to add that for deleting a kprobe?

No, since if the GROUP name is given, we can identify the event.

And sorry. I misunderstood your patch, simply I mixed the group/event is
the name of group/event or the attached group/event.

Actually, the dynamic_events delete command is something like wildcard
unless you specify the options.

> 
> That is, if I create a kprobe with:
> 
>   p:myprobe schedule > dynamic_events
> 
> To remove it, don't we need to have:
> 
>   -:myprobe schedule >> dynamic_events

Yes, it is possible. But you can also do

 -:kprobes/myprobe >> dynamic_events 

So, the "schedule" trace point is optional.

Anyway, let me comment on your patch again.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Steven Rostedt]

On Wed, 13 Oct 2021 07:42:26 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Tue, 12 Oct 2021 12:03:10 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 12 Oct 2021 23:31:07 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > Hmm, this seems something wrong. Via dynamic_events interface, all the
> > > events must be parsed equaly. If you have to pass the attached "system/event"
> > > that's something wrong. The dynamic_events interface will accept 
> > > 
> > > -:[GROUP/]EVENT [optional arguments]
> > > 
> > > Or
> > > 
> > > !e:[GROUP/]EVENT [optional arguments]
> > > 
> > > What did you expect other that these syntax?  
> > 
> > But there are non "optional arguments".
> > 
> > To create the event probe, we need to send:
> > 
> >   e:[GROUP/]EVENT system/event [optional arguments]
> > 
> > Where the "system/event" is what we attach to. Similar to adding a function
> > or address to kprobes. Do you not need to add that for deleting a kprobe?  
> 
> No, since if the GROUP name is given, we can identify the event.
> 
> And sorry. I misunderstood your patch, simply I mixed the group/event is
> the name of group/event or the attached group/event.

The GROUP/EVENT is the name of the created eprobe, my "system/event" is the
name of the probe being attached.

> 
> Actually, the dynamic_events delete command is something like wildcard
> unless you specify the options.

OK, so we need to update this a little bit more. But currently, it fails if
you do:

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

It will error out with "-EBUSY".

It would make sense if we echo in exactly what is in dynamic_events with
"-:" in front of it it, that it will remove it. But currently, it expects:

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

Which is completely wrong.


> 
> > 
> > That is, if I create a kprobe with:
> > 
> >   p:myprobe schedule > dynamic_events
> > 
> > To remove it, don't we need to have:
> > 
> >   -:myprobe schedule >> dynamic_events  
> 
> Yes, it is possible. But you can also do
> 
>  -:kprobes/myprobe >> dynamic_events 
> 
> So, the "schedule" trace point is optional.
> 
> Anyway, let me comment on your patch again.

OK, we can make it optional, but we need to fix it so that it also works.

-- Steve

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Masami Hiramatsu]

On Tue, 12 Oct 2021 20:08:56 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 13 Oct 2021 07:42:26 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Tue, 12 Oct 2021 12:03:10 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Tue, 12 Oct 2021 23:31:07 +0900
> > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >   
> > > > Hmm, this seems something wrong. Via dynamic_events interface, all the
> > > > events must be parsed equaly. If you have to pass the attached "system/event"
> > > > that's something wrong. The dynamic_events interface will accept 
> > > > 
> > > > -:[GROUP/]EVENT [optional arguments]
> > > > 
> > > > Or
> > > > 
> > > > !e:[GROUP/]EVENT [optional arguments]
> > > > 
> > > > What did you expect other that these syntax?  
> > > 
> > > But there are non "optional arguments".
> > > 
> > > To create the event probe, we need to send:
> > > 
> > >   e:[GROUP/]EVENT system/event [optional arguments]
> > > 
> > > Where the "system/event" is what we attach to. Similar to adding a function
> > > or address to kprobes. Do you not need to add that for deleting a kprobe?  
> > 
> > No, since if the GROUP name is given, we can identify the event.
> > 
> > And sorry. I misunderstood your patch, simply I mixed the group/event is
> > the name of group/event or the attached group/event.
> 
> The GROUP/EVENT is the name of the created eprobe, my "system/event" is the
> name of the probe being attached.

OK, I understand that.

> 
> > 
> > Actually, the dynamic_events delete command is something like wildcard
> > unless you specify the options.
> 
> OK, so we need to update this a little bit more. But currently, it fails if
> you do:
> 
>  # echo 'e:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
>  # cat dynamic_events
> eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8

BTW, Isn't there 'e:' prefix like below?
e:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8

>  # echo '-:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
> 
> It will error out with "-EBUSY".

That may be another reason. (Did you enable the eprobes/hrstate?)
If the event doesn't match, it will return -ENOENT.

> 
> It would make sense if we echo in exactly what is in dynamic_events with
> "-:" in front of it it, that it will remove it. But currently, it expects:
> 
>  # echo '-:eprobes/hrstate state=+0x38($hrtimer):u8' >> dynamic_events
> 
> Which is completely wrong.

Ah, I see what was wrong... you used trace_probe_match_command_args() but
didn't skip the attached system/event. Thus the patch check and skipped it.

> > > That is, if I create a kprobe with:
> > > 
> > >   p:myprobe schedule > dynamic_events
> > > 
> > > To remove it, don't we need to have:
> > > 
> > >   -:myprobe schedule >> dynamic_events  
> > 
> > Yes, it is possible. But you can also do
> > 
> >  -:kprobes/myprobe >> dynamic_events 
> > 
> > So, the "schedule" trace point is optional.
> > 
> > Anyway, let me comment on your patch again.
> 
> OK, we can make it optional, but we need to fix it so that it also works.

Agreed.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Steven Rostedt]

On Wed, 13 Oct 2021 23:42:06 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > Actually, the dynamic_events delete command is something like wildcard
> > > unless you specify the options.  
> > 
> > OK, so we need to update this a little bit more. But currently, it fails if
> > you do:
> > 
> >  # echo 'e:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
> >  # cat dynamic_events
> > eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8  
> 
> BTW, Isn't there 'e:' prefix like below?
> e:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8

Yes, that works too, but you can leave off the GROUP/ when creating the
event, which it will default to "eprobes/".

> 
> >  # echo '-:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
> > 
> > It will error out with "-EBUSY".  
> 
> That may be another reason. (Did you enable the eprobes/hrstate?)
> If the event doesn't match, it will return -ENOENT.

Hmm, now trying to reproduce it, I'm getting:

  -bash: echo: write error: No such file or directory

Which would be -ENOENT. Before I was getting -EBUSY when attaching eprobes
to kprobes with your $$args patch. But that's not applied right now, so I
can't try to reproduce it at the moment. I probably hit something else at
the time anyway.

But still, it's broken, and I need to fix it. I'll make the updates to
allow the above. Perhaps I'll include a test that tests the above too.

-- Steve

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Masami Hiramatsu]

On Tue, 12 Oct 2021 08:19:25 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> When an event probe is to be removed via the API to remove dynamic events,
> an -EBUSY 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, fix it by first testing if the first
> parameter of the event probe to be removed matches the system and event
> that the probe is attached to, and then adjust the argc and argv of the
> parameters to match the rest of the syntax.
> 
> Link: https://lkml.kernel.org/r/20211011211105.48b6a5fd@oasis.local.home
> 
> Fixes: 7491e2c442781 ("tracing: Add a probe that attaches to trace events")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> Changes since v1:
>    - amended the commit with the definition of "slash"
> 
>  kernel/trace/trace_eprobe.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 570d081929fb..2bcfa8da5cef 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -119,6 +119,26 @@ 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;
> +
> +	/* First argument is the system/event the probe is attached to */
> +
> +	if (argc < 1)
> +		return false;

The first argument check should be optional. If the event name matches and
the system name is NULL but argc == 0, it should return true.
(please consider it is a wild card like "-:*/EVENT *")
So if the argc == 0 please skip below and check the event name and
the system name.

Thank you,

> +
> +	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++;
>  
>  	return strcmp(trace_probe_name(&ep->tp), event) == 0 &&
>  	    (!system || strcmp(trace_probe_group_name(&ep->tp), system) == 0) &&
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Steven Rostedt]

On Wed, 13 Oct 2021 07:46:11 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> The first argument check should be optional. If the event name matches and
> the system name is NULL but argc == 0, it should return true.
> (please consider it is a wild card like "-:*/EVENT *")
> So if the argc == 0 please skip below and check the event name and
> the system name.

OK, so I'll make them all optional, but at least, if they are supplied,
they will be checked.

That is, you can't add the options if you don't add the event as well. But
if you do add the event, then it should work.

Basically, we have the following:

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

Then the following should work:

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

But the following will not work:

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


Should this work?

  # echo '-:hrstate' >> dynamic_events

-- Steve

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Masami Hiramatsu]

On Tue, 12 Oct 2021 20:15:59 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 13 Oct 2021 07:46:11 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > The first argument check should be optional. If the event name matches and
> > the system name is NULL but argc == 0, it should return true.
> > (please consider it is a wild card like "-:*/EVENT *")
> > So if the argc == 0 please skip below and check the event name and
> > the system name.
> 
> OK, so I'll make them all optional, but at least, if they are supplied,
> they will be checked.
> 
> That is, you can't add the options if you don't add the event as well. But
> if you do add the event, then it should work.
> 
> Basically, we have the following:
> 
>  # echo 'e:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
>  # cat dynamic_events
> eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8
> 
> Then the following should work:
> 
>   # echo '-:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
>   # echo '-:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
>   # echo '-:eprobes/hrstate timer.hrtimer_cancel' >> dynamic_events
>   # echo '-:eprobes/hrstate' >> dynamic_events

Agreed.

> 
> But the following will not work:
> 
>   # echo '-:hrstate state=+0x38($hrtimer):u8' >> dynamic_events
>   # echo '-:hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
>   # echo '-:hrstate timer.hrtimer_cancel' >> dynamic_events

The first one is agreed. But the rest 2 cases should work because it just omits the
group name. At least {k,u}probe events work.

> Should this work?
> 
>   # echo '-:hrstate' >> dynamic_events

Yes. In this case, all dynamic events which have "hrstate" event name are removed.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2] tracing: Fix event probe removal from dynamic events

[Steven Rostedt]

On Wed, 13 Oct 2021 23:32:44 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > Then the following should work:
> > 
> >   # echo '-:hrstate timer/hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
> >   # echo '-:eprobes/hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
> >   # echo '-:eprobes/hrstate timer.hrtimer_cancel' >> dynamic_events
> >   # echo '-:eprobes/hrstate' >> dynamic_events  
> 
> Agreed.
> 
> > 
> > But the following will not work:
> > 
> >   # echo '-:hrstate state=+0x38($hrtimer):u8' >> dynamic_events
> >   # echo '-:hrstate timer.hrtimer_cancel state=+0x38($hrtimer):u8' >> dynamic_events
> >   # echo '-:hrstate timer.hrtimer_cancel' >> dynamic_events  
> 
> The first one is agreed. But the rest 2 cases should work because it just omits the
> group name. At least {k,u}probe events work.
> 
> > Should this work?
> > 
> >   # echo '-:hrstate' >> dynamic_events  
> 
> Yes. In this case, all dynamic events which have "hrstate" event name are removed.


Thanks for the feedback. I'll update it.

-- Steve