Re: [PATCH v6 3/7] tracing/probe: Have traceprobe_parse_probe_arg() take a const arg

[Masami Hiramatsu <mhiramat@kernel.org>]

On Mon, 16 Aug 2021 23:42:58 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The two places that call traceprobe_parse_probe_arg() allocate a temporary
> buffer to copy the argv[i] into, because argv[i] is constant and the
> traceprobe_parse_probe_arg() will modify it to do the parsing. These two
> places allocate this buffer and then free it right after calling this
> function, leaving the onus of this allocation to the caller.
> 
> As there's about to be a third user of this function that will have to do
> the same thing, instead of having the caller allocate the temporary
> buffer, simply move that allocation into the traceprobe_parse_probe_arg()
> itself, which will simplify the code of the callers.
> 

Thanks, this looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_kprobe.c |  9 +------
>  kernel/trace/trace_probe.c  | 47 ++++++++++++++++++++++---------------
>  kernel/trace/trace_probe.h  |  2 +-
>  kernel/trace/trace_uprobe.c |  9 +------
>  4 files changed, 31 insertions(+), 36 deletions(-)
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 82c3b86013b2..ed1e3c2087ab 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -873,15 +873,8 @@ static int __trace_kprobe_create(int argc, const char *argv[])
>  
>  	/* parse arguments */
>  	for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
> -		tmp = kstrdup(argv[i], GFP_KERNEL);
> -		if (!tmp) {
> -			ret = -ENOMEM;
> -			goto error;
> -		}
> -
>  		trace_probe_log_set_index(i + 2);
> -		ret = traceprobe_parse_probe_arg(&tk->tp, i, tmp, flags);
> -		kfree(tmp);
> +		ret = traceprobe_parse_probe_arg(&tk->tp, i, argv[i], flags);
>  		if (ret)
>  			goto error;	/* This can be -ENOMEM */
>  	}
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 15413ad7cef2..ef717b373443 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -540,26 +540,34 @@ static int __parse_bitfield_probe_arg(const char *bf,
>  }
>  
>  /* String length checking wrapper */
> -static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
> +static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
>  		struct probe_arg *parg, unsigned int flags, int offset)
>  {
>  	struct fetch_insn *code, *scode, *tmp = NULL;
>  	char *t, *t2, *t3;
> +	char *arg;
>  	int ret, len;
>  
> +	arg = kstrdup(argv, GFP_KERNEL);
> +	if (!arg)
> +		return -ENOMEM;
> +
> +	ret = -EINVAL;
>  	len = strlen(arg);
>  	if (len > MAX_ARGSTR_LEN) {
>  		trace_probe_log_err(offset, ARG_TOO_LONG);
> -		return -EINVAL;
> +		goto out;
>  	} else if (len == 0) {
>  		trace_probe_log_err(offset, NO_ARG_BODY);
> -		return -EINVAL;
> +		goto out;
>  	}
>  
> +	ret = -ENOMEM;
>  	parg->comm = kstrdup(arg, GFP_KERNEL);
>  	if (!parg->comm)
> -		return -ENOMEM;
> +		goto out;
>  
> +	ret = -EINVAL;
>  	t = strchr(arg, ':');
>  	if (t) {
>  		*t = '\0';
> @@ -571,22 +579,22 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  				offset += t2 + strlen(t2) - arg;
>  				trace_probe_log_err(offset,
>  						    ARRAY_NO_CLOSE);
> -				return -EINVAL;
> +				goto out;
>  			} else if (t3[1] != '\0') {
>  				trace_probe_log_err(offset + t3 + 1 - arg,
>  						    BAD_ARRAY_SUFFIX);
> -				return -EINVAL;
> +				goto out;
>  			}
>  			*t3 = '\0';
>  			if (kstrtouint(t2, 0, &parg->count) || !parg->count) {
>  				trace_probe_log_err(offset + t2 - arg,
>  						    BAD_ARRAY_NUM);
> -				return -EINVAL;
> +				goto out;
>  			}
>  			if (parg->count > MAX_ARRAY_LEN) {
>  				trace_probe_log_err(offset + t2 - arg,
>  						    ARRAY_TOO_BIG);
> -				return -EINVAL;
> +				goto out;
>  			}
>  		}
>  	}
> @@ -598,29 +606,30 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  	if (strcmp(arg, "$comm") == 0 || strncmp(arg, "\\\"", 2) == 0) {
>  		/* The type of $comm must be "string", and not an array. */
>  		if (parg->count || (t && strcmp(t, "string")))
> -			return -EINVAL;
> +			goto out;
>  		parg->type = find_fetch_type("string");
>  	} else
>  		parg->type = find_fetch_type(t);
>  	if (!parg->type) {
>  		trace_probe_log_err(offset + (t ? (t - arg) : 0), BAD_TYPE);
> -		return -EINVAL;
> +		goto out;
>  	}
>  	parg->offset = *size;
>  	*size += parg->type->size * (parg->count ?: 1);
>  
> +	ret = -ENOMEM;
>  	if (parg->count) {
>  		len = strlen(parg->type->fmttype) + 6;
>  		parg->fmt = kmalloc(len, GFP_KERNEL);
>  		if (!parg->fmt)
> -			return -ENOMEM;
> +			goto out;
>  		snprintf(parg->fmt, len, "%s[%d]", parg->type->fmttype,
>  			 parg->count);
>  	}
>  
>  	code = tmp = kcalloc(FETCH_INSN_MAX, sizeof(*code), GFP_KERNEL);
>  	if (!code)
> -		return -ENOMEM;
> +		goto out;
>  	code[FETCH_INSN_MAX - 1].op = FETCH_OP_END;
>  
>  	ret = parse_probe_arg(arg, parg->type, &code, &code[FETCH_INSN_MAX - 1],
> @@ -628,6 +637,7 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  	if (ret)
>  		goto fail;
>  
> +	ret = -EINVAL;
>  	/* Store operation */
>  	if (!strcmp(parg->type->name, "string") ||
>  	    !strcmp(parg->type->name, "ustring")) {
> @@ -636,7 +646,6 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  		    code->op != FETCH_OP_DATA) {
>  			trace_probe_log_err(offset + (t ? (t - arg) : 0),
>  					    BAD_STRING);
> -			ret = -EINVAL;
>  			goto fail;
>  		}
>  		if ((code->op == FETCH_OP_IMM || code->op == FETCH_OP_COMM ||
> @@ -650,7 +659,6 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  			code++;
>  			if (code->op != FETCH_OP_NOP) {
>  				trace_probe_log_err(offset, TOO_MANY_OPS);
> -				ret = -EINVAL;
>  				goto fail;
>  			}
>  		}
> @@ -672,7 +680,6 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  		code++;
>  		if (code->op != FETCH_OP_NOP) {
>  			trace_probe_log_err(offset, TOO_MANY_OPS);
> -			ret = -EINVAL;
>  			goto fail;
>  		}
>  		code->op = FETCH_OP_ST_RAW;
> @@ -687,6 +694,7 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  			goto fail;
>  		}
>  	}
> +	ret = -EINVAL;
>  	/* Loop(Array) operation */
>  	if (parg->count) {
>  		if (scode->op != FETCH_OP_ST_MEM &&
> @@ -694,13 +702,11 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  		    scode->op != FETCH_OP_ST_USTRING) {
>  			trace_probe_log_err(offset + (t ? (t - arg) : 0),
>  					    BAD_STRING);
> -			ret = -EINVAL;
>  			goto fail;
>  		}
>  		code++;
>  		if (code->op != FETCH_OP_NOP) {
>  			trace_probe_log_err(offset, TOO_MANY_OPS);
> -			ret = -EINVAL;
>  			goto fail;
>  		}
>  		code->op = FETCH_OP_LP_ARRAY;
> @@ -709,6 +715,7 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  	code++;
>  	code->op = FETCH_OP_END;
>  
> +	ret = 0;
>  	/* Shrink down the code buffer */
>  	parg->code = kcalloc(code - tmp + 1, sizeof(*code), GFP_KERNEL);
>  	if (!parg->code)
> @@ -724,6 +731,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
>  				kfree(code->data);
>  	}
>  	kfree(tmp);
> +out:
> +	kfree(arg);
>  
>  	return ret;
>  }
> @@ -745,11 +754,11 @@ static int traceprobe_conflict_field_name(const char *name,
>  	return 0;
>  }
>  
> -int traceprobe_parse_probe_arg(struct trace_probe *tp, int i, char *arg,
> +int traceprobe_parse_probe_arg(struct trace_probe *tp, int i, const char *arg,
>  				unsigned int flags)
>  {
>  	struct probe_arg *parg = &tp->args[i];
> -	char *body;
> +	const char *body;
>  
>  	/* Increment count for freeing args in error case */
>  	tp->nr_args++;
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 227d518e5ba5..42aa084902fa 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -354,7 +354,7 @@ int trace_probe_create(const char *raw_command, int (*createfn)(int, const char
>  #define TPARG_FL_MASK	GENMASK(2, 0)
>  
>  extern int traceprobe_parse_probe_arg(struct trace_probe *tp, int i,
> -				char *arg, unsigned int flags);
> +				const char *argv, unsigned int flags);
>  
>  extern int traceprobe_update_arg(struct probe_arg *arg);
>  extern void traceprobe_free_probe_arg(struct probe_arg *arg);
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 1e2a92e7607d..93ff96541971 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -684,16 +684,9 @@ static int __trace_uprobe_create(int argc, const char **argv)
>  
>  	/* parse arguments */
>  	for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
> -		tmp = kstrdup(argv[i], GFP_KERNEL);
> -		if (!tmp) {
> -			ret = -ENOMEM;
> -			goto error;
> -		}
> -
>  		trace_probe_log_set_index(i + 2);
> -		ret = traceprobe_parse_probe_arg(&tu->tp, i, tmp,
> +		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i],
>  					is_return ? TPARG_FL_RETURN : 0);
> -		kfree(tmp);
>  		if (ret)
>  			goto error;
>  	}
> -- 
> 2.30.2


-- 
Masami Hiramatsu <mhiramat@kernel.org>