Signed-off-by: Ryan Chung <seokwoo.chung130@gmail.com>
---
kernel/trace/trace_fprobe.c | 247 ++++++++++++++++++++++++++++--------
1 file changed, 192 insertions(+), 55 deletions(-)
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index b36ade43d4b3..ec5b6e1c1a1b 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -191,6 +191,9 @@ struct trace_fprobe {
bool tprobe;
struct tracepoint_user *tuser;
struct trace_probe tp;
+ char *filter;
+ char *nofilter;
+ bool list_mode;
};
static bool is_trace_fprobe(struct dyn_event *ev)
@@ -203,14 +206,10 @@ static struct trace_fprobe *to_trace_fprobe(struct dyn_event *ev)
return container_of(ev, struct trace_fprobe, devent);
}
-/**
- * for_each_trace_fprobe - iterate over the trace_fprobe list
- * @pos: the struct trace_fprobe * for each entry
- * @dpos: the struct dyn_event * to use as a loop cursor
- */
-#define for_each_trace_fprobe(pos, dpos) \
- for_each_dyn_event(dpos) \
- if (is_trace_fprobe(dpos) && (pos = to_trace_fprobe(dpos)))
+static struct trace_fprobe *trace_fprobe_from_dyn(struct dyn_event *ev)
+{
+ return is_trace_fprobe(ev) ? to_trace_fprobe(ev) : NULL;
+}
static bool trace_fprobe_is_return(struct trace_fprobe *tf)
{
@@ -227,6 +226,109 @@ static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
return tf->symbol ? tf->symbol : "unknown";
}
+static bool has_wildcard(const char *s)
+{
+ return s && (strchr(s, '*') || strchr(s, '?'));
+}
+
+static int parse_fprobe_spec(const char *in, bool is_tracepoint,
+ char **base, bool *is_return, bool *list_mode,
+ char **filter, char **nofilter)
+{
+ const char *p;
+ char *work = NULL;
+ char *b = NULL, *f = NULL, *nf = NULL;
+ bool legacy_ret = false;
+ bool list = false;
+ int ret = 0;
+
+ if (!in || !base || !is_return || !list_mode || !filter || !nofilter)
+ return -EINVAL;
+
+ *base = NULL; *filter = NULL; *nofilter = NULL;
+ *is_return = false; *list_mode = false;
+
+ if (is_tracepoint) {
+ if (strchr(in, ',') || strchr(in, ':'))
+ return -EINVAL;
+ if (strstr(in, "%return"))
+ return -EINVAL;
+ for (p = in; *p; p++)
+ if (!isalnum(*p) && *p != '_')
+ return -EINVAL;
+ b = kstrdup(in, GFP_KERNEL);
+ if (!b)
+ return -ENOMEM;
+ *base = b;
+ return 0;
+ }
+
+ work = kstrdup(in, GFP_KERNEL);
+ if (!work)
+ return -ENOMEM;
+
+ p = strstr(work, "%return");
+ if (p) {
+ if (!strcmp(p, ":exit")) {
+ *is_return = true;
+ *p = '\0';
+ } else if (!strcmp(p, ":entry")) {
+ *p = '\0';
+ } else {
+ ret = -EINVAL;
+ goto out;
+ }
+ }
+
+ list = !!strchr(work, ',') || has_wildcard(work);
+ if (legacy_ret)
+ *is_return = true;
+
+ b = kstrdup(work, GFP_KERNEL);
+ if (!b) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ if (list) {
+ char *tmp = b, *tok;
+ size_t fsz = strlen(b) + 1, nfsz = strlen(b) + 1;
+
+ f = kzalloc(fsz, GFP_KERNEL);
+ nf = kzalloc(nfsz, GFP_KERNEL);
+ if (!f || !nf) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ while ((tok = strsep(&tmp, ",")) != NULL) {
+ char *dst;
+ bool neg = (*tok == '!');
+
+ if (*tok == '\0')
+ continue;
+ if (neg)
+ tok++;
+ dst = neg ? nf : f;
+ if (dst[0] != '\0')
+ strcat(dst, ",");
+ strcat(dst, tok);
+ }
+ *list_mode = true;
+ }
+
+ *base = b; b = NULL;
+ *filter = f; f = NULL;
+ *nofilter = nf; nf = NULL;
+
+out:
+ kfree(work);
+ kfree(b);
+ kfree(f);
+ kfree(nf);
+ return ret;
+}
+
static bool trace_fprobe_is_busy(struct dyn_event *ev)
{
struct trace_fprobe *tf = to_trace_fprobe(ev);
@@ -556,13 +658,17 @@ static void free_trace_fprobe(struct trace_fprobe *tf)
trace_probe_cleanup(&tf->tp);
if (tf->tuser)
tracepoint_user_put(tf->tuser);
+ kfree(tf->filter);
+ kfree(tf->nofilter);
kfree(tf->symbol);
kfree(tf);
}
}
/* Since alloc_trace_fprobe() can return error, check the pointer is ERR too. */
-DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) free_trace_fprobe(_T))
+DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *,
+ if (!IS_ERR_OR_NULL(_T))
+ free_trace_fprobe(_T))
/*
* Allocate new trace_probe and initialize it (including fprobe).
@@ -605,10 +711,16 @@ static struct trace_fprobe *find_trace_fprobe(const char *event,
struct dyn_event *pos;
struct trace_fprobe *tf;
- for_each_trace_fprobe(tf, pos)
+ list_for_each_entry(pos, &dyn_event_list, list) {
+ tf = trace_fprobe_from_dyn(pos);
+ if (!tf)
+ continue;
+
if (strcmp(trace_probe_name(&tf->tp), event) == 0 &&
strcmp(trace_probe_group_name(&tf->tp), group) == 0)
return tf;
+ }
+
return NULL;
}
@@ -835,7 +947,12 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
if (trace_fprobe_is_tracepoint(tf))
return __regsiter_tracepoint_fprobe(tf);
- /* TODO: handle filter, nofilter or symbol list */
+ /* Registration path:
+ * - list_mode: pass filter/nofilter
+ * - single: pass symbol only (legacy)
+ */
+ if (tf->list_mode)
+ return register_fprobe(&tf->fp, tf->filter, tf->nofilter);
return register_fprobe(&tf->fp, tf->symbol, NULL);
}
@@ -1114,7 +1231,11 @@ static int __tprobe_event_module_cb(struct notifier_block *self,
return NOTIFY_DONE;
mutex_lock(&event_mutex);
- for_each_trace_fprobe(tf, pos) {
+ list_for_each_entry(pos, &dyn_event_list, list) {
+ tf = trace_fprobe_from_dyn(pos);
+ if (!tf)
+ continue;
+
/* Skip fprobe and disabled tprobe events. */
if (!trace_fprobe_is_tracepoint(tf) || !tf->tuser)
continue;
@@ -1155,55 +1276,35 @@ static int parse_symbol_and_return(int argc, const char *argv[],
char **symbol, bool *is_return,
bool is_tracepoint)
{
- char *tmp = strchr(argv[1], '%');
- int i;
-
- if (tmp) {
- int len = tmp - argv[1];
-
- if (!is_tracepoint && !strcmp(tmp, "%return")) {
- *is_return = true;
- } else {
- trace_probe_log_err(len, BAD_ADDR_SUFFIX);
- return -EINVAL;
- }
- *symbol = kmemdup_nul(argv[1], len, GFP_KERNEL);
- } else
- *symbol = kstrdup(argv[1], GFP_KERNEL);
- if (!*symbol)
- return -ENOMEM;
-
- if (*is_return)
- return 0;
+ int i, ret;
+ bool list_mode = false;
+ char *filter = NULL; *nofilter = NULL;
- if (is_tracepoint) {
- tmp = *symbol;
- while (*tmp && (isalnum(*tmp) || *tmp == '_'))
- tmp++;
- if (*tmp) {
- /* find a wrong character. */
- trace_probe_log_err(tmp - *symbol, BAD_TP_NAME);
- kfree(*symbol);
- *symbol = NULL;
- return -EINVAL;
- }
- }
+ ret = parse_fprobe_spec(argv[1], is_tracepoint, symbol, is_return,
+ &list_mode, &filter, &nofilter);
+ if (ret)
+ return ret;
- /* If there is $retval, this should be a return fprobe. */
for (i = 2; i < argc; i++) {
- tmp = strstr(argv[i], "$retval");
+ char *tmp = strstr(argv[i], "$retval");
+
if (tmp && !isalnum(tmp[7]) && tmp[7] != '_') {
if (is_tracepoint) {
trace_probe_log_set_index(i);
trace_probe_log_err(tmp - argv[i], RETVAL_ON_PROBE);
kfree(*symbol);
*symbol = NULL;
+ kfree(filter);
+ kfree(nofilter);
return -EINVAL;
}
*is_return = true;
break;
}
}
+
+ kfree(filter);
+ kfree(nofilter);
return 0;
}
@@ -1247,6 +1348,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
int i, new_argc = 0, ret = 0;
bool is_tracepoint = false;
bool is_return = false;
+ bool list_mode = false;
+
+ char *parsed_filter __free(kfree) = NULL;
+ char *parsed_nofilter __free(kfree) = NULL;
+ bool has_wild = false;
if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
return -ECANCELED;
@@ -1267,8 +1373,9 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
trace_probe_log_set_index(1);
- /* a symbol(or tracepoint) must be specified */
- ret = parse_symbol_and_return(argc, argv, &symbol, &is_return, is_tracepoint);
+ /* Parse spec early (single vs list, suffix, base symbol) */
+ ret = parse_fprobe_spec(argv[1], is_tracepoint, &symbol, &is_return,
+ &list_mode, &parsed_filter, &parsed_nofilter);
if (ret < 0)
return -EINVAL;
@@ -1283,10 +1390,16 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
return -EINVAL;
}
- if (!event) {
- ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
- if (!ebuf)
- return -ENOMEM;
+ if (!event) {
+ /*
+ * Event name rules:
+ * - For list/wildcard: require explicit [GROUP/]EVENT
+ * - For single literal: autogenerate symbol__entry/symbol__exit
+ */
+ if (list_mode || has_wildcard(symbol)) {
+ trace_probe_log_err(0, NO_GROUP_NAME);
+ return -EINVAL;
+ }
/* Make a new event name */
if (is_tracepoint)
snprintf(ebuf, MAX_EVENT_NAME_LEN, "%s%s",
@@ -1319,7 +1432,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
NULL, NULL, NULL, sbuf);
}
}
- if (!ctx->funcname)
+
+ if (!list_mode && !has_wildcard(symbol) && !is_tracepoint)
ctx->funcname = symbol;
abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
@@ -1353,6 +1467,21 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
return ret;
}
+ /* carry list parsing result into tf */
+ if (!is_tracepoint) {
+ tf->list_mode = list_mode;
+ if (parsed_filter) {
+ tf->filter = kstrdup(parsed_filter, GFP_KERNEL);
+ if (!tf->filter)
+ return -ENOMEM;
+ }
+ if (parsed_nofilter) {
+ tf->nofilter = kstrdup(parsed_nofilter, GFP_KERNEL);
+ if (!tf->nofilter)
+ return -ENOMEM;
+ }
+ }
+
/* parse arguments */
for (i = 0; i < argc; i++) {
trace_probe_log_set_index(i + 2);
@@ -1439,8 +1568,16 @@ static int trace_fprobe_show(struct seq_file *m, struct dyn_event *ev)
seq_printf(m, ":%s/%s", trace_probe_group_name(&tf->tp),
trace_probe_name(&tf->tp));
- seq_printf(m, " %s%s", trace_fprobe_symbol(tf),
- trace_fprobe_is_return(tf) ? "%return" : "");
+ seq_printf(m, "%s", trace_fprobe_symbol(tf));
+ if (!trace_fprobe_is_tracepoint(tf)) {
+ if (tf->list_mode) {
+ if (trace_fprobe_is_return(tf))
+ seq_puts(m, ":exit");
+ } else {
+ if (trace_fprobe_is_return(tf))
+ seq_puts(m, "%return");
+ }
+ }
for (i = 0; i < tf->tp.nr_args; i++)
seq_printf(m, " %s=%s", tf->tp.args[i].name, tf->tp.args[i].comm);
--
2.43.0Hi Ryan,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.17]
[also build test ERROR on linus/master next-20251010]
[cannot apply to trace/for-next]
[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/docs-tracing-fprobe-document-list-filters-and-entry-exit/20251010-111713
base: v6.17
patch link: https://lore.kernel.org/r/20251004235001.133111-4-seokwoo.chung130%40gmail.com
patch subject: [PATCH v3 3/5] tracing: fprobe: support comma-separated symbols and :entry/:exit
config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20251010/202510102331.y36ENO9m-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251010/202510102331.y36ENO9m-lkp@intel.com/reproduce)
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/202510102331.y36ENO9m-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
kernel/trace/trace_fprobe.c: In function 'parse_fprobe_spec':
>> kernel/trace/trace_fprobe.c:274:28: error: assignment of read-only location '*p'
274 | *p = '\0';
| ^
kernel/trace/trace_fprobe.c:276:28: error: assignment of read-only location '*p'
276 | *p = '\0';
| ^
kernel/trace/trace_fprobe.c: In function 'parse_symbol_and_return':
>> kernel/trace/trace_fprobe.c:1281:31: error: 'nofilter' undeclared (first use in this function); did you mean 'filter'?
1281 | char *filter = NULL; *nofilter = NULL;
| ^~~~~~~~
| filter
kernel/trace/trace_fprobe.c:1281:31: note: each undeclared identifier is reported only once for each function it appears in
kernel/trace/trace_fprobe.c: In function 'trace_fprobe_create_internal':
kernel/trace/trace_fprobe.c:1355:14: warning: unused variable 'has_wild' [-Wunused-variable]
1355 | bool has_wild = false;
| ^~~~~~~~
kernel/trace/trace_fprobe.c: At top level:
>> kernel/trace/trace_fprobe.c:1275:12: warning: 'parse_symbol_and_return' defined but not used [-Wunused-function]
1275 | static int parse_symbol_and_return(int argc, const char *argv[],
| ^~~~~~~~~~~~~~~~~~~~~~~
vim +274 kernel/trace/trace_fprobe.c
233
234 static int parse_fprobe_spec(const char *in, bool is_tracepoint,
235 char **base, bool *is_return, bool *list_mode,
236 char **filter, char **nofilter)
237 {
238 const char *p;
239 char *work = NULL;
240 char *b = NULL, *f = NULL, *nf = NULL;
241 bool legacy_ret = false;
242 bool list = false;
243 int ret = 0;
244
245 if (!in || !base || !is_return || !list_mode || !filter || !nofilter)
246 return -EINVAL;
247
248 *base = NULL; *filter = NULL; *nofilter = NULL;
249 *is_return = false; *list_mode = false;
250
251 if (is_tracepoint) {
252 if (strchr(in, ',') || strchr(in, ':'))
253 return -EINVAL;
254 if (strstr(in, "%return"))
255 return -EINVAL;
256 for (p = in; *p; p++)
257 if (!isalnum(*p) && *p != '_')
258 return -EINVAL;
259 b = kstrdup(in, GFP_KERNEL);
260 if (!b)
261 return -ENOMEM;
262 *base = b;
263 return 0;
264 }
265
266 work = kstrdup(in, GFP_KERNEL);
267 if (!work)
268 return -ENOMEM;
269
270 p = strstr(work, "%return");
271 if (p) {
272 if (!strcmp(p, ":exit")) {
273 *is_return = true;
> 274 *p = '\0';
275 } else if (!strcmp(p, ":entry")) {
276 *p = '\0';
277 } else {
278 ret = -EINVAL;
279 goto out;
280 }
281 }
282
283 list = !!strchr(work, ',') || has_wildcard(work);
284 if (legacy_ret)
285 *is_return = true;
286
287 b = kstrdup(work, GFP_KERNEL);
288 if (!b) {
289 ret = -ENOMEM;
290 goto out;
291 }
292
293 if (list) {
294 char *tmp = b, *tok;
295 size_t fsz = strlen(b) + 1, nfsz = strlen(b) + 1;
296
297 f = kzalloc(fsz, GFP_KERNEL);
298 nf = kzalloc(nfsz, GFP_KERNEL);
299 if (!f || !nf) {
300 ret = -ENOMEM;
301 goto out;
302 }
303
304 while ((tok = strsep(&tmp, ",")) != NULL) {
305 char *dst;
306 bool neg = (*tok == '!');
307
308 if (*tok == '\0')
309 continue;
310 if (neg)
311 tok++;
312 dst = neg ? nf : f;
313 if (dst[0] != '\0')
314 strcat(dst, ",");
315 strcat(dst, tok);
316 }
317 *list_mode = true;
318 }
319
320 *base = b; b = NULL;
321 *filter = f; f = NULL;
322 *nofilter = nf; nf = NULL;
323
324 out:
325 kfree(work);
326 kfree(b);
327 kfree(f);
328 kfree(nf);
329 return ret;
330 }
331
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Ryan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.17]
[also build test WARNING on linus/master next-20251010]
[cannot apply to trace/for-next]
[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/docs-tracing-fprobe-document-list-filters-and-entry-exit/20251010-111713
base: v6.17
patch link: https://lore.kernel.org/r/20251004235001.133111-4-seokwoo.chung130%40gmail.com
patch subject: [PATCH v3 3/5] tracing: fprobe: support comma-separated symbols and :entry/:exit
config: x86_64-randconfig-073-20251010 (https://download.01.org/0day-ci/archive/20251010/202510102214.7msIkpAr-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251010/202510102214.7msIkpAr-lkp@intel.com/reproduce)
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/202510102214.7msIkpAr-lkp@intel.com/
All warnings (new ones prefixed by >>):
kernel/trace/trace_fprobe.c:274:7: error: read-only variable is not assignable
274 | *p = '\0';
| ~~ ^
kernel/trace/trace_fprobe.c:276:7: error: read-only variable is not assignable
276 | *p = '\0';
| ~~ ^
kernel/trace/trace_fprobe.c:1281:24: error: use of undeclared identifier 'nofilter'; did you mean 'filter'?
1281 | char *filter = NULL; *nofilter = NULL;
| ^~~~~~~~
| filter
kernel/trace/trace_fprobe.c:1281:8: note: 'filter' declared here
1281 | char *filter = NULL; *nofilter = NULL;
| ^
kernel/trace/trace_fprobe.c:1284:26: error: use of undeclared identifier 'nofilter'; did you mean 'filter'?
1284 | &list_mode, &filter, &nofilter);
| ^~~~~~~~
| filter
kernel/trace/trace_fprobe.c:1281:8: note: 'filter' declared here
1281 | char *filter = NULL; *nofilter = NULL;
| ^
kernel/trace/trace_fprobe.c:1298:11: error: use of undeclared identifier 'nofilter'; did you mean 'filter'?
1298 | kfree(nofilter);
| ^~~~~~~~
| filter
kernel/trace/trace_fprobe.c:1281:8: note: 'filter' declared here
1281 | char *filter = NULL; *nofilter = NULL;
| ^
kernel/trace/trace_fprobe.c:1307:8: error: use of undeclared identifier 'nofilter'; did you mean 'filter'?
1307 | kfree(nofilter);
| ^~~~~~~~
| filter
kernel/trace/trace_fprobe.c:1281:8: note: 'filter' declared here
1281 | char *filter = NULL; *nofilter = NULL;
| ^
>> kernel/trace/trace_fprobe.c:1355:7: warning: unused variable 'has_wild' [-Wunused-variable]
1355 | bool has_wild = false;
| ^~~~~~~~
1 warning and 6 errors generated.
vim +/has_wild +1355 kernel/trace/trace_fprobe.c
1274
1275 static int parse_symbol_and_return(int argc, const char *argv[],
1276 char **symbol, bool *is_return,
1277 bool is_tracepoint)
1278 {
1279 int i, ret;
1280 bool list_mode = false;
1281 char *filter = NULL; *nofilter = NULL;
1282
1283 ret = parse_fprobe_spec(argv[1], is_tracepoint, symbol, is_return,
1284 &list_mode, &filter, &nofilter);
1285 if (ret)
1286 return ret;
1287
1288 for (i = 2; i < argc; i++) {
1289 char *tmp = strstr(argv[i], "$retval");
1290
1291 if (tmp && !isalnum(tmp[7]) && tmp[7] != '_') {
1292 if (is_tracepoint) {
1293 trace_probe_log_set_index(i);
1294 trace_probe_log_err(tmp - argv[i], RETVAL_ON_PROBE);
1295 kfree(*symbol);
1296 *symbol = NULL;
1297 kfree(filter);
1298 kfree(nofilter);
1299 return -EINVAL;
1300 }
1301 *is_return = true;
1302 break;
1303 }
1304 }
1305
1306 kfree(filter);
> 1307 kfree(nofilter);
1308 return 0;
1309 }
1310
1311 static int trace_fprobe_create_internal(int argc, const char *argv[],
1312 struct traceprobe_parse_context *ctx)
1313 {
1314 /*
1315 * Argument syntax:
1316 * - Add fentry probe:
1317 * f[:[GRP/][EVENT]] [MOD:]KSYM [FETCHARGS]
1318 * - Add fexit probe:
1319 * f[N][:[GRP/][EVENT]] [MOD:]KSYM%return [FETCHARGS]
1320 * - Add tracepoint probe:
1321 * t[:[GRP/][EVENT]] TRACEPOINT [FETCHARGS]
1322 *
1323 * Fetch args:
1324 * $retval : fetch return value
1325 * $stack : fetch stack address
1326 * $stackN : fetch Nth entry of stack (N:0-)
1327 * $argN : fetch Nth argument (N:1-)
1328 * $comm : fetch current task comm
1329 * @ADDR : fetch memory at ADDR (ADDR should be in kernel)
1330 * @SYM[+|-offs] : fetch memory at SYM +|- offs (SYM is a data symbol)
1331 * Dereferencing memory fetch:
1332 * +|-offs(ARG) : fetch memory at ARG +|- offs address.
1333 * Alias name of args:
1334 * NAME=FETCHARG : set NAME as alias of FETCHARG.
1335 * Type of args:
1336 * FETCHARG:TYPE : use TYPE instead of unsigned long.
1337 */
1338 struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
1339 const char *event = NULL, *group = FPROBE_EVENT_SYSTEM;
1340 struct module *mod __free(module_put) = NULL;
1341 const char **new_argv __free(kfree) = NULL;
1342 char *symbol __free(kfree) = NULL;
1343 char *ebuf __free(kfree) = NULL;
1344 char *gbuf __free(kfree) = NULL;
1345 char *sbuf __free(kfree) = NULL;
1346 char *abuf __free(kfree) = NULL;
1347 char *dbuf __free(kfree) = NULL;
1348 int i, new_argc = 0, ret = 0;
1349 bool is_tracepoint = false;
1350 bool is_return = false;
1351 bool list_mode = false;
1352
1353 char *parsed_filter __free(kfree) = NULL;
1354 char *parsed_nofilter __free(kfree) = NULL;
> 1355 bool has_wild = false;
1356
1357 if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
1358 return -ECANCELED;
1359
1360 if (argv[0][0] == 't') {
1361 is_tracepoint = true;
1362 group = TRACEPOINT_EVENT_SYSTEM;
1363 }
1364
1365 if (argv[0][1] != '\0') {
1366 if (argv[0][1] != ':') {
1367 trace_probe_log_set_index(0);
1368 trace_probe_log_err(1, BAD_MAXACT);
1369 return -EINVAL;
1370 }
1371 event = &argv[0][2];
1372 }
1373
1374 trace_probe_log_set_index(1);
1375
1376 /* Parse spec early (single vs list, suffix, base symbol) */
1377 ret = parse_fprobe_spec(argv[1], is_tracepoint, &symbol, &is_return,
1378 &list_mode, &parsed_filter, &parsed_nofilter);
1379 if (ret < 0)
1380 return -EINVAL;
1381
1382 trace_probe_log_set_index(0);
1383 if (event) {
1384 gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
1385 if (!gbuf)
1386 return -ENOMEM;
1387 ret = traceprobe_parse_event_name(&event, &group, gbuf,
1388 event - argv[0]);
1389 if (ret)
1390 return -EINVAL;
1391 }
1392
1393 if (!event) {
1394 /*
1395 * Event name rules:
1396 * - For list/wildcard: require explicit [GROUP/]EVENT
1397 * - For single literal: autogenerate symbol__entry/symbol__exit
1398 */
1399 if (list_mode || has_wildcard(symbol)) {
1400 trace_probe_log_err(0, NO_GROUP_NAME);
1401 return -EINVAL;
1402 }
1403 /* Make a new event name */
1404 if (is_tracepoint)
1405 snprintf(ebuf, MAX_EVENT_NAME_LEN, "%s%s",
1406 isdigit(*symbol) ? "_" : "", symbol);
1407 else
1408 snprintf(ebuf, MAX_EVENT_NAME_LEN, "%s__%s", symbol,
1409 is_return ? "exit" : "entry");
1410 sanitize_event_name(ebuf);
1411 event = ebuf;
1412 }
1413
1414 if (is_return)
1415 ctx->flags |= TPARG_FL_RETURN;
1416 else
1417 ctx->flags |= TPARG_FL_FENTRY;
1418
1419 ctx->funcname = NULL;
1420 if (is_tracepoint) {
1421 /* Get tracepoint and lock its module until the end of the registration. */
1422 struct tracepoint *tpoint;
1423
1424 ctx->flags |= TPARG_FL_TPOINT;
1425 mod = NULL;
1426 tpoint = find_tracepoint(symbol, &mod);
1427 if (tpoint) {
1428 sbuf = kmalloc(KSYM_NAME_LEN, GFP_KERNEL);
1429 if (!sbuf)
1430 return -ENOMEM;
1431 ctx->funcname = kallsyms_lookup((unsigned long)tpoint->probestub,
1432 NULL, NULL, NULL, sbuf);
1433 }
1434 }
1435
1436 if (!list_mode && !has_wildcard(symbol) && !is_tracepoint)
1437 ctx->funcname = symbol;
1438
1439 abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
1440 if (!abuf)
1441 return -ENOMEM;
1442 argc -= 2; argv += 2;
1443 new_argv = traceprobe_expand_meta_args(argc, argv, &new_argc,
1444 abuf, MAX_BTF_ARGS_LEN, ctx);
1445 if (IS_ERR(new_argv))
1446 return PTR_ERR(new_argv);
1447 if (new_argv) {
1448 argc = new_argc;
1449 argv = new_argv;
1450 }
1451 if (argc > MAX_TRACE_ARGS) {
1452 trace_probe_log_set_index(2);
1453 trace_probe_log_err(0, TOO_MANY_ARGS);
1454 return -E2BIG;
1455 }
1456
1457 ret = traceprobe_expand_dentry_args(argc, argv, &dbuf);
1458 if (ret)
1459 return ret;
1460
1461 /* setup a probe */
1462 tf = alloc_trace_fprobe(group, event, symbol, argc, is_return, is_tracepoint);
1463 if (IS_ERR(tf)) {
1464 ret = PTR_ERR(tf);
1465 /* This must return -ENOMEM, else there is a bug */
1466 WARN_ON_ONCE(ret != -ENOMEM);
1467 return ret;
1468 }
1469
1470 /* carry list parsing result into tf */
1471 if (!is_tracepoint) {
1472 tf->list_mode = list_mode;
1473 if (parsed_filter) {
1474 tf->filter = kstrdup(parsed_filter, GFP_KERNEL);
1475 if (!tf->filter)
1476 return -ENOMEM;
1477 }
1478 if (parsed_nofilter) {
1479 tf->nofilter = kstrdup(parsed_nofilter, GFP_KERNEL);
1480 if (!tf->nofilter)
1481 return -ENOMEM;
1482 }
1483 }
1484
1485 /* parse arguments */
1486 for (i = 0; i < argc; i++) {
1487 trace_probe_log_set_index(i + 2);
1488 ctx->offset = 0;
1489 ret = traceprobe_parse_probe_arg(&tf->tp, i, argv[i], ctx);
1490 if (ret)
1491 return ret; /* This can be -ENOMEM */
1492 }
1493
1494 if (is_return && tf->tp.entry_arg) {
1495 tf->fp.entry_handler = trace_fprobe_entry_handler;
1496 tf->fp.entry_data_size = traceprobe_get_entry_data_size(&tf->tp);
1497 if (ALIGN(tf->fp.entry_data_size, sizeof(long)) > MAX_FPROBE_DATA_SIZE) {
1498 trace_probe_log_set_index(2);
1499 trace_probe_log_err(0, TOO_MANY_EARGS);
1500 return -E2BIG;
1501 }
1502 }
1503
1504 ret = traceprobe_set_print_fmt(&tf->tp,
1505 is_return ? PROBE_PRINT_RETURN : PROBE_PRINT_NORMAL);
1506 if (ret < 0)
1507 return ret;
1508
1509 ret = register_trace_fprobe_event(tf);
1510 if (ret) {
1511 trace_probe_log_set_index(1);
1512 if (ret == -EILSEQ)
1513 trace_probe_log_err(0, BAD_INSN_BNDRY);
1514 else if (ret == -ENOENT)
1515 trace_probe_log_err(0, BAD_PROBE_ADDR);
1516 else if (ret != -ENOMEM && ret != -EEXIST)
1517 trace_probe_log_err(0, FAIL_REG_PROBE);
1518 return -EINVAL;
1519 }
1520
1521 /* 'tf' is successfully registered. To avoid freeing, assign NULL. */
1522 tf = NULL;
1523
1524 return 0;
1525 }
1526
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Sun, 5 Oct 2025 08:46:57 +0900
Ryan Chung <seokwoo.chung130@gmail.com> wrote:
Please describe what this patch adds, for what reason.
> Signed-off-by: Ryan Chung <seokwoo.chung130@gmail.com>
> ---
> kernel/trace/trace_fprobe.c | 247 ++++++++++++++++++++++++++++--------
> 1 file changed, 192 insertions(+), 55 deletions(-)
>
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index b36ade43d4b3..ec5b6e1c1a1b 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -191,6 +191,9 @@ struct trace_fprobe {
> bool tprobe;
> struct tracepoint_user *tuser;
> struct trace_probe tp;
> + char *filter;
> + char *nofilter;
> + bool list_mode;
> };
>
> static bool is_trace_fprobe(struct dyn_event *ev)
> @@ -203,14 +206,10 @@ static struct trace_fprobe *to_trace_fprobe(struct dyn_event *ev)
> return container_of(ev, struct trace_fprobe, devent);
> }
>
> -/**
> - * for_each_trace_fprobe - iterate over the trace_fprobe list
> - * @pos: the struct trace_fprobe * for each entry
> - * @dpos: the struct dyn_event * to use as a loop cursor
> - */
> -#define for_each_trace_fprobe(pos, dpos) \
> - for_each_dyn_event(dpos) \
> - if (is_trace_fprobe(dpos) && (pos = to_trace_fprobe(dpos)))
Why remove this? This is for finding all fprobes.
> +static struct trace_fprobe *trace_fprobe_from_dyn(struct dyn_event *ev)
> +{
> + return is_trace_fprobe(ev) ? to_trace_fprobe(ev) : NULL;
> +}
>
> static bool trace_fprobe_is_return(struct trace_fprobe *tf)
> {
> @@ -227,6 +226,109 @@ static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
> return tf->symbol ? tf->symbol : "unknown";
> }
>
> +static bool has_wildcard(const char *s)
> +{
> + return s && (strchr(s, '*') || strchr(s, '?'));
> +}
> +
> +static int parse_fprobe_spec(const char *in, bool is_tracepoint,
> + char **base, bool *is_return, bool *list_mode,
> + char **filter, char **nofilter)
> +{
> + const char *p;
> + char *work = NULL;
> + char *b = NULL, *f = NULL, *nf = NULL;
See below (out: label)
> + bool legacy_ret = false;
> + bool list = false;
> + int ret = 0;
nit: sort local variable by line length. (longer to shorter)
> +
> + if (!in || !base || !is_return || !list_mode || !filter || !nofilter)
> + return -EINVAL;
> +
> + *base = NULL; *filter = NULL; *nofilter = NULL;
> + *is_return = false; *list_mode = false;
> +
> + if (is_tracepoint) {
> + if (strchr(in, ',') || strchr(in, ':'))
> + return -EINVAL;
> + if (strstr(in, "%return"))
> + return -EINVAL;
It seems below loop checks all above cases.
> + for (p = in; *p; p++)
> + if (!isalnum(*p) && *p != '_')
> + return -EINVAL;
This only allows that the @in must be a symbol name.
> + b = kstrdup(in, GFP_KERNEL);
> + if (!b)
> + return -ENOMEM;
> + *base = b;
> + return 0;
> + }
> +
> + work = kstrdup(in, GFP_KERNEL);
> + if (!work)
> + return -ENOMEM;
> +
> + p = strstr(work, "%return");
Note that strstr does not care it ends with given string.
> + if (p) {
> + if (!strcmp(p, ":exit")) {
> + *is_return = true;
> + *p = '\0';
> + } else if (!strcmp(p, ":entry")) {
> + *p = '\0';
> + } else {
> + ret = -EINVAL;
> + goto out;
> + }
> + }
> +
> + list = !!strchr(work, ',') || has_wildcard(work);
Wildcard is OK for legacy.
> + if (legacy_ret)
> + *is_return = true;
> +
> + b = kstrdup(work, GFP_KERNEL);
> + if (!b) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + if (list) {
> + char *tmp = b, *tok;
> + size_t fsz = strlen(b) + 1, nfsz = strlen(b) + 1;
size_t fsz, nfsz;
fsz = nfsz = strlen(b) + 1;
> +
> + f = kzalloc(fsz, GFP_KERNEL);
> + nf = kzalloc(nfsz, GFP_KERNEL);
> + if (!f || !nf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + while ((tok = strsep(&tmp, ",")) != NULL) {
> + char *dst;
> + bool neg = (*tok == '!');
> +
> + if (*tok == '\0')
> + continue;
> + if (neg)
> + tok++;
> + dst = neg ? nf : f;
> + if (dst[0] != '\0')
> + strcat(dst, ",");
> + strcat(dst, tok);
> + }
> + *list_mode = true;
> + }
> +
> + *base = b; b = NULL;
> + *filter = f; f = NULL;
> + *nofilter = nf; nf = NULL;
> +
> +out:
> + kfree(work);
> + kfree(b);
> + kfree(f);
> + kfree(nf);
Instead of using goto only for kfree(), use __free(kfree)
to clean those up automatically.
> + return ret;
> +}
> +
> static bool trace_fprobe_is_busy(struct dyn_event *ev)
> {
> struct trace_fprobe *tf = to_trace_fprobe(ev);
> @@ -556,13 +658,17 @@ static void free_trace_fprobe(struct trace_fprobe *tf)
> trace_probe_cleanup(&tf->tp);
> if (tf->tuser)
> tracepoint_user_put(tf->tuser);
> + kfree(tf->filter);
> + kfree(tf->nofilter);
> kfree(tf->symbol);
> kfree(tf);
> }
> }
>
> /* Since alloc_trace_fprobe() can return error, check the pointer is ERR too. */
> -DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) free_trace_fprobe(_T))
> +DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *,
> + if (!IS_ERR_OR_NULL(_T))
> + free_trace_fprobe(_T))
OK, it looks good to clean up. But please do it separated patch.
Do not touch if it is not related to your change.
>
> /*
> * Allocate new trace_probe and initialize it (including fprobe).
> @@ -605,10 +711,16 @@ static struct trace_fprobe *find_trace_fprobe(const char *event,
> struct dyn_event *pos;
> struct trace_fprobe *tf;
>
> - for_each_trace_fprobe(tf, pos)
> + list_for_each_entry(pos, &dyn_event_list, list) {
> + tf = trace_fprobe_from_dyn(pos);
> + if (!tf)
> + continue;
> +
> if (strcmp(trace_probe_name(&tf->tp), event) == 0 &&
> strcmp(trace_probe_group_name(&tf->tp), group) == 0)
> return tf;
> + }
> +
Ditto and there is no need to change.
> return NULL;
> }
>
> @@ -835,7 +947,12 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
> if (trace_fprobe_is_tracepoint(tf))
> return __regsiter_tracepoint_fprobe(tf);
>
> - /* TODO: handle filter, nofilter or symbol list */
> + /* Registration path:
> + * - list_mode: pass filter/nofilter
> + * - single: pass symbol only (legacy)
> + */
> + if (tf->list_mode)
> + return register_fprobe(&tf->fp, tf->filter, tf->nofilter);
> return register_fprobe(&tf->fp, tf->symbol, NULL);
> }
>
> @@ -1114,7 +1231,11 @@ static int __tprobe_event_module_cb(struct notifier_block *self,
> return NOTIFY_DONE;
>
> mutex_lock(&event_mutex);
> - for_each_trace_fprobe(tf, pos) {
> + list_for_each_entry(pos, &dyn_event_list, list) {
> + tf = trace_fprobe_from_dyn(pos);
> + if (!tf)
> + continue;
> +
> /* Skip fprobe and disabled tprobe events. */
> if (!trace_fprobe_is_tracepoint(tf) || !tf->tuser)
> continue;
> @@ -1155,55 +1276,35 @@ static int parse_symbol_and_return(int argc, const char *argv[],
> char **symbol, bool *is_return,
> bool is_tracepoint)
> {
> - char *tmp = strchr(argv[1], '%');
> - int i;
> -
> - if (tmp) {
> - int len = tmp - argv[1];
> -
> - if (!is_tracepoint && !strcmp(tmp, "%return")) {
> - *is_return = true;
> - } else {
> - trace_probe_log_err(len, BAD_ADDR_SUFFIX);
> - return -EINVAL;
> - }
> - *symbol = kmemdup_nul(argv[1], len, GFP_KERNEL);
> - } else
> - *symbol = kstrdup(argv[1], GFP_KERNEL);
> - if (!*symbol)
> - return -ENOMEM;
> -
> - if (*is_return)
> - return 0;
> + int i, ret;
> + bool list_mode = false;
> + char *filter = NULL; *nofilter = NULL;
Sort it as other functions. longer line to shorter.
>
> - if (is_tracepoint) {
> - tmp = *symbol;
> - while (*tmp && (isalnum(*tmp) || *tmp == '_'))
> - tmp++;
> - if (*tmp) {
> - /* find a wrong character. */
> - trace_probe_log_err(tmp - *symbol, BAD_TP_NAME);
> - kfree(*symbol);
> - *symbol = NULL;
> - return -EINVAL;
> - }
> - }
> + ret = parse_fprobe_spec(argv[1], is_tracepoint, symbol, is_return,
> + &list_mode, &filter, &nofilter);
> + if (ret)
> + return ret;
>
> - /* If there is $retval, this should be a return fprobe. */
> for (i = 2; i < argc; i++) {
> - tmp = strstr(argv[i], "$retval");
> + char *tmp = strstr(argv[i], "$retval");
> +
> if (tmp && !isalnum(tmp[7]) && tmp[7] != '_') {
> if (is_tracepoint) {
> trace_probe_log_set_index(i);
> trace_probe_log_err(tmp - argv[i], RETVAL_ON_PROBE);
> kfree(*symbol);
> *symbol = NULL;
> + kfree(filter);
> + kfree(nofilter);
> return -EINVAL;
> }
> *is_return = true;
> break;
> }
> }
> +
> + kfree(filter);
> + kfree(nofilter);
> return 0;
> }
>
> @@ -1247,6 +1348,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> int i, new_argc = 0, ret = 0;
> bool is_tracepoint = false;
> bool is_return = false;
> + bool list_mode = false;
> +
Do not split local variable definitions with empty lines.
> + char *parsed_filter __free(kfree) = NULL;
> + char *parsed_nofilter __free(kfree) = NULL;
> + bool has_wild = false;
Please sort.
>
> if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
> return -ECANCELED;
> @@ -1267,8 +1373,9 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>
> trace_probe_log_set_index(1);
>
> - /* a symbol(or tracepoint) must be specified */
> - ret = parse_symbol_and_return(argc, argv, &symbol, &is_return, is_tracepoint);
> + /* Parse spec early (single vs list, suffix, base symbol) */
> + ret = parse_fprobe_spec(argv[1], is_tracepoint, &symbol, &is_return,
> + &list_mode, &parsed_filter, &parsed_nofilter);
Hmm, if so, where is the parse_symbol_and_return() called?
I think you can pick the $retval search loop from the
parse_symbol_and_return() for updating is_return (or make
it failure if is_tracepoint == true).
> if (ret < 0)
> return -EINVAL;
>
> @@ -1283,10 +1390,16 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> return -EINVAL;
> }
>
> - if (!event) {
> - ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> - if (!ebuf)
> - return -ENOMEM;
> + if (!event) {
> + /*
> + * Event name rules:
> + * - For list/wildcard: require explicit [GROUP/]EVENT
> + * - For single literal: autogenerate symbol__entry/symbol__exit
> + */
nit: to avoid confusing, comment should be indented as same as the
code. Or, put the comment right before the `if`.
> + if (list_mode || has_wildcard(symbol)) {
> + trace_probe_log_err(0, NO_GROUP_NAME);
> + return -EINVAL;
> + }
> /* Make a new event name */
> if (is_tracepoint)
> snprintf(ebuf, MAX_EVENT_NAME_LEN, "%s%s",
> @@ -1319,7 +1432,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> NULL, NULL, NULL, sbuf);
> }
> }
> - if (!ctx->funcname)
> +
> + if (!list_mode && !has_wildcard(symbol) && !is_tracepoint)
> ctx->funcname = symbol;
>
> abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
> @@ -1353,6 +1467,21 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> return ret;
> }
>
> + /* carry list parsing result into tf */
> + if (!is_tracepoint) {
> + tf->list_mode = list_mode;
> + if (parsed_filter) {
> + tf->filter = kstrdup(parsed_filter, GFP_KERNEL);
> + if (!tf->filter)
> + return -ENOMEM;
> + }
> + if (parsed_nofilter) {
> + tf->nofilter = kstrdup(parsed_nofilter, GFP_KERNEL);
> + if (!tf->nofilter)
> + return -ENOMEM;
> + }
> + }
Odd indentation. Please fix.
> +
> /* parse arguments */
> for (i = 0; i < argc; i++) {
> trace_probe_log_set_index(i + 2);
> @@ -1439,8 +1568,16 @@ static int trace_fprobe_show(struct seq_file *m, struct dyn_event *ev)
> seq_printf(m, ":%s/%s", trace_probe_group_name(&tf->tp),
> trace_probe_name(&tf->tp));
>
> - seq_printf(m, " %s%s", trace_fprobe_symbol(tf),
> - trace_fprobe_is_return(tf) ? "%return" : "");
> + seq_printf(m, "%s", trace_fprobe_symbol(tf));
> + if (!trace_fprobe_is_tracepoint(tf)) {
> + if (tf->list_mode) {
> + if (trace_fprobe_is_return(tf))
> + seq_puts(m, ":exit");
In both cases, we can use ":exit" suffix. This means we will
accept legacy "%return" for backward compatibility, but
shows ":exit" always.
> + } else {
> + if (trace_fprobe_is_return(tf))
> + seq_puts(m, "%return");
> + }
> + }
>
> for (i = 0; i < tf->tp.nr_args; i++)
> seq_printf(m, " %s=%s", tf->tp.args[i].name, tf->tp.args[i].comm);
> --
> 2.43.0
>
Thank you,
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
Hi. Thanks for the quick review and comments. Here are my responses to
the remaining points.
On Wed, Oct 08, 2025 at 07:09:37PM +0900, Masami Hiramatsu wrote:
> On Sun, 5 Oct 2025 08:46:57 +0900
> Ryan Chung <seokwoo.chung130@gmail.com> wrote:
>
> Please describe what this patch adds, for what reason.
>
This is my mistake; I forgot to do so. I will make sure to include it
next time.
> > Signed-off-by: Ryan Chung <seokwoo.chung130@gmail.com>
> > ---
> > kernel/trace/trace_fprobe.c | 247 ++++++++++++++++++++++++++++--------
> > 1 file changed, 192 insertions(+), 55 deletions(-)
> >
> > diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> > index b36ade43d4b3..ec5b6e1c1a1b 100644
> > --- a/kernel/trace/trace_fprobe.c
> > +++ b/kernel/trace/trace_fprobe.c
> > @@ -191,6 +191,9 @@ struct trace_fprobe {
> > bool tprobe;
> > struct tracepoint_user *tuser;
> > struct trace_probe tp;
> > + char *filter;
> > + char *nofilter;
> > + bool list_mode;
> > };
> >
> > static bool is_trace_fprobe(struct dyn_event *ev)
> > @@ -203,14 +206,10 @@ static struct trace_fprobe *to_trace_fprobe(struct dyn_event *ev)
> > return container_of(ev, struct trace_fprobe, devent);
> > }
> >
> > -/**
> > - * for_each_trace_fprobe - iterate over the trace_fprobe list
> > - * @pos: the struct trace_fprobe * for each entry
> > - * @dpos: the struct dyn_event * to use as a loop cursor
> > - */
> > -#define for_each_trace_fprobe(pos, dpos) \
> > - for_each_dyn_event(dpos) \
> > - if (is_trace_fprobe(dpos) && (pos = to_trace_fprobe(dpos)))
>
> Why remove this? This is for finding all fprobes.
>
I will revert this and keep for_each_trace_fprobe as is.
> > +static struct trace_fprobe *trace_fprobe_from_dyn(struct dyn_event *ev)
> > +{
> > + return is_trace_fprobe(ev) ? to_trace_fprobe(ev) : NULL;
> > +}
> >
> > static bool trace_fprobe_is_return(struct trace_fprobe *tf)
> > {
> > @@ -227,6 +226,109 @@ static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
> > return tf->symbol ? tf->symbol : "unknown";
> > }
> >
> > +static bool has_wildcard(const char *s)
> > +{
> > + return s && (strchr(s, '*') || strchr(s, '?'));
> > +}
> > +
> > +static int parse_fprobe_spec(const char *in, bool is_tracepoint,
> > + char **base, bool *is_return, bool *list_mode,
> > + char **filter, char **nofilter)
> > +{
> > + const char *p;
> > + char *work = NULL;
> > + char *b = NULL, *f = NULL, *nf = NULL;
>
> See below (out: label)
>
I will switch those temporaries to __free(kfree) and drop the goto that
existed only to kfree. This addresses the cleanup pattern comment.
> > + bool legacy_ret = false;
> > + bool list = false;
> > + int ret = 0;
>
> nit: sort local variable by line length. (longer to shorter)
>
Ok. I will sort locals longest -> shortest and fix a few initializations
(char *filter = NULL, char *nofilter = Null;).
> > +
> > + if (!in || !base || !is_return || !list_mode || !filter || !nofilter)
> > + return -EINVAL;
> > +
> > + *base = NULL; *filter = NULL; *nofilter = NULL;
> > + *is_return = false; *list_mode = false;
> > +
> > + if (is_tracepoint) {
> > + if (strchr(in, ',') || strchr(in, ':'))
> > + return -EINVAL;
> > + if (strstr(in, "%return"))
> > + return -EINVAL;
>
> It seems below loop checks all above cases.
>
I will remove the redundant pre-checks and rely on the validation loop,
with precise rules.
> > + for (p = in; *p; p++)
> > + if (!isalnum(*p) && *p != '_')
> > + return -EINVAL;
>
> This only allows that the @in must be a symbol name.
>
Just to clarify: should tracepoint arguments support the subsystem:event
format (e.g., "sched:sched_switch"), or should they remain restricted to
simple symbol names only? The current validation enforces
symbol-name-only, but I wanted to confirm this is the intended behavior
before next version.
> > + b = kstrdup(in, GFP_KERNEL);
> > + if (!b)
> > + return -ENOMEM;
> > + *base = b;
> > + return 0;
> > + }
> > +
> > + work = kstrdup(in, GFP_KERNEL);
> > + if (!work)
> > + return -ENOMEM;
> > +
> > + p = strstr(work, "%return");
>
> Note that strstr does not care it ends with given string.
>
Good catch. I will replace it with explicit end-of-string checks so we
accept only a single terminal suffix: %return, :entry, or :exit.
Partial/embedded matches will be rejected.
> > + if (p) {
> > + if (!strcmp(p, ":exit")) {
> > + *is_return = true;
> > + *p = '\0';
> > + } else if (!strcmp(p, ":entry")) {
> > + *p = '\0';
> > + } else {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > + }
> > +
> > + list = !!strchr(work, ',') || has_wildcard(work);
>
> Wildcard is OK for legacy.
>
I will keep the wildcard acceptance for the legacy string, and treat
presence of "," or wildcard as "list mode" that builds filter/nofilter
for register_fprobe(); otherwise it remains single-symbol legacy.
> > + if (legacy_ret)
> > + *is_return = true;
> > +
> > + b = kstrdup(work, GFP_KERNEL);
> > + if (!b) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + if (list) {
> > + char *tmp = b, *tok;
> > + size_t fsz = strlen(b) + 1, nfsz = strlen(b) + 1;
>
> size_t fsz, nfsz;
>
> fsz = nfsz = strlen(b) + 1;
>
I will adopt the above style.
> > +
> > + f = kzalloc(fsz, GFP_KERNEL);
> > + nf = kzalloc(nfsz, GFP_KERNEL);
> > + if (!f || !nf) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + while ((tok = strsep(&tmp, ",")) != NULL) {
> > + char *dst;
> > + bool neg = (*tok == '!');
> > +
> > + if (*tok == '\0')
> > + continue;
> > + if (neg)
> > + tok++;
> > + dst = neg ? nf : f;
> > + if (dst[0] != '\0')
> > + strcat(dst, ",");
> > + strcat(dst, tok);
> > + }
> > + *list_mode = true;
> > + }
> > +
> > + *base = b; b = NULL;
> > + *filter = f; f = NULL;
> > + *nofilter = nf; nf = NULL;
> > +
> > +out:
> > + kfree(work);
> > + kfree(b);
> > + kfree(f);
> > + kfree(nf);
>
> Instead of using goto only for kfree(), use __free(kfree)
> to clean those up automatically.
>
Ok. As mentioned above, I will convert all such temporaries to
__free(kfree) and remove the goto cleanup.
> > + return ret;
> > +}
> > +
> > static bool trace_fprobe_is_busy(struct dyn_event *ev)
> > {
> > struct trace_fprobe *tf = to_trace_fprobe(ev);
> > @@ -556,13 +658,17 @@ static void free_trace_fprobe(struct trace_fprobe *tf)
> > trace_probe_cleanup(&tf->tp);
> > if (tf->tuser)
> > tracepoint_user_put(tf->tuser);
> > + kfree(tf->filter);
> > + kfree(tf->nofilter);
> > kfree(tf->symbol);
> > kfree(tf);
> > }
> > }
> >
> > /* Since alloc_trace_fprobe() can return error, check the pointer is ERR too. */
> > -DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) free_trace_fprobe(_T))
> > +DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *,
> > + if (!IS_ERR_OR_NULL(_T))
> > + free_trace_fprobe(_T))
>
> OK, it looks good to clean up. But please do it separated patch.
> Do not touch if it is not related to your change.
>
Do you want this to be in a separate series or for this patch series?
> >
> > /*
> > * Allocate new trace_probe and initialize it (including fprobe).
> > @@ -605,10 +711,16 @@ static struct trace_fprobe *find_trace_fprobe(const char *event,
> > struct dyn_event *pos;
> > struct trace_fprobe *tf;
> >
> > - for_each_trace_fprobe(tf, pos)
> > + list_for_each_entry(pos, &dyn_event_list, list) {
> > + tf = trace_fprobe_from_dyn(pos);
> > + if (!tf)
> > + continue;
> > +
> > if (strcmp(trace_probe_name(&tf->tp), event) == 0 &&
> > strcmp(trace_probe_group_name(&tf->tp), group) == 0)
> > return tf;
> > + }
> > +
>
> Ditto and there is no need to change.
>
Ok. I will revert those sites to the existing macro-based iteration.
> > return NULL;
> > }
> >
> > @@ -835,7 +947,12 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
> > if (trace_fprobe_is_tracepoint(tf))
> > return __regsiter_tracepoint_fprobe(tf);
> >
> > - /* TODO: handle filter, nofilter or symbol list */
> > + /* Registration path:
> > + * - list_mode: pass filter/nofilter
> > + * - single: pass symbol only (legacy)
> > + */
> > + if (tf->list_mode)
> > + return register_fprobe(&tf->fp, tf->filter, tf->nofilter);
> > return register_fprobe(&tf->fp, tf->symbol, NULL);
> > }
> >
> > @@ -1114,7 +1231,11 @@ static int __tprobe_event_module_cb(struct notifier_block *self,
> > return NOTIFY_DONE;
> >
> > mutex_lock(&event_mutex);
> > - for_each_trace_fprobe(tf, pos) {
> > + list_for_each_entry(pos, &dyn_event_list, list) {
> > + tf = trace_fprobe_from_dyn(pos);
> > + if (!tf)
> > + continue;
> > +
> > /* Skip fprobe and disabled tprobe events. */
> > if (!trace_fprobe_is_tracepoint(tf) || !tf->tuser)
> > continue;
> > @@ -1155,55 +1276,35 @@ static int parse_symbol_and_return(int argc, const char *argv[],
> > char **symbol, bool *is_return,
> > bool is_tracepoint)
> > {
> > - char *tmp = strchr(argv[1], '%');
> > - int i;
> > -
> > - if (tmp) {
> > - int len = tmp - argv[1];
> > -
> > - if (!is_tracepoint && !strcmp(tmp, "%return")) {
> > - *is_return = true;
> > - } else {
> > - trace_probe_log_err(len, BAD_ADDR_SUFFIX);
> > - return -EINVAL;
> > - }
> > - *symbol = kmemdup_nul(argv[1], len, GFP_KERNEL);
> > - } else
> > - *symbol = kstrdup(argv[1], GFP_KERNEL);
> > - if (!*symbol)
> > - return -ENOMEM;
> > -
> > - if (*is_return)
> > - return 0;
> > + int i, ret;
> > + bool list_mode = false;
> > + char *filter = NULL; *nofilter = NULL;
>
> Sort it as other functions. longer line to shorter.
>
I did not know this. I will fix the ordering (and see next item about
the function's role).
> >
> > - if (is_tracepoint) {
> > - tmp = *symbol;
> > - while (*tmp && (isalnum(*tmp) || *tmp == '_'))
> > - tmp++;
> > - if (*tmp) {
> > - /* find a wrong character. */
> > - trace_probe_log_err(tmp - *symbol, BAD_TP_NAME);
> > - kfree(*symbol);
> > - *symbol = NULL;
> > - return -EINVAL;
> > - }
> > - }
> > + ret = parse_fprobe_spec(argv[1], is_tracepoint, symbol, is_return,
> > + &list_mode, &filter, &nofilter);
> > + if (ret)
> > + return ret;
> >
> > - /* If there is $retval, this should be a return fprobe. */
> > for (i = 2; i < argc; i++) {
> > - tmp = strstr(argv[i], "$retval");
> > + char *tmp = strstr(argv[i], "$retval");
> > +
> > if (tmp && !isalnum(tmp[7]) && tmp[7] != '_') {
> > if (is_tracepoint) {
> > trace_probe_log_set_index(i);
> > trace_probe_log_err(tmp - argv[i], RETVAL_ON_PROBE);
> > kfree(*symbol);
> > *symbol = NULL;
> > + kfree(filter);
> > + kfree(nofilter);
> > return -EINVAL;
> > }
> > *is_return = true;
> > break;
> > }
> > }
> > +
> > + kfree(filter);
> > + kfree(nofilter);
> > return 0;
> > }
> >
> > @@ -1247,6 +1348,11 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> > int i, new_argc = 0, ret = 0;
> > bool is_tracepoint = false;
> > bool is_return = false;
> > + bool list_mode = false;
> > +
>
> Do not split local variable definitions with empty lines.
>
I will collapse those blocks.
> > + char *parsed_filter __free(kfree) = NULL;
> > + char *parsed_nofilter __free(kfree) = NULL;
> > + bool has_wild = false;
>
> Please sort.
>
I will sort and group them, so no empty line splits.
> >
> > if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
> > return -ECANCELED;
> > @@ -1267,8 +1373,9 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >
> > trace_probe_log_set_index(1);
> >
> > - /* a symbol(or tracepoint) must be specified */
> > - ret = parse_symbol_and_return(argc, argv, &symbol, &is_return, is_tracepoint);
> > + /* Parse spec early (single vs list, suffix, base symbol) */
> > + ret = parse_fprobe_spec(argv[1], is_tracepoint, &symbol, &is_return,
> > + &list_mode, &parsed_filter, &parsed_nofilter);
>
> Hmm, if so, where is the parse_symbol_and_return() called?
> I think you can pick the $retval search loop from the
> parse_symbol_and_return() for updating is_return (or make
> it failure if is_tracepoint == true).
>
Makes sense. I will fold the $retval scan into the new parser so there's
a single source of truth. $retval will remain rejected for tracepoints
with a proper error index. parse_symbol_and_return() can then be removed
or turned into a thin wrapper if still referenced.
> > if (ret < 0)
> > return -EINVAL;
> >
> > @@ -1283,10 +1390,16 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> > return -EINVAL;
> > }
> >
> > - if (!event) {
> > - ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> > - if (!ebuf)
> > - return -ENOMEM;
> > + if (!event) {
> > + /*
> > + * Event name rules:
> > + * - For list/wildcard: require explicit [GROUP/]EVENT
> > + * - For single literal: autogenerate symbol__entry/symbol__exit
> > + */
>
> nit: to avoid confusing, comment should be indented as same as the
> code. Or, put the comment right before the `if`.
>
I will move the comment above the if and align indentation.
> > + if (list_mode || has_wildcard(symbol)) {
> > + trace_probe_log_err(0, NO_GROUP_NAME);
> > + return -EINVAL;
> > + }
> > /* Make a new event name */
> > if (is_tracepoint)
> > snprintf(ebuf, MAX_EVENT_NAME_LEN, "%s%s",
> > @@ -1319,7 +1432,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> > NULL, NULL, NULL, sbuf);
> > }
> > }
> > - if (!ctx->funcname)
> > +
> > + if (!list_mode && !has_wildcard(symbol) && !is_tracepoint)
> > ctx->funcname = symbol;
> >
> > abuf = kmalloc(MAX_BTF_ARGS_LEN, GFP_KERNEL);
> > @@ -1353,6 +1467,21 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> > return ret;
> > }
> >
> > + /* carry list parsing result into tf */
> > + if (!is_tracepoint) {
> > + tf->list_mode = list_mode;
> > + if (parsed_filter) {
> > + tf->filter = kstrdup(parsed_filter, GFP_KERNEL);
> > + if (!tf->filter)
> > + return -ENOMEM;
> > + }
> > + if (parsed_nofilter) {
> > + tf->nofilter = kstrdup(parsed_nofilter, GFP_KERNEL);
> > + if (!tf->nofilter)
> > + return -ENOMEM;
> > + }
> > + }
>
> Odd indentation. Please fix.
>
My mistake. I will fix the indentation here.
> > +
> > /* parse arguments */
> > for (i = 0; i < argc; i++) {
> > trace_probe_log_set_index(i + 2);
> > @@ -1439,8 +1568,16 @@ static int trace_fprobe_show(struct seq_file *m, struct dyn_event *ev)
> > seq_printf(m, ":%s/%s", trace_probe_group_name(&tf->tp),
> > trace_probe_name(&tf->tp));
> >
> > - seq_printf(m, " %s%s", trace_fprobe_symbol(tf),
> > - trace_fprobe_is_return(tf) ? "%return" : "");
> > + seq_printf(m, "%s", trace_fprobe_symbol(tf));
> > + if (!trace_fprobe_is_tracepoint(tf)) {
> > + if (tf->list_mode) {
> > + if (trace_fprobe_is_return(tf))
> > + seq_puts(m, ":exit");
>
> In both cases, we can use ":exit" suffix. This means we will
> accept legacy "%return" for backward compatibility, but
> shows ":exit" always.
>
I will make show always print :exit for return probes, regardless of the
input form, and never print %return.
> > + } else {
> > + if (trace_fprobe_is_return(tf))
> > + seq_puts(m, "%return");
> > + }
> > + }
> >
> > for (i = 0; i < tf->tp.nr_args; i++)
> > seq_printf(m, " %s=%s", tf->tp.args[i].name, tf->tp.args[i].comm);
> > --
> > 2.43.0
> >
>
> Thank you,
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
© 2016 - 2025 Red Hat, Inc.