Re: [PATCH v7 05/10] tracing: wprobe: Use a new seq_print_ip_sym_offset() wrapper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Symbol offsets are discarded in wprobe definitions when displayed via tracefs, preventing round-tripping of event specifications.
- [High] Failure to register hardware breakpoints leaves the trace_probe in a corrupted, pseudo-enabled state, causing future enablement attempts to silently fail.
- [High] The dynamically allocated `symbol` string is permanently leaked every time a wprobe with a symbol is created.
- [Medium] Meaningful code and commit message mismatch regarding the use of a new wrapper function.
--

--- Patch [5]: [PATCH v7 05/10] tracing: wprobe: Use a new seq_print_ip_sym_offset() wrapper ---
commit f3a4507e611a20fcf1578dc82ce81258e7452d39
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

tracing: wprobe: Use a new seq_print_ip_sym_offset() wrapper

The commit message states it refactors code to use a new seq_print_ip_sym_offset()
wrapper. However, the commit only adds an include statement, missing the 
expected implementation changes.

Link: https://lore.kernel.org/all/176226550596.59499.18020648957674458755.stgit@devnote2/

> tracing: wprobe: Use a new seq_print_ip_sym_offset() wrapper
> 
> Use a new seq_print_ip_sym_offset() wrapper function instead of
> using TRACE_ITER(SYM_OFFSET) mask directly.

[Severity: Medium]
Is this commit missing the actual implementation changes? The commit
message claims to refactor code to use a new wrapper function, but the
diff only adds an include directive.

> diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
> index b52f3eac719f9..dd310a87b3333 100644
> --- a/kernel/trace/trace_wprobe.c
> +++ b/kernel/trace/trace_wprobe.c
> @@ -20,6 +20,7 @@
>  #include <asm/ptrace.h>
>  
>  #include "trace_dynevent.h"
> +#include "trace_output.h"
>  #include "trace_probe.h"
>  #include "trace_probe_kernel.h"
>  #include "trace_probe_tmpl.h"

I also noticed a few other concerns in this file:

[Severity: High]
Does parse_address_spec() drop the offset from the base symbol?

kernel/trace/trace_wprobe.c:parse_address_spec() {
    ...
		if (off_str) {
			if (kstrtoint(off_str, 0, &offset) != 0) {
				trace_probe_log_err(off_str - _spec, BAD_PROBE_ADDR);
				return -EINVAL;
			}
			*off_str = '\0';
    ...
}

If the offset string is stripped and discarded here, it causes the
offset to be dropped from the probe's symbol representation. 

When trace_wprobe_show() emits the symbol, it is truncated:

kernel/trace/trace_wprobe.c:trace_wprobe_show() {
    ...
	if (tw->symbol)
		seq_printf(m, " %c@%s:%d", type_char, tw->symbol, len);
    ...
}

If a user reads the event definition via tracefs, it will display the
probe as simply w@jiffies instead of w@jiffies+10. If this definition
is saved and loaded back, it attaches to the base symbol instead of
the offset address, leading to incorrect behavior.

[Severity: High]
Does enable_trace_wprobe() fail to handle rollback properly on error?

kernel/trace/trace_wprobe.c:enable_trace_wprobe() {
    ...
	if (!enabled) {
		list_for_each_entry(tw, trace_probe_probe_list(tp), tp.list) {
			ret = __register_trace_wprobe(tw);
			if (ret < 0) {
				/* TODO: rollback */
				return ret;
			}
		}
	}
    ...
}

If __register_trace_wprobe() fails, it returns an error immediately
without clearing the flags set earlier (TP_FLAG_PROFILE or file links). 

Could this missing rollback leave the trace_probe in an incorrectly
enabled state, causing future enablement attempts to silently fail
because trace_probe_is_enabled() will return true?

[Severity: High]
Does this code leak the symbol string?

kernel/trace/trace_wprobe.c:parse_address_spec() {
    ...
		_addr += offset;
		*symbol = kstrdup(at + 1, GFP_KERNEL);
		if (!*symbol)
			return -ENOMEM;
    ...
}

The string pointed to by symbol is allocated here. Then
__trace_wprobe_create() passes it to alloc_trace_wprobe():

kernel/trace/trace_wprobe.c:__trace_wprobe_create() {
    ...
	ret = parse_address_spec(argv[1], &addr, &type, &len, &symbol);
	if (ret < 0)
		return ret;
    ...
	tw = alloc_trace_wprobe(group, event, symbol, addr, len, type, argc);
    ...
}

But alloc_trace_wprobe() duplicates the string again:

kernel/trace/trace_wprobe.c:alloc_trace_wprobe() {
    ...
	if (symbol) {
		tw->symbol = kstrdup(symbol, GFP_KERNEL);
		if (!tw->symbol)
			return ERR_PTR(-ENOMEM);
	}
    ...
}

The original symbol pointer in __trace_wprobe_create() is never freed
before the function returns on either success or error paths, which
might leak the string memory on every wprobe creation.

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