Re: [PATCH v6 1/4] tracing/fprobe: Support comma-separated symbols and :entry/:exit

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Thu,  5 Feb 2026 08:58:39 -0500
"Seokwoo Chung (Ryan)" <seokwoo.chung130@gmail.com> wrote:

> Extend the fprobe event interface to support:
>   - Comma-separated symbol lists: "func1,func2,func3"
>   - Exclusion prefix: "func1,!func2,func3"
>   - Explicit :entry and :exit suffixes (replacing %return for lists)
> 
> Single-symbol probes retain backward compatibility with %return.
> 
> The list parsing is factored into a dedicated parse_fprobe_list()
> helper that splits comma-separated input into filter (included) and
> nofilter (excluded) strings.  Tracepoint validation now reports the
> error position via trace_probe_log_err() so users can see what went
> wrong in tracefs/error_log.
> 
> Changes since v5:
>   - Fix missing closing brace in the empty-token check that caused a
>     build error.
>   - Remove redundant strchr/strstr checks for tracepoint validation
>     (the character validation loop already rejects ',', ':', and '%').
>   - Add trace_probe_log_err() to the tracepoint character validation
>     loop per reviewer feedback.
>   - Remove unnecessary braces around single-statement if per kernel
>     coding style.
>   - Extract list parsing into parse_fprobe_list() per reviewer feedback
>     to keep parse_fprobe_spec() focused.


Thanks for updating! I have some comments below.

> 
> Update tracefs/README to reflect the new syntax.
> 
> Signed-off-by: Seokwoo Chung (Ryan) <seokwoo.chung130@gmail.com>
> ---
>  kernel/trace/trace.c        |   3 +-
>  kernel/trace/trace_fprobe.c | 219 ++++++++++++++++++++++++++++--------
>  2 files changed, 174 insertions(+), 48 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 8bd4ec08fb36..649a6e6021b4 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -5578,7 +5578,8 @@ static const char readme_msg[] =
>  	"\t           r[maxactive][:[<group>/][<event>]] <place> [<args>]\n"
>  #endif
>  #ifdef CONFIG_FPROBE_EVENTS
> -	"\t           f[:[<group>/][<event>]] <func-name>[%return] [<args>]\n"
> +	"\t           f[:[<group>/][<event>]] <func-name>[:entry|:exit] [<args>]\n"
> +	"\t		(single symbols still accept %return)\n"
>  	"\t           t[:[<group>/][<event>]] <tracepoint> [<args>]\n"
>  #endif
>  #ifdef CONFIG_HIST_TRIGGERS
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index 262c0556e4af..f8846cd1d020 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -187,11 +187,14 @@ DEFINE_FREE(tuser_put, struct tracepoint_user *,
>   */
>  struct trace_fprobe {
>  	struct dyn_event	devent;
> +	char			*filter;

Could you move this filter to the previous line of nofilter?

>  	struct fprobe		fp;
> +	bool			list_mode;


This list_mode seems an alias of (filter || nofilter). In this case, 
we don't need trace_fprobe::list_mode. Please remove this field.


> +	char			*nofilter;

>  	const char		*symbol;
> +	struct trace_probe	tp;

tp must be the last field. Please do not move.

>  	bool			tprobe;
>  	struct tracepoint_user	*tuser;
> -	struct trace_probe	tp;
>  };
>  
>  static bool is_trace_fprobe(struct dyn_event *ev)
> @@ -559,6 +562,8 @@ 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);
>  	}
> @@ -838,7 +843,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);
>  }
>  
> @@ -1154,60 +1164,131 @@ static struct notifier_block tprobe_event_module_nb = {
>  };
>  #endif /* CONFIG_MODULES */
>  
> -static int parse_symbol_and_return(int argc, const char *argv[],
> -				   char **symbol, bool *is_return,
> -				   bool is_tracepoint)
> +static bool has_wildcard(const char *s)
>  {
> -	char *tmp = strchr(argv[1], '%');
> -	int i;
> +	return s && (strchr(s, '*') || strchr(s, '?'));
> +}
>  
> -	if (tmp) {
> -		int len = tmp - argv[1];
> +static int parse_fprobe_list(char *b, char **filter, char **nofilter)
> +{
> +	char *f __free(kfree) = NULL;
> +	char *nf __free(kfree) = NULL;
> +	char *tmp = b, *tok;
> +	size_t sz;
>  
> -		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)
> +	sz = strlen(b) + 1;
> +
> +	f = kzalloc(sz, GFP_KERNEL);
> +	nf = kzalloc(sz, GFP_KERNEL);
> +	if (!f || !nf)
>  		return -ENOMEM;
>  
> -	if (*is_return)
> -		return 0;
> +	while ((tok = strsep(&tmp, ",")) != NULL) {
> +		char *dst;
> +		bool neg = (*tok == '!');
>  
> -	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;
> +		if (*tok == '\0') {
> +			trace_probe_log_err(tmp - b - 1, BAD_TP_NAME);
>  			return -EINVAL;
>  		}
> +
> +		if (neg)
> +			tok++;
> +		dst = neg ? nf : f;
> +		if (dst[0] != '\0')
> +			strcat(dst, ",");
> +		strcat(dst, tok);
>  	}
>  
> -	/* If there is $retval, this should be a return fprobe. */
> -	for (i = 2; i < argc; i++) {
> -		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;
> +	*filter = no_free_ptr(f);
> +	*nofilter = no_free_ptr(nf);
> +
> +	return 0;
> +}
> +
> +static int parse_fprobe_spec(const char *in, bool is_tracepoint,
> +		char **base, bool *is_return, bool *list_mode,
> +		char **filter, char **nofilter)
> +{
> +	char *work __free(kfree) = NULL;
> +	char *b __free(kfree) = NULL;
> +	char *f __free(kfree) = NULL;
> +	char *nf __free(kfree) = NULL;
> +	bool legacy_ret = false;
> +	bool list = false;
> +	const char *p;
> +	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) {
> +		for (p = in; *p; p++)
> +			if (!isalnum(*p) && *p != '_') {
> +				trace_probe_log_err(p - in, BAD_TP_NAME);
> +				return -EINVAL;
> +			}
> +		b = kstrdup(in, GFP_KERNEL);
> +		if (!b)
> +			return -ENOMEM;
> +		*base = no_free_ptr(b);
> +		return 0;
> +	}
> +
> +	work = kstrdup(in, GFP_KERNEL);
> +	if (!work)
> +		return -ENOMEM;
> +
> +	p = strstr(work, "%return");
> +	if (p && p[7] == '\0') {
> +		*is_return = true;
> +		legacy_ret = true;
> +		*(char *)p = '\0';
> +	} else {
> +		/*
> +		 * If "symbol:entry" or "symbol:exit" is given, it is new
> +		 * style probe.
> +		 */
> +		p = strrchr(work, ':');
> +		if (p) {
> +			if (!strcmp(p, ":exit")) {
> +				*is_return = true;
> +				*(char *)p = '\0';
> +			} else if (!strcmp(p, ":entry")) {
> +				*(char *)p = '\0';
> +			} else {
>  				return -EINVAL;
>  			}
> -			*is_return = true;
> -			break;
>  		}
>  	}
> -	return 0;
> +
> +	list = !!strchr(work, ',');
> +	

Here is a needless whitespace (tab) above line.

> +	if (list && legacy_ret)
> +		return -EINVAL;
> +
> +	if (legacy_ret)
> +		*is_return = true;
> +
> +	b = kstrdup(work, GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +
> +	if (list) {
> +		ret = parse_fprobe_list(b, &f, &nf);
> +		if (ret)
> +			return ret;
> +		*list_mode = true;
> +	}
> +
> +	*base = no_free_ptr(b);
> +	*filter = no_free_ptr(f);
> +	*nofilter = no_free_ptr(nf);
> +
> +	return ret;
>  }
>  
>  static int trace_fprobe_create_internal(int argc, const char *argv[],
> @@ -1241,6 +1322,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	const char *event = NULL, *group = FPROBE_EVENT_SYSTEM;
>  	struct module *mod __free(module_put) = NULL;
>  	const char **new_argv __free(kfree) = NULL;
> +	char *parsed_nofilter __free(kfree) = NULL;
> +	char *parsed_filter __free(kfree) = NULL;

nit: we should make those as filter/nofilter simply.

>  	char *symbol __free(kfree) = NULL;
>  	char *ebuf __free(kfree) = NULL;
>  	char *gbuf __free(kfree) = NULL;
> @@ -1249,6 +1332,7 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	char *dbuf __free(kfree) = NULL;
>  	int i, new_argc = 0, ret = 0;
>  	bool is_tracepoint = false;
> +	bool list_mode = false;

Then, we don't need this list_mode. we can replace it with "(filter || nofilter)".

>  	bool is_return = false;
>  
>  	if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
> @@ -1270,11 +1354,26 @@ 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;
>  
> +	for (i = 2; i < argc; i++) {
> +		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);
> +				return -EINVAL;
> +			}
> +			is_return = true;
> +			break;
> +		}
> +	}

Please do this $retval check in parse_fprobe_spec() as parse_symbol_and_return() did.

> +
>  	trace_probe_log_set_index(0);
>  	if (event) {
>  		gbuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
> @@ -1287,6 +1386,15 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	}
>  
>  	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);

 NO_EVENT_NAME error?

> +			return -EINVAL;
> +		}
>  		ebuf = kmalloc(MAX_EVENT_NAME_LEN, GFP_KERNEL);
>  		if (!ebuf)
>  			return -ENOMEM;
> @@ -1322,7 +1430,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);
> @@ -1356,6 +1465,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;
> +		}
> +	}

Also, it is natural to do this inside alloc_trace_fprobe(). Can you pass
filter and nofilter to alloc_trace_fprobe()?
(and just make those NULL instead of using kstrdup()?)

Thank you,


> +
>  	/* parse arguments */
>  	for (i = 0; i < argc; i++) {
>  		trace_probe_log_set_index(i + 2);
> @@ -1442,8 +1566,9 @@ 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) && trace_fprobe_is_return(tf))
> +		seq_puts(m, ":exit");
>  
>  	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
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>