Re: [PATCH v4 02/15] tracing/filters: Fix false success return in process_system_preds()

[sashiko-bot@kernel.org]

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) <mhiramat@kernel.org>

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