* [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
@ 2025-08-12 16:21 Ryan Chung
2025-08-12 18:03 ` Steven Rostedt
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ryan Chung @ 2025-08-12 16:21 UTC (permalink / raw)
To: rostedt, mhiramat, mathieu.desnoyer
Cc: linux-kernel, linux-trace-kernel, linux-kernel-mentees,
Ryan Chung
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.
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>
#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 */
+ {
+ 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);
+
+ 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 */
+ 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 */
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
2025-08-12 16:21 [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list Ryan Chung
@ 2025-08-12 18:03 ` Steven Rostedt
2025-08-13 13:21 ` Ryan Chung
2025-08-14 3:15 ` Masami Hiramatsu
2025-08-17 4:28 ` kernel test robot
2 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2025-08-12 18:03 UTC (permalink / raw)
To: Ryan Chung
Cc: mhiramat, mathieu.desnoyer, linux-kernel, linux-trace-kernel,
linux-kernel-mentees
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.
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@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>
>
> #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 */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
2025-08-12 18:03 ` Steven Rostedt
@ 2025-08-13 13:21 ` Ryan Chung
0 siblings, 0 replies; 6+ messages in thread
From: Ryan Chung @ 2025-08-13 13:21 UTC (permalink / raw)
To: Steven Rostedt
Cc: mhiramat, mathieu.desnoyer, linux-kernel, linux-trace-kernel,
linux-kernel-mentees
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@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.
>
> 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@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>
> >
> > #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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
2025-08-12 16:21 [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list Ryan Chung
2025-08-12 18:03 ` Steven Rostedt
@ 2025-08-14 3:15 ` Masami Hiramatsu
2025-08-19 16:37 ` Ryan Chung
2025-08-17 4:28 ` kernel test robot
2 siblings, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2025-08-14 3:15 UTC (permalink / raw)
To: Ryan Chung
Cc: rostedt, mathieu.desnoyer, linux-kernel, linux-trace-kernel,
linux-kernel-mentees
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 ?
>
> 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.
>
> #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) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
2025-08-12 16:21 [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list Ryan Chung
2025-08-12 18:03 ` Steven Rostedt
2025-08-14 3:15 ` Masami Hiramatsu
@ 2025-08-17 4:28 ` kernel test robot
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2025-08-17 4:28 UTC (permalink / raw)
To: Ryan Chung, rostedt, mhiramat, mathieu.desnoyer
Cc: oe-kbuild-all, linux-kernel, linux-trace-kernel,
linux-kernel-mentees, Ryan Chung
Hi Ryan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.16]
[also build test WARNING on linus/master next-20250815]
[cannot apply to trace/for-next v6.17-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Ryan-Chung/trace-trace_fprobe-c-TODO-handle-filter-nofilter-or-symbol-list/20250813-002748
base: v6.16
patch link: https://lore.kernel.org/r/20250812162101.5981-1-seokwoo.chung130%40gmail.com
patch subject: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
config: s390-randconfig-r073-20250817 (https://download.01.org/0day-ci/archive/20250817/202508171256.CSm9DAkb-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 8.5.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508171256.CSm9DAkb-lkp@intel.com/
smatch warnings:
kernel/trace/trace_fprobe.c:768 __register_trace_fprobe() warn: inconsistent indenting
vim +768 kernel/trace/trace_fprobe.c
732
733 /* Internal register function - just handle fprobe and flags */
734 static int __register_trace_fprobe(struct trace_fprobe *tf)
735 {
736 int i, ret;
737
738 /* Should we need new LOCKDOWN flag for fprobe? */
739 ret = security_locked_down(LOCKDOWN_KPROBES);
740 if (ret)
741 return ret;
742
743 if (trace_fprobe_is_registered(tf))
744 return -EINVAL;
745
746 for (i = 0; i < tf->tp.nr_args; i++) {
747 ret = traceprobe_update_arg(&tf->tp.args[i]);
748 if (ret)
749 return ret;
750 }
751
752 /* Set/clear disabled flag according to tp->flag */
753 if (trace_probe_is_enabled(&tf->tp))
754 tf->fp.flags &= ~FPROBE_FL_DISABLED;
755 else
756 tf->fp.flags |= FPROBE_FL_DISABLED;
757
758 if (trace_fprobe_is_tracepoint(tf)) {
759
760 /* This tracepoint is not loaded yet */
761 if (tf->tpoint == TRACEPOINT_STUB)
762 return 0;
763
764 return __regsiter_tracepoint_fprobe(tf);
765 }
766
767 /* Parse tf->symbol */
> 768 {
769 char *spec, *bang, *p;
770 int n = 0, w = 0, j, rc;
771 char **syms = NULL;
772
773 spec = kstrdup(tf->symbol, GFP_KERNEL);
774 if (!spec)
775 return -ENOMEM;
776
777 /* If a '!' exists, treat it as single symbol + filter */
778 bang = strchr(spec, '!');
779 if (bang) {
780 char *sym, *flt;
781
782 *bang = '\0';
783 sym = strim(spec);
784 flt = strim(bang + 1);
785
786 if (!*sym || !*flt) {
787 kfree(spec);
788 return -EINVAL; /* reject empty symbol/filter */
789 }
790
791 rc = register_fprobe(&tf->fp, sym, flt);
792 kfree(spec);
793 return rc;
794 }
795
796 /* Comma list (or single symbol without '!') */
797 /* First pass: count non-empty tokens */
798 p = spec;
799 while (p) {
800 char *tok = strsep(&p, ",");
801 if (tok && *strim(tok))
802 n++;
803 }
804
805 if (n == 0){
806 kfree(spec);
807 return -EINVAL;
808 }
809
810 /* Allocate array for pointers into spec (callee copies/consumes) */
811 syms = kcalloc(n, sizeof(*syms), GFP_KERNEL);
812 if (!syms) {
813 kfree(spec);
814 return -ENOMEM;
815 }
816
817 /* Second pass: fill, skipping empties */
818 p = spec;
819 while (p) {
820 char *tok = strsep(&p, ",");
821 char *s;
822
823 if (!tok)
824 break;
825 s = strim(tok);
826 if (!*s)
827 continue;
828 syms[w++] = s;
829 }
830
831 /* Dedup in-place */
832 for (i = 0; i < w; i++){
833 if (!syms[i])
834 continue;
835 for (j = i + 1; j < w; j++) {
836 if (syms[j] && !strcmp(syms[i], syms[j]))
837 syms[j] = NULL;
838 }
839 }
840
841 /* Compact */
842 for (i = 0, j = 0; i < w; i++) {
843 if (syms[i])
844 syms[j++] = syms[i];
845 }
846 w = j;
847
848 /* After dedup, ensure we still have at least one symbol */
849 if (w == 0){
850 kfree(syms);
851 kfree(spec);
852 return -EINVAL;
853 }
854
855 /* Register list or single symbol, using the existing bulk API */
856 if (w == 1)
857 rc = register_fprobe(&tf->fp, syms[0], NULL);
858 else
859 rc = register_fprobe_syms(&tf->fp, (const char **)syms, w);
860
861 kfree(syms);
862 kfree(spec);
863 return rc;
864 }
865 }
866
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list
2025-08-14 3:15 ` Masami Hiramatsu
@ 2025-08-19 16:37 ` Ryan Chung
0 siblings, 0 replies; 6+ messages in thread
From: Ryan Chung @ 2025-08-19 16:37 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: rostedt, mathieu.desnoyer, linux-kernel, linux-trace-kernel,
linux-kernel-mentees
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-08-19 16:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-12 16:21 [PATCH] trace/trace_fprobe.c: TODO: handle filter, nofilter or symbol list Ryan Chung
2025-08-12 18:03 ` Steven Rostedt
2025-08-13 13:21 ` Ryan Chung
2025-08-14 3:15 ` Masami Hiramatsu
2025-08-19 16:37 ` Ryan Chung
2025-08-17 4:28 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).