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

From: Masami Hiramatsu
Date: Wed Aug 18 2021 - 12:17:01 EST

Next message: Icenowy Zheng: "Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR"
Previous message: Robert Foss: "Re: [PATCH v1] drm/bridge: anx7625: Don't store unread return value"
In reply to: Steven Rostedt: "[PATCH v6 3/7] tracing/probe: Have traceprobe_parse_probe_arg() take a const arg"
Next in thread: Steven Rostedt: "[PATCH v6 1/7] tracing: Add DYNAMIC flag for dynamic events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
>
> 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@xxxxxxxxxx>

Thank you,

> Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

Next message: Icenowy Zheng: "Re: [PATCH] usb: typec: tcpm: always rediscover when swapping DR"
Previous message: Robert Foss: "Re: [PATCH v1] drm/bridge: anx7625: Don't store unread return value"
In reply to: Steven Rostedt: "[PATCH v6 3/7] tracing/probe: Have traceprobe_parse_probe_arg() take a const arg"
Next in thread: Steven Rostedt: "[PATCH v6 1/7] tracing: Add DYNAMIC flag for dynamic events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]