linux-kernel - Re: [RFC PATCH] tracing: fix return value in __ftrace_event_enable_disable for TRACE_REG

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] [day] [month] [year] [list]

Message-ID: <CA+wEVJbraZfyF1-4J41maJHtBYUAZBNWMUA9z+PWPXYeR5jumw@mail.gmail.com>
Date: Wed, 19 Mar 2025 17:21:14 +0100
From: Gabriele Paoloni <gpaoloni@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>, mathieu.desnoyers@...icios.com, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] tracing: fix return value in __ftrace_event_enable_disable
 for TRACE_REG_UNREGISTER

On Wed, Mar 19, 2025 at 10:13 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Tue, 18 Mar 2025 11:07:00 +0900
> Masami Hiramatsu (Google) <mhiramat@...nel.org> wrote:
>
> > > --- a/kernel/trace/trace_events.c
> > > +++ b/kernel/trace/trace_events.c
> > > @@ -790,7 +790,7 @@ static int __ftrace_event_enable_disable(struct trace_event_file *file,
> > >                             clear_bit(EVENT_FILE_FL_RECORDED_TGID_BIT, &file->flags);
> > >                     }
> > >
> > > -                   call->class->reg(call, TRACE_REG_UNREGISTER, file);
> > > +                   ret = call->class->reg(call, TRACE_REG_UNREGISTER, file);
> >
> > This is not enough. As same as enable failure, this function needs to handle
> > this error to report it and break.
>
> Perhaps all we should do here is:
>
>                         WARN_ON_ONCE(ret);

Yes I was trying to address the request from Masami however,
if we look at the error path of TRACE_REG_REGISTER what happens is:

ret = call->class->reg(call, TRACE_REG_REGISTER, file);
if (ret) {
   if (cmd)
       tracing_stop_cmdline_record();
    if (tgid)
       tracing_stop_tgid_record();
    pr_info("event trace: Could not enable event "
       "%s\n", trace_event_name(call));
    break;

In this case it makes sense since it will undo what was done before
enabling the trace event. However I don't think it would make sense
to invoke tracing_start_cmdline_record and/or tracing_start_tgid_record
if the unregister callbacks fails since the event could be in a sort of
unknown state (i.e. we cannot trust the event to still be enabled).

Also it is probably not appropriate to log the event using exclusively

         pr_info("event trace: Could not disable event "
                 "%s\n", trace_event_name(call));

So I would agree with Steven or else

      if (ret)
           pr_warn("event trace: Could not disable event "
                   "%s\n", trace_event_name(call));

Many thanks for the review
Gab
>
> -- Steve
>