Re: [PATCH v12 11/11] tracing/wprobe: Support BTF struct offset resolution in set_wprobe trigger

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Speculative field parsing for `clear_wprobe` commands unconditionally logs spurious errors and masks genuine syntax/memory errors.

Pre-existing issues:
- [Critical] Mixing `__free` scope-based cleanup with manual async cleanup on the error path causes double free and lockless list corruption.
--

--- Patch [11]: [PATCH v12 11/11] tracing/wprobe: Support BTF struct offset resolution in set_wprobe trigger ---
commit f9bc82719437cd2a74bad3f90ad83888d83a0ed4
Author: Masami Hiramatsu (Google) <[email protected]>

tracing/wprobe: Support BTF struct offset resolution in set_wprobe trigger

Allow set_wprobe trigger to use BTF struct offset resolution to specify
the target address field.

Link: https://lore.kernel.org/all/59637b96946653393a7ad3c7de094094796b39c2.1785067572.git.wangjinchao600@gmail.com/

> diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
> index c64bbdc90a40c..565c106a93805 100644
> --- a/kernel/trace/trace_wprobe.c
> +++ b/kernel/trace/trace_wprobe.c

[ ... ]

> @@ -1013,10 +1190,9 @@ static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops,
>  	 */
>  	struct wprobe_trigger_data *wprobe_data __free(free_wprobe_trigger_data) = NULL;
>  	struct event_trigger_data *trigger_data __free(kfree) = NULL;

[Severity: Critical]
This is a pre-existing issue, but does using __free() here cause a double-free
on the error paths?

If event_trigger_register() fails later in wprobe_trigger_cmd_parse(), it
calls event_trigger_free(trigger_data). This queues the trigger onto the
lockless trigger_data_free_list for asynchronous freeing by a background
kthread.

However, when the error path returns without clearing trigger_data, the
compiler-inserted __free destructors execute synchronously. This immediately
frees both wprobe_data and trigger_data while they are still linked in the
kthread's cleanup list.

When the kthread eventually processes the list, could it traverse freed
memory, trigger a use-after-free, and attempt to free the objects again?

> -	char *event_str, *field_str, *count_str, *comment;
> +	char *event_str, *comment;
>  	struct trace_event_file *wprobe_file;
>  	struct trace_array *tr = file->tr;
> -	struct trace_event_call *event;
>  	bool remove, clear = false;
>  	struct trace_wprobe *tw;
>  	char *param, *filter;

[ ... ]

> @@ -1075,42 +1251,24 @@ static int wprobe_trigger_cmd_parse(struct event_command *cmd_ops,
>  
>  	/* clear_wprobe does not need field. */
>  	if (!clear) {
> -		char *offs;
> +		char *field_str = strsep(&param, ":");
>  
> -		/* Find target field, which must be equivarent to "void *" */
> -		field_str = strsep(&param, ":");
>  		if (!field_str) {
>  			wprobe_trigger_log_err(file, glob, strlen(glob), WPROBE_NEED_FIELD);
>  			return -EINVAL;
>  		}
> -
> -		offs = strpbrk(field_str, "+-");
> -		if (offs) {
> -			long val;
> -
> -			if (kstrtol(offs, 0, &val) < 0) {
> -				wprobe_trigger_log_err(file, glob, offs - glob, BAD_DEREF_OFFS);
> -				return -EINVAL;
> -			}
> -			wprobe_data->adjust = val;
> -			*offs = '\0';
> -		}
> -
> -		event = file->event_call;
> -		field = trace_find_event_field(event, field_str);
> -		if (!field) {
> -			wprobe_trigger_log_err(file, glob, field_str - glob, NO_EVENT_FIELD);
> -			return -ENOENT;
> -		}
> -
> -		if (field->size != sizeof(void *)) {
> -			wprobe_trigger_log_err(file, glob, field_str - glob, WPROBE_BAD_FIELD);
> -			return -ENOEXEC;
> +		ret = wprobe_trigger_field_parse(field_str, file, wprobe_data, glob);
> +		if (ret < 0)
> +			return ret;
> +	} else if (param) {
> +		char *orig_param = param;
> +		char *field_str = strsep(&param, ":");
> +
> +		ret = wprobe_trigger_field_parse(field_str, file, wprobe_data, glob);
> +		if (ret < 0) {
> +			/* field_str was not a field, so it must be count_str */
> +			param = orig_param;
>  		}

[Severity: Medium]
Will this speculative field parsing unconditionally log spurious errors?

When a valid clear_wprobe command is given with a count but no field (e.g.,
clear_wprobe:watch:10), this branch speculatively attempts to parse 10 as a
field string. wprobe_trigger_field_parse() fails and unconditionally calls
wprobe_trigger_log_err(), leaving a bogus NO_EVENT_FIELD error in the trace
error_log:

kernel/trace/trace_wprobe.c:wprobe_trigger_field_parse() {
    ...
	field = trace_find_event_field(file->event_call, field_str);
	if (!field) {
		wprobe_trigger_log_err(file, glob, field_str - glob, NO_EVENT_FIELD);
		return -ENOENT;
	}
    ...
}

Because this code falls back to parsing the parameter as a count upon any
negative return code, the command ultimately succeeds, but the user
receives a false-positive error in the logs.

Additionally, if field parsing encounters a genuine error (like -EINVAL
from a malformed BTF offset or -ENOMEM), will it be swallowed by the fallback
and incorrectly masked?

> -		wprobe_data->offset = field->offset;
> -		wprobe_data->field = kstrdup(field_str, GFP_KERNEL);
> -		if (!wprobe_data->field)
> -			return -ENOMEM;
>  	}
>  
>  	trigger_data = trigger_data_alloc(cmd_ops, cmd, param, wprobe_data);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178611679753.237811.10190793347982785419.stgit@devnote2?part=11