| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20260120155047.4ef3c378@gandalf.local.home>
Date: Tue, 20 Jan 2026 15:50:47 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: "Seokwoo Chung (Ryan)" <seokwoo.chung130@...il.com>
Cc: mhiramat@...nel.org, corbet@....net, shuah@...nel.org,
mathieu.desnoyers@...icios.com, linux-kernel@...r.kernel.org,
linux-trace-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v5 1/3] tracing/fprobe: Support comma-separated symbols
and :entry/:exit
On Wed, 14 Jan 2026 17:13:38 -0500
"Seokwoo Chung (Ryan)" <seokwoo.chung130@...il.com> wrote:
Missing change log. The subject is the "what" the patch is doing, the
change log needs the "why".
> Signed-off-by: Seokwoo Chung (Ryan) <seokwoo.chung130@...il.com>
> ---
> kernel/trace/trace.c | 4 +--
> kernel/trace/trace_fprobe.c | 49 ++++++++++++++++++++-----------------
> kernel/trace/trace_probe.h | 2 ++
> 3 files changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 10cdcc7b194e..73b59d47dfe7 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -5578,8 +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>[:entry|:exit] [<args>]\n"
> - "\t (single symbols still accept %return)\n"
> + "\t f[:[<group>/][<event>]] <func-name>[%return] [<args>]\n"
> + "\t f[:[<group>/][<event>]] <func-list>[:entry|:exit] [<args>]\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 5a2a41eea603..1663c341ddb4 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -187,16 +187,23 @@ DEFINE_FREE(tuser_put, struct tracepoint_user *,
> */
> struct trace_fprobe {
> struct dyn_event devent;
> - char *filter;
> +
> struct fprobe fp;
> - bool list_mode;
> - char *nofilter;
> const char *symbol;
> - struct trace_probe tp;
> + char *filter;
> + char *nofilter;
> +
> bool tprobe;
> struct tracepoint_user *tuser;
> +
> + struct trace_probe tp;
Why is this being updated? Nothing in the change log says why?
> };
>
> +static inline bool trace_fprobe_has_list(const struct trace_fprobe *tf)
> +{
> + return tf->filter || tf->nofilter;
> +}
> +
> static bool is_trace_fprobe(struct dyn_event *ev)
> {
> return ev->ops == &trace_fprobe_ops;
> @@ -847,7 +854,7 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
> * - list_mode: pass filter/nofilter
> * - single: pass symbol only (legacy)
> */
> - if (tf->list_mode)
> + if (trace_fprobe_has_list(tf))
> return register_fprobe(&tf->fp, tf->filter, tf->nofilter);
> return register_fprobe(&tf->fp, tf->symbol, NULL);
> }
> @@ -1188,11 +1195,18 @@ static int parse_fprobe_spec(const char *in, bool is_tracepoint,
> *base = NULL; *filter = NULL; *nofilter = NULL;
> *is_return = false; *list_mode = false;
>
> - if (is_tracepoint) {
> + if (is_tracepoint)
> + {
> if (strchr(in, ',') || strchr(in, ':'))
> + {
> + trace_probe_log_err(0, BAD_TP_NAME);
> return -EINVAL;
> + }
> if (strstr(in, "%return"))
> + {
> + trace_probe_log_err(p - in, BAD_TP_NAME);
> return -EINVAL;
> + }
> for (p = in; *p; p++)
> if (!isalnum(*p) && *p != '_')
> return -EINVAL;
> @@ -1225,6 +1239,7 @@ static int parse_fprobe_spec(const char *in, bool is_tracepoint,
> } else if (!strcmp(p, ":entry")) {
> *(char *)p = '\0';
> } else {
> + trace_probe_log_err(p - work, BAD_LIST_SUFFIX);
> return -EINVAL;
> }
> }
> @@ -1233,6 +1248,7 @@ static int parse_fprobe_spec(const char *in, bool is_tracepoint,
> list = !!strchr(work, ',');
>
> if (list && legacy_ret) {
> + trace_probe_log_err(p - work, BAD_LEGACY_RET);
> return -EINVAL;
> }
>
> @@ -1245,12 +1261,9 @@ static int parse_fprobe_spec(const char *in, bool is_tracepoint,
>
> if (list) {
> char *tmp = b, *tok;
> - size_t fsz, nfsz;
>
> - fsz = nfsz = strlen(b) + 1;
> -
> - f = kzalloc(fsz, GFP_KERNEL);
> - nf = kzalloc(nfsz, GFP_KERNEL);
> + f = kzalloc(strlen(b) + 1, GFP_KERNEL);
> + nf = kzalloc(strlen(b) + 1, GFP_KERNEL);
> if (!f || !nf)
> return -ENOMEM;
>
> @@ -1261,6 +1274,7 @@ static int parse_fprobe_spec(const char *in, bool is_tracepoint,
> if (*tok == '\0') {
> trace_probe_log_err(tmp - b - 1, BAD_TP_NAME);
> return -EINVAL;
> + }
>
> if (neg)
> tok++;
> @@ -1455,17 +1469,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>
> /* 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;
> - }
> + tf->filter = no_free_ptr(parsed_filter);
> + tf->nofilter = no_free_ptr(parsed_nofilter);
> }
>
> /* parse arguments */
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 9fc56c937130..b8b0544eb7cd 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -494,9 +494,11 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
> C(BAD_PROBE_ADDR, "Invalid probed address or symbol"), \
> C(NON_UNIQ_SYMBOL, "The symbol is not unique"), \
> C(BAD_RETPROBE, "Retprobe address must be an function entry"), \
> + C(BAD_LEGACY_RET, "Legacy %return not allowed with list"), \
> C(NO_TRACEPOINT, "Tracepoint is not found"), \
> C(BAD_TP_NAME, "Invalid character in tracepoint name"),\
> C(BAD_ADDR_SUFFIX, "Invalid probed address suffix"), \
> + C(BAD_LIST_SUFFIX, "Bad list suffix"), \
> C(NO_GROUP_NAME, "Group name is not specified"), \
> C(GROUP_TOO_LONG, "Group name is too long"), \
> C(BAD_GROUP_NAME, "Group name must follow the same rules as C identifiers"), \
This definitely needs discussion on why this is needed.
-- Steve
Powered by blists - more mailing lists