Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list

[Ryan Chung <seokwoo.chung130@gmail.com>]

On Thu, Aug 14, 2025 at 12:15:04PM +0900, Masami Hiramatsu wrote:
> Hi Ryan,
> 
> On Wed, 13 Aug 2025 01:21:01 +0900
> Ryan Chung <seokwoo.chung130@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 ?
> 

Yes. I will add the syntax in the document.
To clarify, by document, you mean
Documentation/trace/fprobetrace.rst?

> > 
> > Please note that this was my personal interpretation of what TODO
> > required here. Welcoming any feedback. 
> > 
> > Signed-off-by: Ryan Chung <seokwoo.chung130@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.
> 

I will fix this in v2.

> >  
> >  #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.
> 

I will move the parsing into parse_symbol_and_return()
so syntax errors are detected at parse time.

> > +    {
> > +        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.)
> 

Understood. I will drop strim() and treat tokens as you mentioned.
I will leave return behavior unchanged.

> > +
> > +            if (!*sym || !*flt) {
> > +                kfree(spec);
> 
> Please use __free(kfree) instead of repeating kfree().
> 

I will also include this in v2.

> > +                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.
> 

I will log parse failiures with trace_probe_log_err().

> > +            }
> > +
> > +            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.
> 

Understood. I will also fix this in v2.

> > +        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.
> 

I see. I will remove the dedup here.

> > +        }
> > +
> > +        /* 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 for the clarification.
I will change as followed:
- tokens prefixed with '!' go to the nofileter list
- all other tokens go to filter list
- pass both to 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) <mhiramat@kernel.org>

Thank you. Please let me know if you have any questions or concerns.

Best regards,
Ryan Chung