Re: [PATCH 02/10] tracing/filters: Fix false success return in process_system_preds()

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 25 Oct 2026 01:25:42 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_events_filter.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 18ce3ec00d34..c2c35b4de85e 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -2332,8 +2332,10 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
>  			filter_disable(file);
>  			parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
>  			append_filter_err(tr, pe, filter);
> -		} else
> +		} else {
>  			event_set_filtered_flag(file);
> +			fail = false;
> +		}
>  
>  
>  		filter_item = kzalloc_obj(*filter_item);
> @@ -2348,8 +2350,6 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
>  		filter_item->filter = event_filter(file);
>  		event_set_filter(file, filter);
>  		filter = NULL;
> -
> -		fail = false;
>  	}
>  
>  	if (fail)
> @@ -2364,14 +2364,17 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
>  	return 0;
>   fail:
>  	/* No call succeeded */
> -	free_filter_list(filter_list);
> +	if (!list_empty(&filter_list->list))
> +		delay_free_filter(filter_list);
> +	else
> +		free_filter_list(filter_list);
>  	parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
>  	return -EINVAL;
>   fail_mem:
>  	__free_filter(filter);
>  
> -	/* If any call succeeded, we still need to sync */
> -	if (!fail)
> +	/* If any filter was replaced, we still need to sync */
> +	if (!list_empty(&filter_list->list))
>  		delay_free_filter(filter_list);
>  	else
>  		free_filter_list(filter_list);

Hmm, I think this can be cleaned up a bit better.

 1) remove the fail boolean
 2) consolidate the if (!list_empty()) logic.

Like this:

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 5e74d2dae40a..4d763c55bb7b 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -2305,7 +2305,7 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	struct filter_list *filter_item;
 	struct event_filter *filter = NULL;
 	struct filter_head *filter_list;
-	bool fail = true;
+	int ret = -ENOMEM;
 	int err;
 
 	filter_list = kmalloc_obj(*filter_list);
@@ -2332,8 +2332,10 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 			filter_disable(file);
 			parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
 			append_filter_err(tr, pe, filter);
-		} else
+		} else {
 			event_set_filtered_flag(file);
+			ret = 0;
+		}
 
 
 		filter_item = kzalloc_obj(*filter_item);
@@ -2348,11 +2350,9 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 		filter_item->filter = event_filter(file);
 		event_set_filter(file, filter);
 		filter = NULL;
-
-		fail = false;
 	}
 
-	if (fail)
+	if (ret)
 		goto fail;
 
 	/*
@@ -2364,19 +2364,18 @@ static int process_system_preds(struct trace_subsystem_dir *dir,
 	return 0;
  fail:
 	/* No call succeeded */
-	free_filter_list(filter_list);
 	parse_error(pe, FILT_ERR_BAD_SUBSYS_FILTER, 0);
-	return -EINVAL;
+	ret = -EINVAL;
  fail_mem:
 	__free_filter(filter);
 
-	/* If any call succeeded, we still need to sync */
-	if (!fail)
+	/* If any filter was replaced, we still need to sync */
+	if (!list_empty(&filter_list->list))
 		delay_free_filter(filter_list);
 	else
 		free_filter_list(filter_list);
 
-	return -ENOMEM;
+	return ret;
 }
 
 static int create_filter_start(char *filter_string, bool set_str,


-- Steve