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

Date:   Sat, 30 Sep 2023 04:03:57 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Nicholas 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 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;

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;

> +	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;
		}

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;