Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2025-08-12 18:03:15
Also in:
linux-kernel-mentees, lkml
On Wed, 13 Aug 2025 01:21:01 +0900 Ryan Chung [off-list ref] 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)
quoted hunk ↗ jump to hunk
Please note that this was my personal interpretation of what TODO required here. Welcoming any feedback. Signed-off-by: Ryan Chung <redacted> --- 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 */