Re: [PATCH] tracing/boottime: Fix kprobe multiple events

[Maximilian Werner]

We are a group of students from Leibniz University Hannover and
this patch is part of a project of ours. That's why both of us
signed this off.

Should we have added Masami to Cc? He didn't appear in the
get_maintainer script.

-- Maximilian & Sascha

On 17.06.20 17:06, Steven Rostedt wrote:
> On Wed, 17 Jun 2020 11:05:21 -0400
> Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> > On Wed, 17 Jun 2020 16:08:17 +0200
> > Sascha Ortmann <sascha.ortmann@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > > Fix boottime kprobe events to add multiple events even if one fails
> > > and report probe generation failures.
> > >
> > > As an example, when we try to set multiprobe kprobe events in
> > > bootconfig like this:
> > >
> > > ftrace.event.kprobes.vfsevents {
> > > 	probes = "vfs_read $arg1 $arg2,,
> > >                   !error! not reported;?", // leads to error
> > > 		 "vfs_write $arg1 $arg2"
> > > }
> > >
> > > this will not work like expected. After commit
> > > da0f1f4167e3af69e1d8b32d6d65195ddd2bfb64 ("tracing/boottime:
> > > Fix kprobe event API usage"), the function
> > > trace_boot_add_kprobe_event will not produce any error message,
> > > aborting the function and stopping subsequent probes from getting
> > > installed when adding a probe fails at kprobe_event_gen_cmd_start.
> > > Furthermore, probes continue when kprobe_event_gen_cmd_end fails
> > > (and kprobe_event_gen_cmd_start did not fail). In this case the
> > > function even returns successfully when the last call to
> > > kprobe_event_gen_cmd_end is successful.
> > >
> > > The behaviour of reporting and aborting after failures is not
> > > consistent.
> > >
> > > The function trace_boot_add_kprobe_event now continues even when
> > > one of the multiple events fails. Each failure is now reported
> > > individually. Since the function can only return one result to the
> > > caller, the function returns now the last failure (or none, if
> > > nothing fails).
> > >
> > > Cc: linux-kernel@xxxxxxxxxxxx
> > > Signed-off-by: Maximilian Werner <maximilian.werner96@xxxxxxxxx>
> > > Signed-off-by: Sascha Ortmann <sascha.ortmann@xxxxxxxxxxxxxxxxxxxx>
> >
> > Why the double signed off by?
> >
> > Masami, I'm fine with this, but needs your review.
>
> [ It appears that Masami wasn't in the Cc ]
>
>
> > -- Steve
> >
> > > ---
> > >   kernel/trace/trace_boot.c | 16 +++++++++++-----
> > >   1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/kernel/trace/trace_boot.c b/kernel/trace/trace_boot.c
> > > index 9de29bb45a27..dbb50184e060 100644
> > > --- a/kernel/trace/trace_boot.c
> > > +++ b/kernel/trace/trace_boot.c
> > > @@ -95,18 +95,24 @@ 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 error = 0;
> > >   	int ret = 0;
> > >   
> > >   	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, val);
> > > -		if (ret)
> > > -			break;
> > > +		error = kprobe_event_gen_cmd_start(&cmd, event, val);
> > > +		if (error) {
> > > +			pr_err("Failed to generate probe: %s\n", buf);
> > > +			ret = error;
> > > +			continue;
> > > +		}
> > >   
> > > -		ret = kprobe_event_gen_cmd_end(&cmd);
> > > -		if (ret)
> > > +		error = kprobe_event_gen_cmd_end(&cmd);
> > > +		if (error) {
> > >   			pr_err("Failed to add probe: %s\n", buf);
> > > +			ret = error;
> > > +		}
> > >   	}
> > >   
> > >   	return ret;