| 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
| ||
|
Message-ID: <aJyRblXjHcxz4EGF@gmail.com>
Date: Wed, 13 Aug 2025 22:21:50 +0900
From: Ryan Chung <seokwoo.chung130@...il.com>
To: Steven Rostedt <rostedt@...dmis.org>
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 Tue, Aug 12, 2025 at 02:03:57PM -0400, Steven Rostedt wrote:
> 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)
>
Thank you. I will make sure to follow the style guide.
> >
> > 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.
>
I will remove the block and integrate the code directly.
Is this the recommended way in linux kernel development?
> > + char *spec, *bang, *p;
> > + int n = 0, w = 0, j, rc;
>
> Indentation is always 8 byte tabs (not spaces).
>
I will convert to 8 byte tabs as mentioned.
> > + char **syms = NULL;
> > +
> > + spec = kstrdup(tf->symbol, GFP_KERNEL);
>
> Why did you declare spec as "char **" when you use it as "char *"?
>
This is my mistake.
I will correct the declaration.
> > + 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?
>
Yes I did use AI but not in a blatant way of copy-and-paste.
I am relatively new to the codebase and kernel development and therefore used
AI to help me get up to speed.
Please let me know if you don't recommend using AI.
> -- 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 */
>
I will send v2 shortly with the above comments in mind.
Best regards,
Ryan Chung
Powered by blists - more mailing lists