by Masami Hiramatsu

[permalink] [raw]

Subject: Re: [PATCH v7 08/11] tracing/probes: Add BTF retval type support

On Thu, 27 Apr 2023 10:19:01 +0900
"Masami Hiramatsu (Google)" <[email protected]> wrote:

> From: Masami Hiramatsu (Google) <[email protected]>
>
> Check the target function has non-void retval type and set the correct
> fetch type if user doesn't specify it.
> If the function returns void, $retval is rejected as below;
>
> # echo 'f unregister_kprobes%return $retval' >> dynamic_events
> sh: write error: No such file or directory
> # cat error_log
> [ 37.488397] trace_fprobe: error: This function returns 'void' type
> Command: f unregister_kprobes%return $retval
> ^
>
> Signed-off-by: Masami Hiramatsu (Google) <[email protected]>
> ---
> Changes in v7:
> - Introduce this as a new patch.
> ---
> kernel/trace/trace_probe.c | 65 +++++++++++++++++++++++++++++++++++++++++---
> kernel/trace/trace_probe.h | 1 +
> 2 files changed, 61 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 4c3c70862a9a..16d8edfe3d15 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -371,13 +371,13 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
> return NULL;
> }
>
> -static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr)
> +static const struct btf_type *find_btf_func_proto(const char *funcname)
> {
> struct btf *btf = traceprobe_get_btf();
> const struct btf_type *t;
> s32 id;
>
> - if (!btf || !funcname || !nr)
> + if (!btf || !funcname)
> return ERR_PTR(-EINVAL);
>
> id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
> @@ -394,6 +394,20 @@ static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr
> if (!btf_type_is_func_proto(t))
> return ERR_PTR(-ENOENT);
>
> + return t;
> +}
> +
> +static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr)
> +{
> + const struct btf_type *t;
> +
> + if (!funcname || !nr)
> + return ERR_PTR(-EINVAL);
> +
> + t = find_btf_func_proto(funcname);
> + if (IS_ERR(t))
> + return (const struct btf_param *)t;
> +
> *nr = btf_type_vlen(t);
>
> if (*nr)
> @@ -452,6 +466,32 @@ static const struct fetch_type *parse_btf_arg_type(int arg_idx,
> return find_fetch_type(typestr, ctx->flags);
> }
>
> +static const struct fetch_type *parse_btf_retval_type(
> + struct traceprobe_parse_context *ctx)
> +{
> + struct btf *btf = traceprobe_get_btf();
> + const char *typestr = NULL;
> + const struct btf_type *t;
> +
> + if (btf && ctx->funcname) {
> + t = find_btf_func_proto(ctx->funcname);
> + if (!IS_ERR(t))
> + typestr = type_from_btf_id(btf, t->type);
> + }
> +
> + return find_fetch_type(typestr, ctx->flags);
> +}
> +
> +static bool is_btf_retval_void(const char *funcname)
> +{
> + const struct btf_type *t;
> +
> + t = find_btf_func_proto(funcname);
> + if (IS_ERR(t))
> + return false;
> +
> + return t->type == 0;
> +}
> #else
> static struct btf *traceprobe_get_btf(void)
> {
> @@ -469,8 +509,15 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
> trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
> return -EOPNOTSUPP;
> }
> +
> #define parse_btf_arg_type(idx, ctx) \
> find_fetch_type(NULL, ctx->flags)
> +
> +#define parse_btf_retval_type(ctx) \
> + find_fetch_type(NULL, ctx->flags)
> +
> +#define is_btf_retval_void(funcname) (false)
> +
> #endif
>
> #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
> @@ -500,7 +547,12 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
>
> if (strcmp(arg, "retval") == 0) {
> if (ctx->flags & TPARG_FL_RETURN) {
> - code->op = FETCH_OP_RETVAL;
> + if ((ctx->flags & TPARG_FL_KERNEL) &&
> + is_btf_retval_void(ctx->funcname)) {
> + trace_probe_log_err(ctx->offset, NO_RETVAL);
> + ret = -ENOENT;
> + } else
> + code->op = FETCH_OP_RETVAL;
> } else {
> trace_probe_log_err(ctx->offset, RETVAL_ON_PROBE);
> ret = -EINVAL;
> @@ -887,9 +939,12 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
> goto fail;
>
> /* Update storing type if BTF is available */
> - if (IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS) &&
> - !t && code->op == FETCH_OP_ARG)
> + if (IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS) && !t) {
> + if (code->op == FETCH_OP_ARG)
> parg->type = parse_btf_arg_type(code->param, ctx);
> + else if (code->op == FETCH_OP_RETVAL)
> + parg->type = parse_btf_retval_type(ctx);
> + }

Oops, here are wrong indents. I have to fix this.

Thanks,

>
> ret = -EINVAL;
> /* Store operation */
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 8c5b029c5d62..3eb7c37c0984 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -444,6 +444,7 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
> C(BAD_EVENT_NAME, "Event name must follow the same rules as C identifiers"), \
> C(EVENT_EXIST, "Given group/event name is already used by another event"), \
> C(RETVAL_ON_PROBE, "$retval is not available on probe"), \
> + C(NO_RETVAL, "This function returns 'void' type"), \
> C(BAD_STACK_NUM, "Invalid stack number"), \
> C(BAD_ARG_NUM, "Invalid argument number"), \
> C(BAD_VAR, "Invalid $-valiable specified"), \
>

--
Masami Hiramatsu (Google) <[email protected]>