Message-Id: <20250902112147.165c8030837b6b21cf402fd3@kernel.org>
Date: Tue, 2 Sep 2025 11:21:47 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Ryan Chung <seokwoo.chung130@...il.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, Mathieu Desnoyers
 <mathieu.desnoyers@...icios.com>, linux-trace-kernel@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] trace_fprobe.c: TODO: handle filter, nofilter or
 symbol list

Hi Ryan,

Thanks for update.

On Fri, 29 Aug 2025 19:20:50 +0900
Ryan Chung <seokwoo.chung130@...il.com> wrote:

> This v2 addresses the TODO in trace_fprobe to handle comma-separated
> symbol lists and the '!' prefix. Tokens starting with '!' are collected
> as "nofilter", and the others as "filter", then passed to
> register_fprobe() accordingly. Empty tokens are rejected and errors are
> reported with trace_probe_log_err().

OK, can you describe how it changes the syntax. You may find more things
to write it down.

For example, fprobe is not only a function entry, but also it supports
function return. How it is specified? Current "%return" suffix is introduced
for single symbol (function), like schedule%return. If we introduce a list
of symbols and filters, it looks more complicated.

For example, "!funcAB,funcC,funcA*%return" seems like the exit of funcA*,
the entry of funcC, but not covers funcAB. It is naturally misleading
users. We have to check "funcA*%return,!funcAB,funcC" pattern.

Thus, I think we should use another suffix, like ":exit" (I think the colon
does strongly separate than comma, what do you think?), or just
prohibit to use "%return" but user needs to specify "$retval" in fetcharg
to specify it is the fprobe on function exit. (this maybe more natural)

The reason why I talked about how to specify the exit point of a function
so far, is because the variables that can be accessed at the entrance
and exit are different.

Also, fprobe supports event-name autogeneration from the symbol name,
e.g. 'symbol__entry' or 'symbol__exit'. Recently I found the symbol
already supports wildcards, so sanitize it from the event name [1]

[1] https://lore.kernel.org/all/175535345114.282990.12294108192847938710.stgit@devnote2/

To use this list-style filters, we may need to reject if there is no
event name. Of cause we can generate event-name from the symbol list
but you need to sanitize non alphabet-number characters.

Ah, here is another one, symbol is used for ctx->funcname, and this is
used for querying BTF. But obviously BTF context is unusable for this
case. So we should set the ctx->funcname = NULL with listed filter.

> 
> Questions for maintainers (to confirm my understanding):
>   * Parsing location: Masami suggested doing the parsing in the parse
>     stage (e.g., via parse_symbol_and_return()). v2 keeps the logic in
>     __register_trace_fprobe(), but I can move the call into the parsing
>     path in v3 if that is the preferred place. Is that correct?

Most of above processes have been done in parse_symbol_and_return(),
thus the parsing it should be done around there.

>   * Documentation: I plan to update the user-facing docs for fprobe
>     syntax. Is Documentation/trace/ the right place (e.g., 
>     Documentation/trace/fprobetrace.rst)?

Yes, please explain it with examples.

Also, can you add a testcase (in a sparate patch) for this syntax?)

Thank you,

> 
> Link: https://lore.kernel.org/linux-trace-kernel/20250812162101.5981-1-seokwoo.chung130@gmail.com/
> Signed-off-by: Ryan Chung <seokwoo.chung130@...il.com>

> ---
> 
> Changes in v2:
>   * Classify '!' tokens as nofilter, others as filter; pass both to
>     register_fprobe().
>   * Reject empty tokens; log errors with trace_probe_log_*().
>   * Use __free(kfree) for temporary buffers.
>   * Keep subject and style per "Submitting patches" (tabs, wrapping).
>   * No manual dedup (leave to ftrace).
> 
>  kernel/trace/trace_fprobe.c | 48 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index b36ade43d4b3..d731d9754a39 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -815,6 +815,11 @@ static int trace_fprobe_verify_target(struct trace_fprobe *tf)
>  static int __register_trace_fprobe(struct trace_fprobe *tf)

This is not a good place to add anymore, because this change turned out
not to meet the expected prerequisites. (when I commented TODO here,
I didn't expected too.)

Anyway, this is a good opportunity to review this TODO deeper.

Thank you,

>  {
>  	int i, ret;
> +	const char *p, *q;
> +	size_t spec_len, flen = 0, nflen = 0, tlen;
> +	bool have_f = false, have_nf = false;
> +	char *filter __free(kfree) = NULL;
> +	char *nofilter __free(kfree) = NULL;
>  
>  	/* Should we need new LOCKDOWN flag for fprobe? */
>  	ret = security_locked_down(LOCKDOWN_KPROBES);
> @@ -835,8 +840,47 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
>  	if (trace_fprobe_is_tracepoint(tf))
>  		return __regsiter_tracepoint_fprobe(tf);
>  
> -	/* TODO: handle filter, nofilter or symbol list */
> -	return register_fprobe(&tf->fp, tf->symbol, NULL);
> +	spec_len = strlen(tf->symbol);
> +	filter = kzalloc(spec_len + 1, GFP_KERNEL);
> +	nofilter = kzalloc(spec_len + 1, GFP_KERNEL);
> +	if (!filter || !nofilter)
> +		return -ENOMEM;
> +
> +	p = tf->symbol;
> +	for (p = tf->symbol; p; p = q ? q + 1 : NULL) {
> +		q = strchr(p, ',');
> +		tlen = q ? (size_t)(q-p) : strlen(p);
> +
> +		/* reject empty token */
> +		if (!tlen) {
> +			trace_probe_log_set_index(1);
> +			trace_probe_log_err(0, BAD_TP_NAME);
> +			return -EINVAL;
> +		}
> +
> +		if (*p == '!') {
> +			if (tlen == 1) {
> +				trace_probe_log_set_index(1);
> +				trace_probe_log_err(0, BAD_TP_NAME);
> +				return -EINVAL;
> +			}
> +			if (have_nf)
> +				nofilter[nflen++] = ',';
> +			memcpy(nofilter + nflen, p + 1, tlen - 1);
> +			nflen += tlen - 1;
> +			have_nf = true;
> +		} else {
> +			if (have_f)
> +				filter[flen++] = ',';
> +			memcpy(filter + flen, p, tlen);
> +			flen += tlen;
> +			have_f = true;
> +		}
> +	}
> +
> +	return register_fprobe(&tf->fp,
> +			have_f ? filter : NULL,
> +			have_nf ? nofilter : NULL);
>  }
>  
>  /* Internal unregister function - just handle fprobe and flags */
> -- 
> 2.43.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists