Re: [PATCH v3 03/16] tracing/filters: Fix false success return in process_system_preds()

[Masami Hiramatsu (Google)]

On Wed, 28 Oct 2026 09:28:46 -0400, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Wed, 28 Oct 2026 09:43:53 +0900
> "Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > 
> > In process_system_preds(), "fail" is initialized to true to indicate
> > whether any event in the subsystem successfully applied the filter.
> > However, "fail = false;" was placed at the end of the event loop outside
> > the error check, unconditionally clearing "fail" on every iteration even
> > when process_preds() returned an error.
> > 
> > As a result, if every event in the subsystem fails to match the filter
> > (e.g., an invalid field), process_system_preds() returns 0 (success)
> > instead of -EINVAL. Consequently, create_system_filter() considers the
> > operation successful and silently resets the subsystem filter display to
> > the default global filter message instead of reporting the parse error.
> > 
> > Move "fail = false;" into the else branch of the error check so that
> > "fail" is only cleared when at least one event filter succeeds. Because
> > file->filter is still replaced even when process_preds() fails, check
> > !list_empty(&filter_list->list) on the fail and fail_mem paths so any
> > replaced old filters are freed after an RCU grace period via
> > delay_free_filter().
> > 
> > Fixes: 80765597bc58 ("tracing: Rewrite filter logic to be simpler and faster")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> > ---
> >  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 f7c067a58ba4..617bc3c0bb0c 100644
> > --- a/kernel/trace/trace_events_filter.c
> > +++ b/kernel/trace/trace_events_filter.c
> > @@ -2354,8 +2354,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);
> > @@ -2370,8 +2372,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)
> > @@ -2386,14 +2386,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);
> 
> I still prefer my suggestion which cleans up the code and gets rid of the
> duplicate if statement for freeing.
> 
>   https://lore.kernel.org/all/20261026153258.5259323d@xxxxxxxxxxxxxxxxxx/

Ah, indeed. Sorry I missed it. Let me update it.

Thanks!

> 
> -- Steve
>

-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>