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) <[email protected]>
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 <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Cc: [email protected]
---
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))
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 <[email protected]>
Cc: [email protected]
---
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
On Sat, 25 Apr 2020 14:49:17 +0900
Masami Hiramatsu <[email protected]> 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 <[email protected]>
> Cc: [email protected]
> ---
> 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
On Sat, 25 Apr 2020 10:00:20 -0400
Steven Rostedt <[email protected]> wrote:
> On Sat, 25 Apr 2020 14:49:17 +0900
> Masami Hiramatsu <[email protected]> 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 <[email protected]>
> > Cc: [email protected]
> > ---
> > 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 <[email protected]>
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 <[email protected]>
> Cc: [email protected]
Thanks for fixing this!
Reviewed-by: Tom Zanussi <[email protected]>
> ---
> 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
>
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 <[email protected]> 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) <[email protected]>
--
Masami Hiramatsu <[email protected]>
On Wed, 20 May 2020 23:22:20 +0900
Masami Hiramatsu <[email protected]> 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
On Wed, 20 May 2020 10:33:46 -0400
Steven Rostedt <[email protected]> wrote:
> On Wed, 20 May 2020 23:22:20 +0900
> Masami Hiramatsu <[email protected]> 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
On Wed, 20 May 2020 10:40:05 -0400
Steven Rostedt <[email protected]> wrote:
> On Wed, 20 May 2020 10:33:46 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Wed, 20 May 2020 23:22:20 +0900
> > Masami Hiramatsu <[email protected]> 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 <[email protected]>