Re: [PATCH v9.1 05/11] tracing/probes: Move event parameter fetching code to common parser

Steven Rostedt
2023-05-05T21:41:23Z

On Tue, 2 May 2023 11:18:03 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -283,27 +283,53 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
> return 0;
> }
>
> +static int parse_trace_event_arg(char *arg, struct fetch_insn *code,
> + struct traceprobe_parse_context *ctx)
> +{
> + struct ftrace_event_field *field;
> + struct list_head *head;
> +
> + head = trace_get_fields(ctx->event);
> + list_for_each_entry(field, head, link) {
> + if (!strcmp(arg, field->name)) {
> + code->op = FETCH_OP_TP_ARG;
> + code->data = field;
> + return 0;
> + }
> + }
> + return -ENOENT;
> +}
> +
> #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
>
> static int parse_probe_vars(char *arg, const struct fetch_type *t,
> - struct fetch_insn *code, unsigned int flags, int offs)
> + struct fetch_insn *code,
> + struct traceprobe_parse_context *ctx)
> {
> unsigned long param;
> int ret = 0;
> int len;
>
> - if (flags & TPARG_FL_TEVENT) {
> + if (ctx->flags & TPARG_FL_TEVENT) {
> if (code->data)
> return -EFAULT;
> - code->data = kstrdup(arg, GFP_KERNEL);
> - if (!code->data)
> - return -ENOMEM;
> - code->op = FETCH_OP_TP_ARG;
> - } else if (strcmp(arg, "retval") == 0) {
> - if (flags & TPARG_FL_RETURN) {
> + ret = parse_trace_event_arg(arg, code, ctx);
> + if (!ret)
> + return 0;
> + if (strcmp(arg, "comm") == 0 || strcmp(arg, "COMM") == 0) {
> + code->op = FETCH_OP_COMM;
> + return 0;
> + }
> + /* backward compatibility */
> + ctx->offset = 0;
> + goto inval_var;
> + }

So this is a bit inconsistent in this function. We have here an if
statement that returns 0 on success, and jumps to inval_var if it reaches
the end.

The rest of the if statements below, also goes to inval_var, or returns
error, or just falls through the if statement to return ret. It's somewhat
random.

This should be cleaned up (see patch at the end).

> +
> + if (strcmp(arg, "retval") == 0) {
> + if (ctx->flags & TPARG_FL_RETURN) {
> code->op = FETCH_OP_RETVAL;
> } else {
> - trace_probe_log_err(offs, RETVAL_ON_PROBE);
> + trace_probe_log_err(ctx->offset, RETVAL_ON_PROBE);
> ret = -EINVAL;
> }
> } else if ((len = str_has_prefix(arg, "stack"))) {
> @@ -313,9 +339,9 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
> ret = kstrtoul(arg + len, 10, &param);
> if (ret) {
> goto inval_var;
> - } else if ((flags & TPARG_FL_KERNEL) &&
> + } else if ((ctx->flags & TPARG_FL_KERNEL) &&
> param > PARAM_MAX_STACK) {
> - trace_probe_log_err(offs, BAD_STACK_NUM);
> + trace_probe_log_err(ctx->offset, BAD_STACK_NUM);
> ret = -EINVAL;
> } else {
> code->op = FETCH_OP_STACK;
> @@ -326,13 +352,13 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
> } else if (strcmp(arg, "comm") == 0 || strcmp(arg, "COMM") == 0) {
> code->op = FETCH_OP_COMM;
> #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> - } else if (tparg_is_function_entry(flags) &&
> + } else if (tparg_is_function_entry(ctx->flags) &&
> (len = str_has_prefix(arg, "arg"))) {
> ret = kstrtoul(arg + len, 10, &param);
> if (ret) {
> goto inval_var;
> } else if (!param || param > PARAM_MAX_STACK) {
> - trace_probe_log_err(offs, BAD_ARG_NUM);
> + trace_probe_log_err(ctx->offset, BAD_ARG_NUM);
> return -EINVAL;
> }
> code->op = FETCH_OP_ARG;
> @@ -341,7 +367,7 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
> * The tracepoint probe will probe a stub function, and the
> * first parameter of the stub is a dummy and should be ignored.
> */
> - if (flags & TPARG_FL_TPOINT)
> + if (ctx->flags & TPARG_FL_TPOINT)
> code->param++;
> #endif
> } else
> @@ -350,7 +376,7 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
> return ret;
>
> inval_var:
> - trace_probe_log_err(offs, BAD_VAR);
> + trace_probe_log_err(ctx->offset, BAD_VAR);
> return -EINVAL;
> }
>

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 84a9f0446390..a30aab8cef7f 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -307,6 +307,7 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
struct traceprobe_parse_context *ctx)
{
unsigned long param;
+ int err = BAD_VAR;
int ret = 0;
int len;

@@ -322,45 +323,61 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
}
/* backward compatibility */
ctx->offset = 0;
- goto inval_var;
+ goto inval;
}

if (strcmp(arg, "retval") == 0) {
if (ctx->flags & TPARG_FL_RETURN) {
code->op = FETCH_OP_RETVAL;
- } else {
- trace_probe_log_err(ctx->offset, RETVAL_ON_PROBE);
- ret = -EINVAL;
+ return 0;
}
- } else if ((len = str_has_prefix(arg, "stack"))) {
+ err = RETVAL_ON_PROBE;
+ ret = -EINVAL;
+ goto inval;
+ }
+
+ if ((len = str_has_prefix(arg, "stack"))) {
+
if (arg[len] == '\0') {
code->op = FETCH_OP_STACKP;
- } else if (isdigit(arg[len])) {
+ return 0;
+ }
+
+ if (isdigit(arg[len])) {
ret = kstrtoul(arg + len, 10, &param);
- if (ret) {
- goto inval_var;
- } else if ((ctx->flags & TPARG_FL_KERNEL) &&
- param > PARAM_MAX_STACK) {
- trace_probe_log_err(ctx->offset, BAD_STACK_NUM);
+ if (ret)
+ goto inval;
+
+ if ((ctx->flags & TPARG_FL_KERNEL) &&
+ param > PARAM_MAX_STACK) {
+ err = BAD_STACK_NUM;
ret = -EINVAL;
- } else {
- code->op = FETCH_OP_STACK;
- code->param = (unsigned int)param;
+ goto inval;
}
- } else
- goto inval_var;
- } else if (strcmp(arg, "comm") == 0 || strcmp(arg, "COMM") == 0) {
+ code->op = FETCH_OP_STACK;
+ code->param = (unsigned int)param;
+ return 0;
+ }
+ goto inval;
+ }
+
+ if (strcmp(arg, "comm") == 0 || strcmp(arg, "COMM") == 0) {
code->op = FETCH_OP_COMM;
+ return 0;
+ }
+
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
- } else if (tparg_is_function_entry(ctx->flags) &&
- (len = str_has_prefix(arg, "arg"))) {
+ if (tparg_is_function_entry(ctx->flags) &&
+ (len = str_has_prefix(arg, "arg"))) {
ret = kstrtoul(arg + len, 10, &param);
- if (ret) {
- goto inval_var;
- } else if (!param || param > PARAM_MAX_STACK) {
- trace_probe_log_err(ctx->offset, BAD_ARG_NUM);
- return -EINVAL;
+ if (ret)
+ goto inval;
+
+ if (!param || param > PARAM_MAX_STACK) {
+ err = BAD_ARG_NUM;
+ goto inval;
}
+
code->op = FETCH_OP_ARG;
code->param = (unsigned int)param - 1;
/*
@@ -369,14 +386,11 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
*/
if (ctx->flags & TPARG_FL_TPOINT)
code->param++;
+ return 0;
#endif
- } else
- goto inval_var;
-
- return ret;

-inval_var:
- trace_probe_log_err(ctx->offset, BAD_VAR);
+inval:
+ trace_probe_log_err(ctx->offset, err);
return -EINVAL;
}