Hi Ryan, Thanks for update.
On Fri, 29 Aug 2025 19:20:50 +0900 Ryan Chung <seokwoo.chung...@gmail.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.chung...@gmail.com/ > Signed-off-by: Ryan Chung <seokwoo.chung...@gmail.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) <mhira...@kernel.org>
