Hi Ryan,
On Wed, 13 Aug 2025 01:21:01 +0900
Ryan Chung <seokwoo.chung...@gmail.com> wrote:
> Resolve TODO in `__register_trace_fprobe()`:
> parse `tf->symbol` robustly (support `sym!filter` and comma-separated lists),
> trim tokens, ignore empties, deduplicate symbols, use bulk registration for
> lists, return `-EEXIST` if already registered, and preserve
> lockdown/tracepoint deferral semantics.
Thanks for the improvement!
And could you add the new syntax in the document too ?
>
> Please note that this was my personal interpretation of what TODO
> required here. Welcoming any feedback.
>
> Signed-off-by: Ryan Chung <seokwoo.chung...@gmail.com>
> ---
> kernel/trace/trace_fprobe.c | 102 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index b40fa59159ac..37d4260b9012 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -12,6 +12,8 @@
> #include <linux/security.h>
> #include <linux/tracepoint.h>
> #include <linux/uaccess.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
Headers should be sorted alphabetically.
>
> #include "trace_dynevent.h"
> #include "trace_probe.h"
> @@ -762,8 +764,104 @@ static int __register_trace_fprobe(struct trace_fprobe
> *tf)
> return __regsiter_tracepoint_fprobe(tf);
> }
>
> - /* TODO: handle filter, nofilter or symbol list */
> - return register_fprobe(&tf->fp, tf->symbol, NULL);
> + /* Parse tf->symbol */
Please make this parse and check as a sub-function instead of new
scope. Also, it should be done in parse_symbol_and_return(), so that
we can handle wrong syntax when parsing it.
> + {
> + char *spec, *bang, *p;
> + int n = 0, w = 0, j, rc;
> + char **syms = NULL;
> +
> + spec = kstrdup(tf->symbol, GFP_KERNEL);
> + if (!spec)
> + return -ENOMEM;
> +
> + /* If a '!' exists, treat it as single symbol + filter */
> + bang = strchr(spec, '!');
> + if (bang) {
> + char *sym, *flt;
> +
> + *bang = '\0';
> + sym = strim(spec);
> + flt = strim(bang + 1);
You don't need to do strim, since if there is a space, it
should be parsed already. New syntax must be ',' separated.
My basic syntax for this probe event is;
WORD WORD WORD[:OPTWORD] SUBWORD[,SUBWORD]
OPTWORD is qualifying the previous WORD, SUBWORDs are not
quarifying, but the same-level words. (Currently using "%return"
for the return of the function, that is a special case.)
> +
> + if (!*sym || !*flt) {
> + kfree(spec);
Please use __free(kfree) instead of repeating kfree().
> + return -EINVAL; /* reject empty symbol/filter */
Also, before returning an error, use trace_probe_log_err() to
notice the reason and the place of the error to user.
> + }
> +
> + rc = register_fprobe(&tf->fp, sym, flt);
> + kfree(spec);
> + return rc;
> + }
> +
> + /* Comma list (or single symbol without '!') */
> + /* First pass: count non-empty tokens */
> + p = spec;
> + while (p) {
> + char *tok = strsep(&p, ",");
> + if (tok && *strim(tok))
> + n++;
> + }
> +
> + if (n == 0){
> + kfree(spec);
> + return -EINVAL;
> + }
> +
> + /* Allocate array for pointers into spec (callee copies/consumes) */
> + syms = kcalloc(n, sizeof(*syms), GFP_KERNEL);
> + if (!syms) {
> + kfree(spec);
> + return -ENOMEM;
> + }
> +
> + /* Second pass: fill, skipping empties */
Again, symbol should not have a space.
> + p = spec;
> + while (p) {
> + char *tok = strsep(&p, ",");
> + char *s;
> +
> + if (!tok)
> + break;
> + s = strim(tok);
> + if (!*s)
> + continue;
> + syms[w++] = s;
> + }
> +
> + /* Dedup in-place */
> + for (i = 0; i < w; i++){
> + if (!syms[i])
> + continue;
> + for (j = i + 1; j < w; j++) {
> + if (syms[j] && !strcmp(syms[i], syms[j]))
> + syms[j] = NULL;
> + }
I think dedup will be done in ftrace, so we don't need to do this
costly operation.
> + }
> +
> + /* Compact */
> + for (i = 0, j = 0; i < w; i++) {
> + if (syms[i])
> + syms[j++] = syms[i];
> + }
> + w = j;
> +
> + /* After dedup, ensure we still have at least one symbol */
> + if (w == 0){
> + kfree(syms);
> + kfree(spec);
> + return -EINVAL;
> + }
> +
> + /* Register list or single symbol, using the existing bulk API */
> + if (w == 1)
> + rc = register_fprobe(&tf->fp, syms[0], NULL);
Hmm, you might misunderstand this. What you need to do is to classify
the list of symbols with '!' as nofilter, and others as "filter",
and pass those as "register_fprobe(&tf->fp, filter, nofilter)".
Thank you,
> + else
> + rc = register_fprobe_syms(&tf->fp, (const char **)syms, w);
> +
> + kfree(syms);
> + kfree(spec);
> + return rc;
> + }
> }
>
> /* Internal unregister function - just handle fprobe and flags */
> --
> 2.43.0
>
--
Masami Hiramatsu (Google) <mhira...@kernel.org>
