linux-kernel - Re: [PATCH v2] 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]

Date:   Wed, 22 Nov 2023 16:20:51 -0500
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 v2] trace: tracing_event_filter: fast path when no
 subsystem filters

On Thu, 12 Oct 2023 16:37:35 -0400
Nick Lowell <nicholas.lowell@...il.com> wrote:

Sorry, I was traveling when this was sent, and I missed it.


> I really appreciate the continued feedback.  I was able to reproduce.
> I think I'm understanding better but still need some help.
> I am actually wondering if remove_filter_string(system->filter) should

You mean to return true if filter->filter_string was not NULL?

> also return bool as an OR'd input for sync.
> Should it be something like this?
> 
>         if (!strcmp(strstrip(filter_string), "0")) {
> -               filter_free_subsystem_preds(dir, tr);
> -               remove_filter_string(system->filter);
> +              bool sync;

I would just make this an int;

> +
> +              sync = filter_free_subsystem_preds(dir, tr);
> +              sync = sync || remove_filter_string(system->filter);

And then just have:

		sync |= remove_filter_string(system->filter);

>                 filter = system->filter;
>                 system->filter = NULL;
> -               /* Ensure all filters are no longer used */
> -               tracepoint_synchronize_unregister();
> +              /* If nothing was freed, we do not need to sync */
> +              if(sync) {
> +                      /* Ensure all filters are no longer used */
> +                      tracepoint_synchronize_unregister();
> +              }
>                 filter_free_subsystem_filters(dir, tr);
>                 __free_filter(filter);
>                 goto out_unlock;
> 
> > Maybe even pass in "sync" to the filter_free_subsystem_filters() to make
> > sure there were nothing to be freed, and do the WARN_ON_ONCE() then.
> >
> >                 __free_filter(filter);
> >                 goto out_unlock;
> >         }
> >
> > -- Steve  
> 
> I'm not sure if I see the reasoning for the WARN_ON_ONCE() in
> filter_free_subsystem_filters()
> because it ends up checking the same if(!filter) just like
> filter_free_subsystem_preds() did earlier. It doesn't
> seem to do anything with system->filter.  I actually wonder if !sync,
> could filter_free_subsystem_filters()
> be skipped altogether.  Help me if I'm missing something.

The point is, code always changes. It's a bug if one of the filters had
content in filter_free_subsystem_filters() and sync is 0, hence the
WARN_ON_ONCE() if it does.

WARN_ON*()s are added to make sure the code is acting the way it is expected
to act. Yes, it should never trigger, but if it does, we know there's a bug
somewhere.

-- Steve