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

[Steven Rostedt]

On Tue,  2 May 2023 11:18:03 +0900
"Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> 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;
 }