Re: [PATCH v7 02/10] x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Memory leak of the `symbol` string in `__trace_wprobe_create()`.
- [Medium] `enable_trace_wprobe()` fails to rollback trace probe state on hardware breakpoint registration error, leading to state inconsistency and silent tracing failures.
--

--- Patch [2]: [PATCH v7 02/10] x86: hw_breakpoint: Add a kconfig to clarify when a breakpoint fires ---
commit f8a7aeb231575f15f9227ab411134bde7408e480
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

tracing: wprobe: Add watchpoint probe event based on hardware breakpoint

Add a new probe event for the hardware breakpoint called wprobe-event.
This wprobe allows user to trace (watch) the memory access at the
specified memory address. The events can be defined via the dynamic_events
interface file.

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

> diff --git a/kernel/trace/trace_wprobe.c b/kernel/trace/trace_wprobe.c
> new file mode 100644
> index 0000000000000..b52f3eac719f9
> --- /dev/null
> +++ b/kernel/trace/trace_wprobe.c
> @@ -0,0 +1,692 @@

[ ... ]

> +static int enable_trace_wprobe(struct trace_event_call *call,
> +			       struct trace_event_file *file)
> +{
> +	struct trace_probe *tp;
> +	struct trace_wprobe *tw;
> +	bool enabled;
> +	int ret = 0;
> +
> +	tp = trace_probe_primary_from_call(call);
> +	if (WARN_ON_ONCE(!tp))
> +		return -ENODEV;
> +	enabled = trace_probe_is_enabled(tp);
> +
> +	if (file) {
> +		ret = trace_probe_add_file(tp, file);
> +		if (ret)
> +			return ret;
> +	} else {
> +		trace_probe_set_flag(tp, TP_FLAG_PROFILE);
> +	}
> +
> +	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;

[Severity: Medium]
Will this leave the trace probe state inconsistent if hardware breakpoint
registration fails?

Earlier in enable_trace_wprobe(), trace_probe_add_file() or
trace_probe_set_flag() modify the probe state. If __register_trace_wprobe()
returns an error, the function returns immediately without rolling back those
changes.

Could this cause subsequent enable attempts to skip registration entirely and
silently fail to trace?

> +			}
> +		}
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int __trace_wprobe_create(int argc, const char *argv[])
> +{
> +	/*
> +	 * Argument syntax:
> +	 *  b[:[GRP/][EVENT]] SPEC
> +	 *
> +	 * SPEC:
> +	 *  [r|w|rw]@[ADDR|SYMBOL[+OFFS]][:LEN]
> +	 */
> +	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
> +	struct trace_wprobe *tw __free(free_trace_wprobe) = NULL;
> +	const char *event = NULL, *group = WPROBE_EVENT_SYSTEM;
> +	const char *tplog __free(trace_probe_log_clear) = NULL;
> +	char *symbol = NULL;
> +	unsigned long addr;
> +	int len, type, i;
> +	int ret = 0;
> +
> +	if (argv[0][0] != 'w')
> +		return -ECANCELED;
> +
> +	if (argc < 2)
> +		return -EINVAL;
> +
> +	tplog = trace_probe_log_init("wprobe", argc, argv);
> +
> +	if (argv[0][1] != '\0') {
> +		if (argv[0][1] != ':') {
> +			trace_probe_log_set_index(0);
> +			trace_probe_log_err(1, BAD_MAXACT_TYPE);
> +			/* Invalid format */
> +			return -EINVAL;
> +		}
> +		event = &argv[0][2];
> +	}
> +
> +	trace_probe_log_set_index(1);
> +	ret = parse_address_spec(argv[1], &addr, &type, &len, &symbol);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!event)
> +		event = symbol ? symbol : "wprobe";
> +
> +	argc -= 2; argv += 2;
> +	tw = alloc_trace_wprobe(group, event, symbol, addr, len, type, argc);
> +	if (IS_ERR(tw))
> +		return PTR_ERR(tw);

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

When a wprobe event is created using a symbol name, parse_address_spec()
allocates a string for symbol. This string is then passed to
alloc_trace_wprobe() which duplicates it again using kstrdup().

However, the original symbol pointer does not appear to be freed before
__trace_wprobe_create() returns. Should symbol be annotated with __free(kfree)
or explicitly freed?

> +
> +	ctx = kzalloc_obj(*ctx);
> +	if (!ctx)
> +		return -ENOMEM;

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