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

Date:   Mon, 2 Oct 2023 10:23:23 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Nick Lowell <nicholas.lowell@...il.com>
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

On Mon, 2 Oct 2023 09:57:34 -0400
Nick Lowell <nicholas.lowell@...il.com> wrote:
> >
> > 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?

Yes.

> 
> 
> > 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.
> 

Correct.

> 
> > > +     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?
> 

Because if it exists, we still want to free it and do the synchronization,
and set it to NULL. In other words, it means we missed something and need
to revert back to the original behavior.

The WARN_ON_ONCE() documents that we never expect that to happen, and if it
does, it means we have a bug.

-- Steve


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