linux-kernel - Re: [PATCH] trace: tracing_event_filter: fast path when no subsystem filters

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: <CAFEqNJ0kskvj+hipFcqdhkzaFrA+bNtGX2Yk29yJoQ54_t7aVg@mail.gmail.com>
Date:   Mon, 2 Oct 2023 10:01:44 -0400
From:   Nick Lowell <nicholas.lowell@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     mhiramat@...nel.org, linux-kernel@...r.kernel.org,
        linux-trace-kernel@...r.kernel.org,
        Nicholas Lowell <nlowell@...mark.com>
Subject: Re: [PATCH] trace: tracing_event_filter: fast path when no subsystem filters

Sending again in plain text mode.
Thanks for the great feedback!  Hopefully my inline comments/questions
aren't garbled.

On Sat, Sep 30, 2023 at 4:04 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Tue, 26 Sep 2023 10:20:58 -0400
> Nicholas Lowell <nicholas.lowell@...il.com> wrote:
>
> > From: Nicholas Lowell <nlowell@...mark.com>
> >
> > If there are no filters in the event subsystem, then there's no
> > reason to continue and hit the potentially time consuming
> > tracepoint_synchronize_unregister function.  This should give
> > a speed up for initial disabling/configuring
> >
> > Signed-off-by: Nicholas Lowell <nlowell@...mark.com>
> > ---
> >  kernel/trace/trace_events_filter.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > index 33264e510d16..93653d37a132 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -1317,22 +1317,29 @@ void free_event_filter(struct event_filter *filter)
> >       __free_filter(filter);
> >  }
> >
> > -static inline void __remove_filter(struct trace_event_file *file)
> > +static inline int __remove_filter(struct trace_event_file *file)
> >  {
> >       filter_disable(file);
> > -     remove_filter_string(file->filter);
> > +     if (file->filter)
> > +             remove_filter_string(file->filter);
> > +     else
> > +             return 0;
> > +
> > +     return 1;
>
> The above looks awkward. What about:
>
>         if (!file->filter)
>                 return 0;
>
>         remove_filter_string(file->filter);
>         return 1;
>
> ?
>
> Or better yet:
>
>         if (!file->filter)
>                 return false;
>
>         remove_filter_string(file->filter);
>         return true;
>

Is it safe to assume you would like the function's return type to
change from int to bool if I go with option 2?

> and ...
>
> >  }
> >
> > -static void filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
> > +static int filter_free_subsystem_preds(struct trace_subsystem_dir *dir,
> >                                       struct trace_array *tr)
> >  {
> >       struct trace_event_file *file;
> > +     int i = 0;
>
> We don't really need a counter. It's either do the synchronization or
> we don't.
>
>         bool do_sync = false;
>
> >
> >       list_for_each_entry(file, &tr->events, list) {
> >               if (file->system != dir)
> >                       continue;
> > -             __remove_filter(file);
> > +             i += __remove_filter(file);
>
>                 if (remove_filter(file))
>                         do_sync = true;
>
> >       }
>
>         return do_sync;
>

Going to assume the same here--that return type should change from int to bool.

> > +     return i;
> >  }
> >
> >  static inline void __free_subsystem_filter(struct trace_event_file *file)
> > @@ -2411,7 +2418,9 @@ int apply_subsystem_event_filter(struct trace_subsystem_dir *dir,
> >       }
> >
> >       if (!strcmp(strstrip(filter_string), "0")) {
> > -             filter_free_subsystem_preds(dir, tr);
> > +             if (filter_free_subsystem_preds(dir, tr) == 0)
> > +                     goto out_unlock;
> > +
>
>                 /* If nothing was freed, we do not need to sync */
>                 if (!filter_free_subsystem_preds(dir, tr))
>                         goto out_unlock;
>
> And yes, add the comment.
>
> And actually, in that block with the goto out_unlock, we should have:
>
>                 if (!filter_free_subsystem_preds(dir, tr)) {
>                         if (!(WARN_ON_ONCE(system->filter))
>                                 goto out_unlock;
>                 }
>

Can you explain why the WARN_ON_ONCE should be in a conditional?
Don't we still want the original conditional to cause the goto regardless?

                if (!filter_free_subsystem_preds(dir, tr)) {
                        WARN_ON_ONCE(system->filter);
                        goto out_unlock;
                }

> If there were no preds, ideally there would be no subsystem filter. But
> if that's not the case, we need to warn about that and then continue.
>
> -- Steve
>
> >               remove_filter_string(system->filter);
> >               filter = system->filter;
> >               system->filter = NULL;
>