Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] tracing/filters: process_system_preds() returns 0 on ENOMEM after match --
commit a15564919ee696f8c1b9df5aeee0a37935f51db7 Author: Masami Hiramatsu (Google) <[email protected]> tracing/filters: Fix false success return in process_system_preds() This commit replaces the boolean fail flag in process_system_preds() with ret so that process_system_preds() returns -EINVAL when no event in the subsystem matches the filter. It also consolidates the fail and fail_mem error cleanup paths. > diff --git a/kernel/trace/trace_events_filter.c > b/kernel/trace/trace_events_filter.c > index 28977d10e60b6..b35ecb01697b8 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -2337,7 +2337,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); > @@ -2364,8 +2364,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); > @@ -2380,11 +2382,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; > > /* > @@ -2396,19 +2396,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; [Severity: Medium] Can process_system_preds() return 0 instead of -ENOMEM when an allocation fails after at least one event matches the filter? In kernel/trace/trace_events_filter.c:process_system_preds(), ret is initialized to -ENOMEM and cleared to 0 as soon as process_preds() succeeds for an event: err = process_preds(file->event_call, filter_string, filter, pe); if (err) { ... } else { event_set_filtered_flag(file); ret = 0; } filter_item = kzalloc_obj(*filter_item); if (!filter_item) goto fail_mem; If kzalloc_obj(*filter_item) fails on that same iteration, or if kzalloc_obj(*filter), kstrdup(), or kzalloc_obj(*filter_item) fails on a subsequent event in the loop, jumping to fail_mem returns ret (which is now 0) instead of -ENOMEM. That causes create_system_filter() and apply_subsystem_event_filter() to treat the aborted update as a success, leaving the remaining events in the subsystem without the new filter applied. Also, if kzalloc_obj(*filter_item) fails right after event_set_filtered_flag(file) runs, EVENT_FILE_FL_FILTERED is set and trace_buffered_event_enable() is called, while fail_mem frees the new filter without installing it via event_set_filter(file, filter). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/179323784538.740114.15323117974242744566.stgit@devnote2?part=2
