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) [off-list ref]
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.
quoted hunk ↗ jump to hunk
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