[PATCH] tracing: react to error return from traceprobe_parse_event_name()

[PATCH] tracing: react to error return from traceprobe_parse_event_name()

[Lukas Bulwahn]

The function traceprobe_parse_event_name() may set the first two function
arguments to a non-null value and still return -EINVAL to indicate an
unsuccessful completion of the function. Hence, it is not sufficient to
just check the result of the two function arguments for being not null,
but the return value also needs to be checked.

Commit 95c104c378dc ("tracing: Auto generate event name when creating a
group of events") changed the error-return-value checking of the second
traceprobe_parse_event_name() invocation in __trace_eprobe_create() and
removed checking the return value to jump to the error handling case.

Reinstate using the return value in the error-return-value checking.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
 kernel/trace/trace_eprobe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
index 4a0e9d927443..460d3ec8a256 100644
--- a/kernel/trace/trace_eprobe.c
+++ b/kernel/trace/trace_eprobe.c
@@ -883,7 +883,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
 	trace_probe_log_set_index(1);
 	sys_event = argv[1];
 	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
-	if (!sys_event || !sys_name) {
+	if (!ret || !sys_event || !sys_name) {
 		trace_probe_log_err(0, NO_EVENT_INFO);
 		goto parse_error;
 	}
-- 
2.17.1

Re: [PATCH] tracing: react to error return from traceprobe_parse_event_name()

[Linyu Yuan]

hi Lukas,

On 8/10/2022 11:09 PM, Lukas Bulwahn wrote:
> The function traceprobe_parse_event_name() may set the first two function
> arguments to a non-null value and still return -EINVAL to indicate an
> unsuccessful completion of the function. Hence, it is not sufficient to
> just check the result of the two function arguments for being not null,
> but the return value also needs to be checked.
>
> Commit 95c104c378dc ("tracing: Auto generate event name when creating a
> group of events") changed the error-return-value checking of the second
> traceprobe_parse_event_name() invocation in __trace_eprobe_create() and
> removed checking the return value to jump to the error handling case.
>
> Reinstate using the return value in the error-return-value checking.
>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
>   kernel/trace/trace_eprobe.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index 4a0e9d927443..460d3ec8a256 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -883,7 +883,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
>   	trace_probe_log_set_index(1);
>   	sys_event = argv[1];
>   	ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
> -	if (!sys_event || !sys_name) {
> +	if (!ret || !sys_event || !sys_name) {

that's right, miss case sys_event and sys_name are not null, but invalid.

it should be  if (ret || ...)  ?


>   		trace_probe_log_err(0, NO_EVENT_INFO);
>   		goto parse_error;
>   	}

Re: [PATCH] tracing: react to error return from traceprobe_parse_event_name()

[Lukas Bulwahn]

On Thu, Aug 11, 2022 at 1:29 AM Linyu Yuan <quic_linyyuan@quicinc.com> wrote:
>
> hi Lukas,
>
> On 8/10/2022 11:09 PM, Lukas Bulwahn wrote:
> > The function traceprobe_parse_event_name() may set the first two function
> > arguments to a non-null value and still return -EINVAL to indicate an
> > unsuccessful completion of the function. Hence, it is not sufficient to
> > just check the result of the two function arguments for being not null,
> > but the return value also needs to be checked.
> >
> > Commit 95c104c378dc ("tracing: Auto generate event name when creating a
> > group of events") changed the error-return-value checking of the second
> > traceprobe_parse_event_name() invocation in __trace_eprobe_create() and
> > removed checking the return value to jump to the error handling case.
> >
> > Reinstate using the return value in the error-return-value checking.
> >
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> >   kernel/trace/trace_eprobe.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> > index 4a0e9d927443..460d3ec8a256 100644
> > --- a/kernel/trace/trace_eprobe.c
> > +++ b/kernel/trace/trace_eprobe.c
> > @@ -883,7 +883,7 @@ static int __trace_eprobe_create(int argc, const char *argv[])
> >       trace_probe_log_set_index(1);
> >       sys_event = argv[1];
> >       ret = traceprobe_parse_event_name(&sys_event, &sys_name, buf2, 0);
> > -     if (!sys_event || !sys_name) {
> > +     if (!ret || !sys_event || !sys_name) {
>
> that's right, miss case sys_event and sys_name are not null, but invalid.
>
> it should be  if (ret || ...)  ?
>

Linyu, you are right. The visual symmetry tricked me into believing
the line above was right, but it is clearly wrong if you think about
it and look at the other invocation.

I send out a corrected patch v2:

https://lore.kernel.org/all/20220811071734.20700-1-lukas.bulwahn@gmail.com/

Please review, ack and pick the patch v2. Thanks.

Lukas