linux-kernel - Re: [PATCH v6 1/3] tracing: eprobe: remove duplicate is_good

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3f1b974be23b058cc0f004b518df21716b9dfcfd.camel@kernel.org>
Date:   Sat, 25 Jun 2022 12:25:50 -0500
From:   Tom Zanussi <zanussi@...nel.org>
To:     Linyu Yuan <quic_linyyuan@...cinc.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Masami Hiramatsu <mhiramat@...nel.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/3] tracing: eprobe: remove duplicate is_good_name()
 operation

Hi Linyu,

On Tue, 2022-06-21 at 09:59 +0800, Linyu Yuan wrote:
> traceprobe_parse_event_name() already validate SYSTEM and EVENT name,
> there is no need to call is_good_name() after it.
> 
> Add trace_probe_log_set_index(1) to allow report correct error
> if user input wrong SYSTEM.EVENT format.
> 
> Acked-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
> Signed-off-by: Linyu Yuan <quic_linyyuan@...cinc.com>
> ---
> v2: drop v1 change as it is NACK.
>     add it to remove duplicate is_good_name().
> v3: move it as first patch.
> v4: no change
> v5: add Acked-by tag
> v6: keep is_good_name() check for group and event name
>     add trace_probe_log_set_index(1) to report correct error message.
> 
>  kernel/trace/trace_eprobe.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_eprobe.c
> b/kernel/trace/trace_eprobe.c
> index 7d44785..8979cb9e 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -881,13 +881,12 @@ static int __trace_eprobe_create(int argc,
> const char *argv[])
>         if (!is_good_name(event) || !is_good_name(group))
>                 goto parse_error;
>  
> +       trace_probe_log_set_index(1);

Is this something that you noticed missing in the original code and are
adding now?  If so, please make this a separate patch.  Or is this
something that's needed for the new 'generating event name' code?  If
that's the case, please move this to the other patch.

This one should only contain the code related to the duplicate
is_good_name() removal mentioned in the subject.  Or if this really
does belong here, please provide more explanation of why it's needed if
you remove the duplicate is_good_name() code.

Thanks,

Tom

>         sys_event = argv[1];
>         ret = traceprobe_parse_event_name(&sys_event, &sys_name,
> buf2,
>                                           sys_event - argv[1]);
>         if (ret || !sys_name)
>                 goto parse_error;
> -       if (!is_good_name(sys_event) || !is_good_name(sys_name))
> -               goto parse_error;
>  
>         mutex_lock(&event_mutex);
>         event_call = find_and_get_event(sys_name, sys_event);