[PATCH 0/3] tracing/kprobes: Fix event generation API etc.

[PATCH 0/3] tracing/kprobes: Fix event generation API etc.

[Masami Hiramatsu]

Hello,

Here are bugfix/cleanup patches for the kprobe tracer, [1/3]
is a typo fix, [2/3] is fixing boot-time tracer and [3/3] is
error checking for the new in-kernel kprobe event API.

Tom, I found that your commit 29a154810546 ("tracing: Change
trace_boot to use kprobe_event interface") broke the boot-time
tracer's kprobe event because of wrong API usage. Could you
review it?

I marked [3/3] as a bugfix, because if the loc == NULL, 
__kprobe_event_gen_cmd_start() obviously does not work.
But it reports actual error at kprobe_event_gen_cmd_end().
That is not good for developers to debug it.

Thank you,

---

Masami Hiramatsu (3):
      tracing/kprobes: Fix a double initialization typo
      tracing/boottime: Fix kprobe event API usage
      tracing/kprobes: Reject new event if loc is NULL


 kernel/trace/trace_boot.c   |   20 ++++++++------------
 kernel/trace/trace_kprobe.c |    8 +++++++-
 2 files changed, 15 insertions(+), 13 deletions(-)

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

[PATCH 1/3] tracing/kprobes: Fix a double initialization typo

[Masami Hiramatsu]

Fix a typo that resulted in an unnecessary double
initialization to addr.

Fixes: c7411a1a126f ("tracing/kprobe: Check whether the non-suffixed symbol is notrace")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 kernel/trace/trace_kprobe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index d0568af4a0ef..0d9300c3b084 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -453,7 +453,7 @@ static bool __within_notrace_func(unsigned long addr)
 
 static bool within_notrace_func(struct trace_kprobe *tk)
 {
-	unsigned long addr = addr = trace_kprobe_address(tk);
+	unsigned long addr = trace_kprobe_address(tk);
 	char symname[KSYM_NAME_LEN], *p;
 
 	if (!__within_notrace_func(addr))

[PATCH 2/3] tracing/boottime: Fix kprobe event API usage

[Masami Hiramatsu]

Fix boottime kprobe events to use API correctly for
multiple events.

For example, when we set a multiprobe kprobe events in
bootconfig like below,

  ftrace.event.kprobes.myevent {
  	probes = "vfs_read $arg1 $arg2", "vfs_write $arg1 $arg2"
  }

This cause an error;

  trace_boot: Failed to add probe: p:kprobes/myevent (null)  vfs_read $arg1 $arg2  vfs_write $arg1 $arg2

This shows the 1st argument becomes NULL and multiprobes
are merged to 1 probe.

Fixes: 29a154810546 ("tracing: Change trace_boot to use kprobe_event interface")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 kernel/trace/trace_boot.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
index 06d7feb5255f..9de29bb45a27 100644
--- a/kernel/trace/trace_boot.c
+++ b/kernel/trace/trace_boot.c
@@ -95,24 +95,20 @@ trace_boot_add_kprobe_event(struct xbc_node *node, const char *event)
 	struct xbc_node *anode;
 	char buf[MAX_BUF_LEN];
 	const char *val;
-	int ret;
+	int ret = 0;
 
-	kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
+	xbc_node_for_each_array_value(node, "probes", anode, val) {
+		kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
 
-	ret = kprobe_event_gen_cmd_start(&cmd, event, NULL);
-	if (ret)
-		return ret;
+		ret = kprobe_event_gen_cmd_start(&cmd, event, val);
+		if (ret)
+			break;
 
-	xbc_node_for_each_array_value(node, "probes", anode, val) {
-		ret = kprobe_event_add_field(&cmd, val);
+		ret = kprobe_event_gen_cmd_end(&cmd);
 		if (ret)
-			return ret;
+			pr_err("Failed to add probe: %s\n", buf);
 	}
 
-	ret = kprobe_event_gen_cmd_end(&cmd);
-	if (ret)
-		pr_err("Failed to add probe: %s\n", buf);
-
 	return ret;
 }
 #else

Re: [PATCH 2/3] tracing/boottime: Fix kprobe event API usage

[Steven Rostedt]

On Sat, 25 Apr 2020 14:49:17 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fix boottime kprobe events to use API correctly for
> multiple events.
> 
> For example, when we set a multiprobe kprobe events in
> bootconfig like below,
> 
>   ftrace.event.kprobes.myevent {
>   	probes = "vfs_read $arg1 $arg2", "vfs_write $arg1 $arg2"
>   }
> 
> This cause an error;
> 
>   trace_boot: Failed to add probe: p:kprobes/myevent (null)  vfs_read $arg1 $arg2  vfs_write $arg1 $arg2
> 
> This shows the 1st argument becomes NULL and multiprobes
> are merged to 1 probe.
> 
> Fixes: 29a154810546 ("tracing: Change trace_boot to use kprobe_event interface")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  kernel/trace/trace_boot.c |   20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> index 06d7feb5255f..9de29bb45a27 100644
> --- a/kernel/trace/trace_boot.c
> +++ b/kernel/trace/trace_boot.c
> @@ -95,24 +95,20 @@ trace_boot_add_kprobe_event(struct xbc_node *node, const char *event)
>  	struct xbc_node *anode;
>  	char buf[MAX_BUF_LEN];
>  	const char *val;
> -	int ret;
> +	int ret = 0;
>  
> -	kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
> +	xbc_node_for_each_array_value(node, "probes", anode, val) {
> +		kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
>  
> -	ret = kprobe_event_gen_cmd_start(&cmd, event, NULL);
> -	if (ret)
> -		return ret;
> +		ret = kprobe_event_gen_cmd_start(&cmd, event, val);
> +		if (ret)
> +			break;

Should we break here? What about just printing an error message and
continuing to the next probe. If I start up something with a typo in
the first element, I lose all events. But if I have a typo in the last
one, I get all but that one. I rather have it just fail on the ones that
don't parse properly.

-- Steve


>  
> -	xbc_node_for_each_array_value(node, "probes", anode, val) {
> -		ret = kprobe_event_add_field(&cmd, val);
> +		ret = kprobe_event_gen_cmd_end(&cmd);
>  		if (ret)
> -			return ret;
> +			pr_err("Failed to add probe: %s\n", buf);
>  	}
>  
> -	ret = kprobe_event_gen_cmd_end(&cmd);
> -	if (ret)
> -		pr_err("Failed to add probe: %s\n", buf);
> -
>  	return ret;
>  }
>  #else

Re: [PATCH 2/3] tracing/boottime: Fix kprobe event API usage

[Masami Hiramatsu]

On Sat, 25 Apr 2020 10:00:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 25 Apr 2020 14:49:17 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Fix boottime kprobe events to use API correctly for
> > multiple events.
> > 
> > For example, when we set a multiprobe kprobe events in
> > bootconfig like below,
> > 
> >   ftrace.event.kprobes.myevent {
> >   	probes = "vfs_read $arg1 $arg2", "vfs_write $arg1 $arg2"
> >   }
> > 
> > This cause an error;
> > 
> >   trace_boot: Failed to add probe: p:kprobes/myevent (null)  vfs_read $arg1 $arg2  vfs_write $arg1 $arg2
> > 
> > This shows the 1st argument becomes NULL and multiprobes
> > are merged to 1 probe.
> > 
> > Fixes: 29a154810546 ("tracing: Change trace_boot to use kprobe_event interface")
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  kernel/trace/trace_boot.c |   20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> > index 06d7feb5255f..9de29bb45a27 100644
> > --- a/kernel/trace/trace_boot.c
> > +++ b/kernel/trace/trace_boot.c
> > @@ -95,24 +95,20 @@ trace_boot_add_kprobe_event(struct xbc_node *node, const char *event)
> >  	struct xbc_node *anode;
> >  	char buf[MAX_BUF_LEN];
> >  	const char *val;
> > -	int ret;
> > +	int ret = 0;
> >  
> > -	kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
> > +	xbc_node_for_each_array_value(node, "probes", anode, val) {
> > +		kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
> >  
> > -	ret = kprobe_event_gen_cmd_start(&cmd, event, NULL);
> > -	if (ret)
> > -		return ret;
> > +		ret = kprobe_event_gen_cmd_start(&cmd, event, val);
> > +		if (ret)
> > +			break;
> 
> Should we break here? What about just printing an error message and
> continuing to the next probe. If I start up something with a typo in
> the first element, I lose all events. But if I have a typo in the last
> one, I get all but that one. I rather have it just fail on the ones that
> don't parse properly.

This kprobe_event_gen_cmd_start() causes an error only if there is
a program bug or out of memory, because it never evaluate given probe
definition, but kprobe_event_gen_cmd_end() does. Thus I think this is
correct way to handle the error.

IOW, if you typo a probe, it will be handled by
kprobe_event_gen_cmd_end() and it shows an error message and continue
to process other probe definitions. See below,

> > -	xbc_node_for_each_array_value(node, "probes", anode, val) {
> > -		ret = kprobe_event_add_field(&cmd, val);
> > +		ret = kprobe_event_gen_cmd_end(&cmd);
> >  		if (ret)
> > -			return ret;
> > +			pr_err("Failed to add probe: %s\n", buf);
> >  	}

This continues to next probe ;-)

Thank you,

> >  
> > -	ret = kprobe_event_gen_cmd_end(&cmd);
> > -	if (ret)
> > -		pr_err("Failed to add probe: %s\n", buf);
> > -
> >  	return ret;
> >  }
> >  #else
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/3] tracing/boottime: Fix kprobe event API usage

[Tom Zanussi]

Hi Masami,

On Sat, 2020-04-25 at 14:49 +0900, Masami Hiramatsu wrote:
> Fix boottime kprobe events to use API correctly for
> multiple events.
> 
> For example, when we set a multiprobe kprobe events in
> bootconfig like below,
> 
>   ftrace.event.kprobes.myevent {
>   	probes = "vfs_read $arg1 $arg2", "vfs_write $arg1 $arg2"
>   }
> 
> This cause an error;
> 
>   trace_boot: Failed to add probe: p:kprobes/myevent (null)  vfs_read
> $arg1 $arg2  vfs_write $arg1 $arg2
> 
> This shows the 1st argument becomes NULL and multiprobes
> are merged to 1 probe.
> 
> Fixes: 29a154810546 ("tracing: Change trace_boot to use kprobe_event
> interface")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: stable@vger.kernel.org

Thanks for fixing this!

Reviewed-by: Tom Zanussi <zanussi@kernel.org>


> ---
>  kernel/trace/trace_boot.c |   20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> index 06d7feb5255f..9de29bb45a27 100644
> --- a/kernel/trace/trace_boot.c
> +++ b/kernel/trace/trace_boot.c
> @@ -95,24 +95,20 @@ trace_boot_add_kprobe_event(struct xbc_node
> *node, const char *event)
>  	struct xbc_node *anode;
>  	char buf[MAX_BUF_LEN];
>  	const char *val;
> -	int ret;
> +	int ret = 0;
>  
> -	kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
> +	xbc_node_for_each_array_value(node, "probes", anode, val) {
> +		kprobe_event_cmd_init(&cmd, buf, MAX_BUF_LEN);
>  
> -	ret = kprobe_event_gen_cmd_start(&cmd, event, NULL);
> -	if (ret)
> -		return ret;
> +		ret = kprobe_event_gen_cmd_start(&cmd, event, val);
> +		if (ret)
> +			break;
>  
> -	xbc_node_for_each_array_value(node, "probes", anode, val) {
> -		ret = kprobe_event_add_field(&cmd, val);
> +		ret = kprobe_event_gen_cmd_end(&cmd);
>  		if (ret)
> -			return ret;
> +			pr_err("Failed to add probe: %s\n", buf);
>  	}
>  
> -	ret = kprobe_event_gen_cmd_end(&cmd);
> -	if (ret)
> -		pr_err("Failed to add probe: %s\n", buf);
> -
>  	return ret;
>  }
>  #else
> 

[PATCH 3/3] tracing/kprobes: Reject new event if loc is NULL

[Masami Hiramatsu]

Reject the new event which has NULL location for kprobes.
For kprobes, user must specify at least the location.

Fixes: 2a588dd1d5d6 ("tracing: Add kprobe event command generation functions")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 kernel/trace/trace_kprobe.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 0d9300c3b084..35989383ae11 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -940,6 +940,9 @@ EXPORT_SYMBOL_GPL(kprobe_event_cmd_init);
  * complete command or only the first part of it; in the latter case,
  * kprobe_event_add_fields() can be used to add more fields following this.
  *
+ * Unlikely the synth_event_gen_cmd_start(), @loc must be specified. This
+ * returns -EINVAL if @loc == NULL.
+ *
  * Return: 0 if successful, error otherwise.
  */
 int __kprobe_event_gen_cmd_start(struct dynevent_cmd *cmd, bool kretprobe,
@@ -953,6 +956,9 @@ int __kprobe_event_gen_cmd_start(struct dynevent_cmd *cmd, bool kretprobe,
 	if (cmd->type != DYNEVENT_TYPE_KPROBE)
 		return -EINVAL;
 
+	if (!loc)
+		return -EINVAL;
+
 	if (kretprobe)
 		snprintf(buf, MAX_EVENT_NAME_LEN, "r:kprobes/%s", name);
 	else

Re: [PATCH 0/3] tracing/kprobes: Fix event generation API etc.

[Masami Hiramatsu]

Hi Steve,

It seems this fixes are not picked up yet.
Would you have any consideration?

Thank you,

On Sat, 25 Apr 2020 14:48:59 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hello,
> 
> Here are bugfix/cleanup patches for the kprobe tracer, [1/3]
> is a typo fix, [2/3] is fixing boot-time tracer and [3/3] is
> error checking for the new in-kernel kprobe event API.
> 
> Tom, I found that your commit 29a154810546 ("tracing: Change
> trace_boot to use kprobe_event interface") broke the boot-time
> tracer's kprobe event because of wrong API usage. Could you
> review it?
> 
> I marked [3/3] as a bugfix, because if the loc == NULL, 
> __kprobe_event_gen_cmd_start() obviously does not work.
> But it reports actual error at kprobe_event_gen_cmd_end().
> That is not good for developers to debug it.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       tracing/kprobes: Fix a double initialization typo
>       tracing/boottime: Fix kprobe event API usage
>       tracing/kprobes: Reject new event if loc is NULL
> 
> 
>  kernel/trace/trace_boot.c   |   20 ++++++++------------
>  kernel/trace/trace_kprobe.c |    8 +++++++-
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/3] tracing/kprobes: Fix event generation API etc.

[Steven Rostedt]

On Wed, 20 May 2020 23:22:20 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Steve,
> 
> It seems this fixes are not picked up yet.
> Would you have any consideration?
> 


Ah, I missed your reply to my comment :-/

Yeah, I'll pull that in now and start testing it.

Thanks for the reminder.

-- Steve

Re: [PATCH 0/3] tracing/kprobes: Fix event generation API etc.

[Steven Rostedt]

On Wed, 20 May 2020 10:33:46 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 20 May 2020 23:22:20 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi Steve,
> > 
> > It seems this fixes are not picked up yet.
> > Would you have any consideration?
> >   
> 
> 
> Ah, I missed your reply to my comment :-/
> 
> Yeah, I'll pull that in now and start testing it.
> 
> Thanks for the reminder.

No, it's already in mainline:

Merged: 192ffb7515839b1cc8457e0a8c1e09783de019d3

With commits:

 dcbd21c9fca5e954fd4e3d91884907eb6d47187e
 da0f1f4167e3af69e1d8b32d6d65195ddd2bfb64
 5b4dcd2d201a395ad4054067bfae4a07554fbd65

-- Steve

 

Re: [PATCH 0/3] tracing/kprobes: Fix event generation API etc.

[Masami Hiramatsu]

On Wed, 20 May 2020 10:40:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 20 May 2020 10:33:46 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Wed, 20 May 2020 23:22:20 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > Hi Steve,
> > > 
> > > It seems this fixes are not picked up yet.
> > > Would you have any consideration?
> > >   
> > 
> > 
> > Ah, I missed your reply to my comment :-/
> > 
> > Yeah, I'll pull that in now and start testing it.
> > 
> > Thanks for the reminder.
> 
> No, it's already in mainline:
> 
> Merged: 192ffb7515839b1cc8457e0a8c1e09783de019d3
> 
> With commits:
> 
>  dcbd21c9fca5e954fd4e3d91884907eb6d47187e
>  da0f1f4167e3af69e1d8b32d6d65195ddd2bfb64
>  5b4dcd2d201a395ad4054067bfae4a07554fbd65

Oops, I must have checked another branch. Sorry.

Thank you!

-- 
Masami Hiramatsu <mhiramat@kernel.org>