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]/ -- Steve
