Re: [PATCH v2] trace_fprobe.c: TODO: handle filter, nofilter or symbol list
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Date: 2025-09-02 02:21:50
Also in:
lkml
Hi Ryan, Thanks for update. On Fri, 29 Aug 2025 19:20:50 +0900 Ryan Chung [off-list ref] 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/ (local) 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/ (local) Signed-off-by: Ryan Chung <redacted>
quoted hunk ↗ jump to hunk
--- 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,
quoted hunk ↗ jump to hunk
{ 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) [off-list ref]