On Wed, 28 Oct 2026 09:28:46 -0400, Steven Rostedt <[email protected]> wrote: > On Wed, 28 Oct 2026 09:43:53 +0900 > "Masami Hiramatsu (Google)" <[email protected]> wrote: > > > From: Masami Hiramatsu (Google) <[email protected]> > > > > 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: [email protected] > > Signed-off-by: Masami Hiramatsu (Google) <[email protected]> > > --- > > 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/[email protected]/
Ah, indeed. Sorry I missed it. Let me update it. Thanks! > > -- Steve > -- Masami Hiramatsu (Google) <[email protected]>
