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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250812140357.65d19062@gandalf.local.home>
Date: Tue, 12 Aug 2025 14:03:57 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Ryan Chung <seokwoo.chung130@...il.com>
Cc: mhiramat@...nel.org, mathieu.desnoyer@...icios.com,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or
 symbol list

On Wed, 13 Aug 2025 01:21:01 +0900
Ryan Chung <seokwoo.chung130@...il.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.

Hi Ryan,

Please read the Submitting Patches document to have proper format.

 https://docs.kernel.org/process/submitting-patches.html


For example, the change long should have a max column of 74 (with the
exception of cut and paste commands or output)

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

Code does not add random blocks.

> +        char *spec, *bang, *p;
> +        int n = 0, w = 0, j, rc;

Indentation is always 8 byte tabs (not spaces).

> +        char **syms = NULL;
> +
> +        spec = kstrdup(tf->symbol, GFP_KERNEL);

Why did you declare spec as "char **" when you use it as "char *"?

> +        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);
> +
> +            if (!*sym || !*flt) {
> +                kfree(spec);
> +                return -EINVAL; /* reject empty symbol/filter */
> +            }
> +
> +            rc = register_fprobe(&tf->fp, sym, flt);
> +            kfree(spec);
> +            return rc;
> +        }
> +
> +        /* Comma list (or single symbol without '!') */
> +        /* First pass: count non-empty tokens */

Strange comments. Did you use AI to help you write this?

-- Steve

> +        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 */
> +        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;
> +            }
> +        }
> +
> +        /* 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);
> +        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 */