[v2,2/9] bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF

Commit Message

Masami Hiramatsu (Google) July 17, 2023, 3:23 p.m. UTC

From: Masami Hiramatsu (Google) <[e-mail]>

Move generic function-proto find API and getting function parameter API
to BTF library code from trace_probe.c. This will avoid redundant efforts
on different feature.

Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
---
 include/linux/btf.h        |    4 ++++
 kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
 kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
 3 files changed, 64 insertions(+), 35 deletions(-)

Comments

Steven Rostedt July 17, 2023, 6:39 p.m. UTC | #1

On Tue, 18 Jul 2023 00:23:37 +0900
"Masami Hiramatsu (Google)" <[e-mail]> wrote:

> From: Masami Hiramatsu (Google) <[e-mail]>
> 
> Move generic function-proto find API and getting function parameter API
> to BTF library code from trace_probe.c. This will avoid redundant efforts
> on different feature.

 "different features."

> 
> Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> ---
>  include/linux/btf.h        |    4 ++++
>  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
>  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
>  3 files changed, 64 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index cac9f304e27a..98fbbcdd72ec 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -221,6 +221,10 @@ const struct btf_type *
>  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>  		 u32 *type_size);
>  const char *btf_type_str(const struct btf_type *t);
> +const struct btf_type *btf_find_func_proto(struct btf *btf,
> +					   const char *func_name);
> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> +					   s32 *nr);
>  
>  #define for_each_member(i, struct_type, member)			\
>  	for (i = 0, member = btf_type_member(struct_type);	\
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 817204d53372..e015b52956cb 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>  	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
>  }
>  
> +/*
> + * Find a functio proto type by name, and return it.

  "function"

> + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> + */
> +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> +{
> +	const struct btf_type *t;
> +	s32 id;
> +
> +	if (!btf || !func_name)
> +		return ERR_PTR(-EINVAL);
> +
> +	id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
> +	if (id <= 0)
> +		return NULL;
> +
> +	/* Get BTF_KIND_FUNC type */
> +	t = btf_type_by_id(btf, id);
> +	if (!t || !btf_type_is_func(t))
> +		return NULL;
> +
> +	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> +	t = btf_type_by_id(btf, t->type);
> +	if (!t || !btf_type_is_func_proto(t))
> +		return NULL;
> +
> +	return t;
> +}
> +
> +/*
> + * Get function parameter with the number of parameters.
> + * This can return NULL if the function has no parameters.

  " It can return EINVAL if this function's parameters are NULL."

-- Steve


> + */
> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> +{
> +	if (!func_proto || !nr)
> +		return ERR_PTR(-EINVAL);
> +
> +	*nr = btf_type_vlen(func_proto);
> +	if (*nr > 0)
> +		return (const struct btf_param *)(func_proto + 1);
> +	else
> +		return NULL;
> +}
> +
>  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
>  {
>  	while (type_id < btf->start_id)
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index c68a72707852..cd89fc1ebb42 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
>  	return NULL;
>  }
>  
> -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)
> -		return ERR_PTR(-EINVAL);
> -
> -	id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
> -	if (id <= 0)
> -		return ERR_PTR(-ENOENT);
> -
> -	/* Get BTF_KIND_FUNC type */
> -	t = btf_type_by_id(btf, id);
> -	if (!t || !btf_type_is_func(t))
> -		return ERR_PTR(-ENOENT);
> -
> -	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> -	t = btf_type_by_id(btf, t->type);
> -	if (!t || !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,
>  						   bool tracepoint)
>  {
> +	struct btf *btf = traceprobe_get_btf();
>  	const struct btf_param *param;
>  	const struct btf_type *t;
>  
> -	if (!funcname || !nr)
> +	if (!funcname || !nr || !btf)
>  		return ERR_PTR(-EINVAL);
>  
> -	t = find_btf_func_proto(funcname);
> -	if (IS_ERR(t))
> +	t = btf_find_func_proto(btf, funcname);
> +	if (IS_ERR_OR_NULL(t))
>  		return (const struct btf_param *)t;
>  
> -	*nr = btf_type_vlen(t);
> -	param = (const struct btf_param *)(t + 1);
> +	param = btf_get_func_param(t, nr);
> +	if (IS_ERR_OR_NULL(param))
> +		return param;
>  
>  	/* Hide the first 'data' argument of tracepoint */
>  	if (tracepoint) {
> @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
>  	const struct btf_type *t;
>  
>  	if (btf && ctx->funcname) {
> -		t = find_btf_func_proto(ctx->funcname);
> -		if (!IS_ERR(t))
> +		t = btf_find_func_proto(btf, ctx->funcname);
> +		if (!IS_ERR_OR_NULL(t))
>  			typestr = type_from_btf_id(btf, t->type);
>  	}
>  
> @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
>  
>  static bool is_btf_retval_void(const char *funcname)
>  {
> +	struct btf *btf = traceprobe_get_btf();
>  	const struct btf_type *t;
>  
> -	t = find_btf_func_proto(funcname);
> -	if (IS_ERR(t))
> +	if (!btf)
> +		return false;
> +
> +	t = btf_find_func_proto(btf, funcname);
> +	if (IS_ERR_OR_NULL(t))
>  		return false;
>  
>  	return t->type == 0;

Masami Hiramatsu (Google) July 17, 2023, 11:46 p.m. UTC | #2

On Mon, 17 Jul 2023 14:39:14 -0400
Steven Rostedt <[e-mail]> wrote:

> On Tue, 18 Jul 2023 00:23:37 +0900
> "Masami Hiramatsu (Google)" <[e-mail]> wrote:
> 
> > From: Masami Hiramatsu (Google) <[e-mail]>
> > 
> > Move generic function-proto find API and getting function parameter API
> > to BTF library code from trace_probe.c. This will avoid redundant efforts
> > on different feature.
> 
>  "different features."

Thanks!

> 
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> > ---
> >  include/linux/btf.h        |    4 ++++
> >  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
> >  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
> >  3 files changed, 64 insertions(+), 35 deletions(-)
> > 
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index cac9f304e27a..98fbbcdd72ec 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -221,6 +221,10 @@ const struct btf_type *
> >  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
> >  		 u32 *type_size);
> >  const char *btf_type_str(const struct btf_type *t);
> > +const struct btf_type *btf_find_func_proto(struct btf *btf,
> > +					   const char *func_name);
> > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> > +					   s32 *nr);
> >  
> >  #define for_each_member(i, struct_type, member)			\
> >  	for (i = 0, member = btf_type_member(struct_type);	\
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 817204d53372..e015b52956cb 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
> >  	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
> >  }
> >  
> > +/*
> > + * Find a functio proto type by name, and return it.
> 
>   "function"

Oops.

> 
> > + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> > + */
> > +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> > +{
> > +	const struct btf_type *t;
> > +	s32 id;
> > +
> > +	if (!btf || !func_name)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
> > +	if (id <= 0)
> > +		return NULL;
> > +
> > +	/* Get BTF_KIND_FUNC type */
> > +	t = btf_type_by_id(btf, id);
> > +	if (!t || !btf_type_is_func(t))
> > +		return NULL;
> > +
> > +	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> > +	t = btf_type_by_id(btf, t->type);
> > +	if (!t || !btf_type_is_func_proto(t))
> > +		return NULL;
> > +
> > +	return t;
> > +}
> > +
> > +/*
> > + * Get function parameter with the number of parameters.
> > + * This can return NULL if the function has no parameters.
> 
>   " It can return EINVAL if this function's parameters are NULL."

No, as you can see the code, if btf_type_vlen(func_proto) returns 0 (means
the function proto type has no parameters), btf_get_func_param() returns
NULL. This is important point because user needs to use IS_ERR_OR_NULL(ret)
instead of IS_ERR(ret).

Thank you,

> 
> -- Steve
> 
> 
> > + */
> > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> > +{
> > +	if (!func_proto || !nr)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	*nr = btf_type_vlen(func_proto);
> > +	if (*nr > 0)
> > +		return (const struct btf_param *)(func_proto + 1);
> > +	else
> > +		return NULL;
> > +}
> > +
> >  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
> >  {
> >  	while (type_id < btf->start_id)
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index c68a72707852..cd89fc1ebb42 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
> >  	return NULL;
> >  }
> >  
> > -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)
> > -		return ERR_PTR(-EINVAL);
> > -
> > -	id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
> > -	if (id <= 0)
> > -		return ERR_PTR(-ENOENT);
> > -
> > -	/* Get BTF_KIND_FUNC type */
> > -	t = btf_type_by_id(btf, id);
> > -	if (!t || !btf_type_is_func(t))
> > -		return ERR_PTR(-ENOENT);
> > -
> > -	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> > -	t = btf_type_by_id(btf, t->type);
> > -	if (!t || !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,
> >  						   bool tracepoint)
> >  {
> > +	struct btf *btf = traceprobe_get_btf();
> >  	const struct btf_param *param;
> >  	const struct btf_type *t;
> >  
> > -	if (!funcname || !nr)
> > +	if (!funcname || !nr || !btf)
> >  		return ERR_PTR(-EINVAL);
> >  
> > -	t = find_btf_func_proto(funcname);
> > -	if (IS_ERR(t))
> > +	t = btf_find_func_proto(btf, funcname);
> > +	if (IS_ERR_OR_NULL(t))
> >  		return (const struct btf_param *)t;
> >  
> > -	*nr = btf_type_vlen(t);
> > -	param = (const struct btf_param *)(t + 1);
> > +	param = btf_get_func_param(t, nr);
> > +	if (IS_ERR_OR_NULL(param))
> > +		return param;
> >  
> >  	/* Hide the first 'data' argument of tracepoint */
> >  	if (tracepoint) {
> > @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
> >  	const struct btf_type *t;
> >  
> >  	if (btf && ctx->funcname) {
> > -		t = find_btf_func_proto(ctx->funcname);
> > -		if (!IS_ERR(t))
> > +		t = btf_find_func_proto(btf, ctx->funcname);
> > +		if (!IS_ERR_OR_NULL(t))
> >  			typestr = type_from_btf_id(btf, t->type);
> >  	}
> >  
> > @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
> >  
> >  static bool is_btf_retval_void(const char *funcname)
> >  {
> > +	struct btf *btf = traceprobe_get_btf();
> >  	const struct btf_type *t;
> >  
> > -	t = find_btf_func_proto(funcname);
> > -	if (IS_ERR(t))
> > +	if (!btf)
> > +		return false;
> > +
> > +	t = btf_find_func_proto(btf, funcname);
> > +	if (IS_ERR_OR_NULL(t))
> >  		return false;
> >  
> >  	return t->type == 0;
>

Steven Rostedt July 17, 2023, 11:51 p.m. UTC | #3

On Tue, 18 Jul 2023 08:46:34 +0900
Masami Hiramatsu (Google) <[e-mail]> wrote:

> > > + * Get function parameter with the number of parameters.
> > > + * This can return NULL if the function has no parameters.  
> > 
> >   " It can return EINVAL if this function's parameters are NULL."  
> 
> No, as you can see the code, if btf_type_vlen(func_proto) returns 0 (means
> the function proto type has no parameters), btf_get_func_param() returns
> NULL. This is important point because user needs to use IS_ERR_OR_NULL(ret)
> instead of IS_ERR(ret).

I didn't mean to replace what you had, I meant you left that part out. In
other words, you have to check for IS_ERR_OR_NULL(ret), not just "!ret".

-- Steve

> >   
> > > + */
> > > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> > > +{
> > > +	if (!func_proto || !nr)
> > > +		return ERR_PTR(-EINVAL);
> > > +

Alexei Starovoitov July 17, 2023, 11:51 p.m. UTC | #4

On Mon, Jul 17, 2023 at 4:46 PM Masami Hiramatsu <[e-mail]> wrote:
>
> >
> > > + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> > > + */
> > > +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> > > +{
> > > +   const struct btf_type *t;
> > > +   s32 id;
> > > +
> > > +   if (!btf || !func_name)
> > > +           return ERR_PTR(-EINVAL);

Please remove these checks.
We don't do defensive programming in the BPF subsystem.
Don't pass NULL pointers to such functions.

Masami Hiramatsu (Google) July 18, 2023, 1:02 a.m. UTC | #5

On Mon, 17 Jul 2023 19:51:29 -0400
Steven Rostedt <[e-mail]> wrote:

> On Tue, 18 Jul 2023 08:46:34 +0900
> Masami Hiramatsu (Google) <[e-mail]> wrote:
> 
> > > > + * Get function parameter with the number of parameters.
> > > > + * This can return NULL if the function has no parameters.  
> > > 
> > >   " It can return EINVAL if this function's parameters are NULL."  
> > 
> > No, as you can see the code, if btf_type_vlen(func_proto) returns 0 (means
> > the function proto type has no parameters), btf_get_func_param() returns
> > NULL. This is important point because user needs to use IS_ERR_OR_NULL(ret)
> > instead of IS_ERR(ret).
> 
> I didn't mean to replace what you had, I meant you left that part out. In
> other words, you have to check for IS_ERR_OR_NULL(ret), not just "!ret".

Ah, got it! Let me update it.

Thank you!

> 
> -- Steve
> 
> > >   
> > > > + */
> > > > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> > > > +{
> > > > +	if (!func_proto || !nr)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +

Masami Hiramatsu (Google) July 18, 2023, 1:03 a.m. UTC | #6

On Mon, 17 Jul 2023 16:51:29 -0700
Alexei Starovoitov <[e-mail]> wrote:

> On Mon, Jul 17, 2023 at 4:46 PM Masami Hiramatsu <[e-mail]> wrote:
> >
> > >
> > > > + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> > > > + */
> > > > +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> > > > +{
> > > > +   const struct btf_type *t;
> > > > +   s32 id;
> > > > +
> > > > +   if (!btf || !func_name)
> > > > +           return ERR_PTR(-EINVAL);
> 
> Please remove these checks.
> We don't do defensive programming in the BPF subsystem.
> Don't pass NULL pointers to such functions.

OK, we will trust API user to pass a non-NULL parameters.

Thank you!

Donglin Peng July 18, 2023, 2:40 a.m. UTC | #7

On Mon, Jul 17, 2023 at 11:24 PM Masami Hiramatsu (Google)
<[e-mail]> wrote:
>
> From: Masami Hiramatsu (Google) <[e-mail]>
>
> Move generic function-proto find API and getting function parameter API
> to BTF library code from trace_probe.c. This will avoid redundant efforts
> on different feature.
>
> Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> ---
>  include/linux/btf.h        |    4 ++++
>  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
>  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
>  3 files changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index cac9f304e27a..98fbbcdd72ec 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -221,6 +221,10 @@ const struct btf_type *
>  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>                  u32 *type_size);
>  const char *btf_type_str(const struct btf_type *t);
> +const struct btf_type *btf_find_func_proto(struct btf *btf,
> +                                          const char *func_name);
> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> +                                          s32 *nr);
>
>  #define for_each_member(i, struct_type, member)                        \
>         for (i = 0, member = btf_type_member(struct_type);      \
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 817204d53372..e015b52956cb 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>         return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
>  }
>
> +/*
> + * Find a functio proto type by name, and return it.
> + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> + */
> +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> +{
> +       const struct btf_type *t;
> +       s32 id;
> +
> +       if (!btf || !func_name)
> +               return ERR_PTR(-EINVAL);
> +
> +       id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
> +       if (id <= 0)
> +               return NULL;
> +
> +       /* Get BTF_KIND_FUNC type */
> +       t = btf_type_by_id(btf, id);
> +       if (!t || !btf_type_is_func(t))
> +               return NULL;
> +
> +       /* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> +       t = btf_type_by_id(btf, t->type);
> +       if (!t || !btf_type_is_func_proto(t))
> +               return NULL;
> +
> +       return t;
> +}
> +
> +/*
> + * Get function parameter with the number of parameters.
> + * This can return NULL if the function has no parameters.
> + */
> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> +{
> +       if (!func_proto || !nr)
> +               return ERR_PTR(-EINVAL);
> +
> +       *nr = btf_type_vlen(func_proto);
> +       if (*nr > 0)
> +               return (const struct btf_param *)(func_proto + 1);
> +       else
> +               return NULL;
> +}
> +
>  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
>  {
>         while (type_id < btf->start_id)
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index c68a72707852..cd89fc1ebb42 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
>         return NULL;
>  }
>
> -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)
> -               return ERR_PTR(-EINVAL);
> -
> -       id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
> -       if (id <= 0)
> -               return ERR_PTR(-ENOENT);
> -
> -       /* Get BTF_KIND_FUNC type */
> -       t = btf_type_by_id(btf, id);
> -       if (!t || !btf_type_is_func(t))
> -               return ERR_PTR(-ENOENT);
> -
> -       /* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> -       t = btf_type_by_id(btf, t->type);
> -       if (!t || !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,
>                                                    bool tracepoint)
>  {
> +       struct btf *btf = traceprobe_get_btf();

I found that traceprobe_get_btf() only returns the vmlinux's btf. But
if the function is
defined in a kernel module, we should get the module's btf.

-- Donglin

>         const struct btf_param *param;
>         const struct btf_type *t;
>
> -       if (!funcname || !nr)
> +       if (!funcname || !nr || !btf)
>                 return ERR_PTR(-EINVAL);
>
> -       t = find_btf_func_proto(funcname);
> -       if (IS_ERR(t))
> +       t = btf_find_func_proto(btf, funcname);
> +       if (IS_ERR_OR_NULL(t))
>                 return (const struct btf_param *)t;
>
> -       *nr = btf_type_vlen(t);
> -       param = (const struct btf_param *)(t + 1);
> +       param = btf_get_func_param(t, nr);
> +       if (IS_ERR_OR_NULL(param))
> +               return param;
>
>         /* Hide the first 'data' argument of tracepoint */
>         if (tracepoint) {
> @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
>         const struct btf_type *t;
>
>         if (btf && ctx->funcname) {
> -               t = find_btf_func_proto(ctx->funcname);
> -               if (!IS_ERR(t))
> +               t = btf_find_func_proto(btf, ctx->funcname);
> +               if (!IS_ERR_OR_NULL(t))
>                         typestr = type_from_btf_id(btf, t->type);
>         }
>
> @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
>
>  static bool is_btf_retval_void(const char *funcname)
>  {
> +       struct btf *btf = traceprobe_get_btf();
>         const struct btf_type *t;
>
> -       t = find_btf_func_proto(funcname);
> -       if (IS_ERR(t))
> +       if (!btf)
> +               return false;
> +
> +       t = btf_find_func_proto(btf, funcname);
> +       if (IS_ERR_OR_NULL(t))
>                 return false;
>
>         return t->type == 0;
>
>

Masami Hiramatsu (Google) July 18, 2023, 10:44 a.m. UTC | #8

On Tue, 18 Jul 2023 10:40:09 +0800
Donglin Peng <[e-mail]> wrote:

> On Mon, Jul 17, 2023 at 11:24 PM Masami Hiramatsu (Google)
> <[e-mail]> wrote:
> >
> > From: Masami Hiramatsu (Google) <[e-mail]>
> >
> > Move generic function-proto find API and getting function parameter API
> > to BTF library code from trace_probe.c. This will avoid redundant efforts
> > on different feature.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> > ---
> >  include/linux/btf.h        |    4 ++++
> >  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
> >  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
> >  3 files changed, 64 insertions(+), 35 deletions(-)
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index cac9f304e27a..98fbbcdd72ec 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -221,6 +221,10 @@ const struct btf_type *
> >  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
> >                  u32 *type_size);
> >  const char *btf_type_str(const struct btf_type *t);
> > +const struct btf_type *btf_find_func_proto(struct btf *btf,
> > +                                          const char *func_name);
> > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> > +                                          s32 *nr);
> >
> >  #define for_each_member(i, struct_type, member)                        \
> >         for (i = 0, member = btf_type_member(struct_type);      \
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 817204d53372..e015b52956cb 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
> >         return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
> >  }
> >
> > +/*
> > + * Find a functio proto type by name, and return it.
> > + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> > + */
> > +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> > +{
> > +       const struct btf_type *t;
> > +       s32 id;
> > +
> > +       if (!btf || !func_name)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
> > +       if (id <= 0)
> > +               return NULL;
> > +
> > +       /* Get BTF_KIND_FUNC type */
> > +       t = btf_type_by_id(btf, id);
> > +       if (!t || !btf_type_is_func(t))
> > +               return NULL;
> > +
> > +       /* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> > +       t = btf_type_by_id(btf, t->type);
> > +       if (!t || !btf_type_is_func_proto(t))
> > +               return NULL;
> > +
> > +       return t;
> > +}
> > +
> > +/*
> > + * Get function parameter with the number of parameters.
> > + * This can return NULL if the function has no parameters.
> > + */
> > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> > +{
> > +       if (!func_proto || !nr)
> > +               return ERR_PTR(-EINVAL);
> > +
> > +       *nr = btf_type_vlen(func_proto);
> > +       if (*nr > 0)
> > +               return (const struct btf_param *)(func_proto + 1);
> > +       else
> > +               return NULL;
> > +}
> > +
> >  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
> >  {
> >         while (type_id < btf->start_id)
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index c68a72707852..cd89fc1ebb42 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
> >         return NULL;
> >  }
> >
> > -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)
> > -               return ERR_PTR(-EINVAL);
> > -
> > -       id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
> > -       if (id <= 0)
> > -               return ERR_PTR(-ENOENT);
> > -
> > -       /* Get BTF_KIND_FUNC type */
> > -       t = btf_type_by_id(btf, id);
> > -       if (!t || !btf_type_is_func(t))
> > -               return ERR_PTR(-ENOENT);
> > -
> > -       /* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> > -       t = btf_type_by_id(btf, t->type);
> > -       if (!t || !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,
> >                                                    bool tracepoint)
> >  {
> > +       struct btf *btf = traceprobe_get_btf();
> 
> I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> if the function is
> defined in a kernel module, we should get the module's btf.
> 

Good catch! That should be a separated fix (or improvement?)
I think it's better to use btf_get() and btf_put(), and pass btf via
traceprobe_parse_context.

Thank you! 

> -- Donglin
> 
> >         const struct btf_param *param;
> >         const struct btf_type *t;
> >
> > -       if (!funcname || !nr)
> > +       if (!funcname || !nr || !btf)
> >                 return ERR_PTR(-EINVAL);
> >
> > -       t = find_btf_func_proto(funcname);
> > -       if (IS_ERR(t))
> > +       t = btf_find_func_proto(btf, funcname);
> > +       if (IS_ERR_OR_NULL(t))
> >                 return (const struct btf_param *)t;
> >
> > -       *nr = btf_type_vlen(t);
> > -       param = (const struct btf_param *)(t + 1);
> > +       param = btf_get_func_param(t, nr);
> > +       if (IS_ERR_OR_NULL(param))
> > +               return param;
> >
> >         /* Hide the first 'data' argument of tracepoint */
> >         if (tracepoint) {
> > @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
> >         const struct btf_type *t;
> >
> >         if (btf && ctx->funcname) {
> > -               t = find_btf_func_proto(ctx->funcname);
> > -               if (!IS_ERR(t))
> > +               t = btf_find_func_proto(btf, ctx->funcname);
> > +               if (!IS_ERR_OR_NULL(t))
> >                         typestr = type_from_btf_id(btf, t->type);
> >         }
> >
> > @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
> >
> >  static bool is_btf_retval_void(const char *funcname)
> >  {
> > +       struct btf *btf = traceprobe_get_btf();
> >         const struct btf_type *t;
> >
> > -       t = find_btf_func_proto(funcname);
> > -       if (IS_ERR(t))
> > +       if (!btf)
> > +               return false;
> > +
> > +       t = btf_find_func_proto(btf, funcname);
> > +       if (IS_ERR_OR_NULL(t))
> >                 return false;
> >
> >         return t->type == 0;
> >
> >

Masami Hiramatsu (Google) July 18, 2023, 1:56 p.m. UTC | #9

On Tue, 18 Jul 2023 19:44:31 +0900
Masami Hiramatsu (Google) <[e-mail]> wrote:

> > >  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
> > >                                                    bool tracepoint)
> > >  {
> > > +       struct btf *btf = traceprobe_get_btf();
> > 
> > I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> > if the function is
> > defined in a kernel module, we should get the module's btf.
> > 
> 
> Good catch! That should be a separated fix (or improvement?)
> I think it's better to use btf_get() and btf_put(), and pass btf via
> traceprobe_parse_context.

Hmm, it seems that there is no exposed API to get the module's btf.
Should I use btf_idr and btf_idr_lock directly to find the corresponding
btf? If there isn't yet, I will add it too.

Thank you,

Alexei Starovoitov July 18, 2023, 5:11 p.m. UTC | #10

On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[e-mail]> wrote:
>
> On Tue, 18 Jul 2023 19:44:31 +0900
> Masami Hiramatsu (Google) <[e-mail]> wrote:
>
> > > >  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
> > > >                                                    bool tracepoint)
> > > >  {
> > > > +       struct btf *btf = traceprobe_get_btf();
> > >
> > > I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> > > if the function is
> > > defined in a kernel module, we should get the module's btf.
> > >
> >
> > Good catch! That should be a separated fix (or improvement?)
> > I think it's better to use btf_get() and btf_put(), and pass btf via
> > traceprobe_parse_context.
>
> Hmm, it seems that there is no exposed API to get the module's btf.
> Should I use btf_idr and btf_idr_lock directly to find the corresponding
> btf? If there isn't yet, I will add it too.

There is bpf_find_btf_id.
Probably drop 'static' from it and use it.

Masami Hiramatsu (Google) July 18, 2023, 11:03 p.m. UTC | #11

On Tue, 18 Jul 2023 10:11:01 -0700
Alexei Starovoitov <[e-mail]> wrote:

> On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[e-mail]> wrote:
> >
> > On Tue, 18 Jul 2023 19:44:31 +0900
> > Masami Hiramatsu (Google) <[e-mail]> wrote:
> >
> > > > >  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
> > > > >                                                    bool tracepoint)
> > > > >  {
> > > > > +       struct btf *btf = traceprobe_get_btf();
> > > >
> > > > I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> > > > if the function is
> > > > defined in a kernel module, we should get the module's btf.
> > > >
> > >
> > > Good catch! That should be a separated fix (or improvement?)
> > > I think it's better to use btf_get() and btf_put(), and pass btf via
> > > traceprobe_parse_context.
> >
> > Hmm, it seems that there is no exposed API to get the module's btf.
> > Should I use btf_idr and btf_idr_lock directly to find the corresponding
> > btf? If there isn't yet, I will add it too.
> 
> There is bpf_find_btf_id.
> Probably drop 'static' from it and use it.

Thanks! BTW, that API seems to search BTF type info by name. If user want to
specify a module name, do we need a new API? (Or expand the function to parse
a module name in given name?)

Thank you,

Alexei Starovoitov July 18, 2023, 11:12 p.m. UTC | #12

On Tue, Jul 18, 2023 at 4:03 PM Masami Hiramatsu <[e-mail]> wrote:
>
> On Tue, 18 Jul 2023 10:11:01 -0700
> Alexei Starovoitov <[e-mail]> wrote:
>
> > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[e-mail]> wrote:
> > >
> > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > Masami Hiramatsu (Google) <[e-mail]> wrote:
> > >
> > > > > >  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
> > > > > >                                                    bool tracepoint)
> > > > > >  {
> > > > > > +       struct btf *btf = traceprobe_get_btf();
> > > > >
> > > > > I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> > > > > if the function is
> > > > > defined in a kernel module, we should get the module's btf.
> > > > >
> > > >
> > > > Good catch! That should be a separated fix (or improvement?)
> > > > I think it's better to use btf_get() and btf_put(), and pass btf via
> > > > traceprobe_parse_context.
> > >
> > > Hmm, it seems that there is no exposed API to get the module's btf.
> > > Should I use btf_idr and btf_idr_lock directly to find the corresponding
> > > btf? If there isn't yet, I will add it too.
> >
> > There is bpf_find_btf_id.
> > Probably drop 'static' from it and use it.
>
> Thanks! BTW, that API seems to search BTF type info by name. If user want to
> specify a module name, do we need a new API? (Or expand the function to parse
> a module name in given name?)

We can allow users specify module name, but how would it help?
Do you want to allow static func names ?
But module name won't help. There can be many statics with the same name
in the module. Currently pahole filters out all ambiguous things in BTF.
Alan is working on better representation of statics in BTF.
The work is still in progress.

For now I don't see a need for an api to specify module, since it's not
a modifier that can be relied upon to disambiguate.
Hence bpf_find_btf_id that transparently searches across all should be enough.
At least it was enough for all of bpf use cases.

Donglin Peng July 19, 2023, 2:09 a.m. UTC | #13

On Wed, Jul 19, 2023 at 7:03 AM Masami Hiramatsu <[e-mail]> wrote:
>
> On Tue, 18 Jul 2023 10:11:01 -0700
> Alexei Starovoitov <[e-mail]> wrote:
>
> > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[e-mail]> wrote:
> > >
> > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > Masami Hiramatsu (Google) <[e-mail]> wrote:
> > >
> > > > > >  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
> > > > > >                                                    bool tracepoint)
> > > > > >  {
> > > > > > +       struct btf *btf = traceprobe_get_btf();
> > > > >
> > > > > I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> > > > > if the function is
> > > > > defined in a kernel module, we should get the module's btf.
> > > > >
> > > >
> > > > Good catch! That should be a separated fix (or improvement?)
> > > > I think it's better to use btf_get() and btf_put(), and pass btf via
> > > > traceprobe_parse_context.
> > >
> > > Hmm, it seems that there is no exposed API to get the module's btf.
> > > Should I use btf_idr and btf_idr_lock directly to find the corresponding
> > > btf? If there isn't yet, I will add it too.
> >
> > There is bpf_find_btf_id.
> > Probably drop 'static' from it and use it.
>
> Thanks! BTW, that API seems to search BTF type info by name. If user want to
> specify a module name, do we need a new API? (Or expand the function to parse
> a module name in given name?)

btf_get_module_btf can be used to get a module's btf, but we have to use
find_module to get the module by its name firstly.

>
> Thank you,
> --
> Masami Hiramatsu (Google) <[e-mail]>

Alan Maguire July 19, 2023, 12:36 p.m. UTC | #14

On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <[e-mail]>
> 
> Move generic function-proto find API and getting function parameter API
> to BTF library code from trace_probe.c. This will avoid redundant efforts
> on different feature.
> 
> Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> ---
>  include/linux/btf.h        |    4 ++++
>  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
>  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
>  3 files changed, 64 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index cac9f304e27a..98fbbcdd72ec 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -221,6 +221,10 @@ const struct btf_type *
>  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>  		 u32 *type_size);
>  const char *btf_type_str(const struct btf_type *t);
> +const struct btf_type *btf_find_func_proto(struct btf *btf,
> +					   const char *func_name);
> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> +					   s32 *nr);
>  
>  #define for_each_member(i, struct_type, member)			\
>  	for (i = 0, member = btf_type_member(struct_type);	\
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 817204d53372..e015b52956cb 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>  	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
>  }
>  
> +/*
> + * Find a functio proto type by name, and return it.
> + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> + */
> +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> +{
> +	const struct btf_type *t;
> +	s32 id;
> +
> +	if (!btf || !func_name)
> +		return ERR_PTR(-EINVAL);
> +
> +	id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);

as mentioned in my other mail, there are cases where the function name
may have a .isra.0 suffix, but the BTF representation will not. I looked
at this and it seems like symbol names are validated via
traceprobe_parse_event_name() - will this validation allow a "."-suffix
name? I tried the following (with pahole v1.25 that emits BTF for
schedule_work.isra.0):

[45454] FUNC 'schedule_work' type_id=45453 linkage=static

$ echo 'f schedule_work.isra.0 $arg*' >> dynamic_events
bash: echo: write error: No such file or directory

So presuming that such "."-suffixed names are allowed, would it make
sense to fall back to search BTF for the prefix ("schedule_work")
instead of the full name ("schedule_work.isra.0"), as the former is what
makes it into the BTF representation? Thanks!

Alan

> +	if (id <= 0)
> +		return NULL;
> +
> +	/* Get BTF_KIND_FUNC type */
> +	t = btf_type_by_id(btf, id);
> +	if (!t || !btf_type_is_func(t))
> +		return NULL;
> +
> +	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> +	t = btf_type_by_id(btf, t->type);
> +	if (!t || !btf_type_is_func_proto(t))
> +		return NULL;
> +
> +	return t;
> +}
> +
> +/*
> + * Get function parameter with the number of parameters.
> + * This can return NULL if the function has no parameters.
> + */
> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> +{
> +	if (!func_proto || !nr)
> +		return ERR_PTR(-EINVAL);
> +
> +	*nr = btf_type_vlen(func_proto);
> +	if (*nr > 0)
> +		return (const struct btf_param *)(func_proto + 1);
> +	else
> +		return NULL;
> +}
> +
>  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
>  {
>  	while (type_id < btf->start_id)
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index c68a72707852..cd89fc1ebb42 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
>  	return NULL;
>  }
>  
> -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)
> -		return ERR_PTR(-EINVAL);
> -
> -	id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
> -	if (id <= 0)
> -		return ERR_PTR(-ENOENT);
> -
> -	/* Get BTF_KIND_FUNC type */
> -	t = btf_type_by_id(btf, id);
> -	if (!t || !btf_type_is_func(t))
> -		return ERR_PTR(-ENOENT);
> -
> -	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> -	t = btf_type_by_id(btf, t->type);
> -	if (!t || !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,
>  						   bool tracepoint)
>  {
> +	struct btf *btf = traceprobe_get_btf();
>  	const struct btf_param *param;
>  	const struct btf_type *t;
>  
> -	if (!funcname || !nr)
> +	if (!funcname || !nr || !btf)
>  		return ERR_PTR(-EINVAL);
>  
> -	t = find_btf_func_proto(funcname);
> -	if (IS_ERR(t))
> +	t = btf_find_func_proto(btf, funcname);
> +	if (IS_ERR_OR_NULL(t))
>  		return (const struct btf_param *)t;
>  
> -	*nr = btf_type_vlen(t);
> -	param = (const struct btf_param *)(t + 1);
> +	param = btf_get_func_param(t, nr);
> +	if (IS_ERR_OR_NULL(param))
> +		return param;
>  
>  	/* Hide the first 'data' argument of tracepoint */
>  	if (tracepoint) {
> @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
>  	const struct btf_type *t;
>  
>  	if (btf && ctx->funcname) {
> -		t = find_btf_func_proto(ctx->funcname);
> -		if (!IS_ERR(t))
> +		t = btf_find_func_proto(btf, ctx->funcname);
> +		if (!IS_ERR_OR_NULL(t))
>  			typestr = type_from_btf_id(btf, t->type);
>  	}
>  
> @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
>  
>  static bool is_btf_retval_void(const char *funcname)
>  {
> +	struct btf *btf = traceprobe_get_btf();
>  	const struct btf_type *t;
>  
> -	t = find_btf_func_proto(funcname);
> -	if (IS_ERR(t))
> +	if (!btf)
> +		return false;
> +
> +	t = btf_find_func_proto(btf, funcname);
> +	if (IS_ERR_OR_NULL(t))
>  		return false;
>  
>  	return t->type == 0;
> 
>

Masami Hiramatsu (Google) July 19, 2023, 3:15 p.m. UTC | #15

On Wed, 19 Jul 2023 10:09:48 +0800
Donglin Peng <[e-mail]> wrote:

> On Wed, Jul 19, 2023 at 7:03 AM Masami Hiramatsu <[e-mail]> wrote:
> >
> > On Tue, 18 Jul 2023 10:11:01 -0700
> > Alexei Starovoitov <[e-mail]> wrote:
> >
> > > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[e-mail]> wrote:
> > > >
> > > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > > Masami Hiramatsu (Google) <[e-mail]> wrote:
> > > >
> > > > > > >  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
> > > > > > >                                                    bool tracepoint)
> > > > > > >  {
> > > > > > > +       struct btf *btf = traceprobe_get_btf();
> > > > > >
> > > > > > I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> > > > > > if the function is
> > > > > > defined in a kernel module, we should get the module's btf.
> > > > > >
> > > > >
> > > > > Good catch! That should be a separated fix (or improvement?)
> > > > > I think it's better to use btf_get() and btf_put(), and pass btf via
> > > > > traceprobe_parse_context.
> > > >
> > > > Hmm, it seems that there is no exposed API to get the module's btf.
> > > > Should I use btf_idr and btf_idr_lock directly to find the corresponding
> > > > btf? If there isn't yet, I will add it too.
> > >
> > > There is bpf_find_btf_id.
> > > Probably drop 'static' from it and use it.
> >
> > Thanks! BTW, that API seems to search BTF type info by name. If user want to
> > specify a module name, do we need a new API? (Or expand the function to parse
> > a module name in given name?)
> 
> btf_get_module_btf can be used to get a module's btf, but we have to use
> find_module to get the module by its name firstly.

Thanks for the info! OK, this will be useful.

> 
> >
> > Thank you,
> > --
> > Masami Hiramatsu (Google) <[e-mail]>

Masami Hiramatsu (Google) July 19, 2023, 3:17 p.m. UTC | #16

On Tue, 18 Jul 2023 16:12:55 -0700
Alexei Starovoitov <[e-mail]> wrote:

> On Tue, Jul 18, 2023 at 4:03 PM Masami Hiramatsu <[e-mail]> wrote:
> >
> > On Tue, 18 Jul 2023 10:11:01 -0700
> > Alexei Starovoitov <[e-mail]> wrote:
> >
> > > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[e-mail]> wrote:
> > > >
> > > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > > Masami Hiramatsu (Google) <[e-mail]> wrote:
> > > >
> > > > > > >  static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr,
> > > > > > >                                                    bool tracepoint)
> > > > > > >  {
> > > > > > > +       struct btf *btf = traceprobe_get_btf();
> > > > > >
> > > > > > I found that traceprobe_get_btf() only returns the vmlinux's btf. But
> > > > > > if the function is
> > > > > > defined in a kernel module, we should get the module's btf.
> > > > > >
> > > > >
> > > > > Good catch! That should be a separated fix (or improvement?)
> > > > > I think it's better to use btf_get() and btf_put(), and pass btf via
> > > > > traceprobe_parse_context.
> > > >
> > > > Hmm, it seems that there is no exposed API to get the module's btf.
> > > > Should I use btf_idr and btf_idr_lock directly to find the corresponding
> > > > btf? If there isn't yet, I will add it too.
> > >
> > > There is bpf_find_btf_id.
> > > Probably drop 'static' from it and use it.
> >
> > Thanks! BTW, that API seems to search BTF type info by name. If user want to
> > specify a module name, do we need a new API? (Or expand the function to parse
> > a module name in given name?)
> 
> We can allow users specify module name, but how would it help?
> Do you want to allow static func names ?
> But module name won't help. There can be many statics with the same name
> in the module. Currently pahole filters out all ambiguous things in BTF.
> Alan is working on better representation of statics in BTF.
> The work is still in progress.

Ah, got it. So currently we don't have to worry about that case.

> 
> For now I don't see a need for an api to specify module, since it's not
> a modifier that can be relied upon to disambiguate.
> Hence bpf_find_btf_id that transparently searches across all should be enough.
> At least it was enough for all of bpf use cases.

OK. After updating the BTF I will revisit here.

Thank you!

Masami Hiramatsu (Google) July 19, 2023, 3:24 p.m. UTC | #17

On Wed, 19 Jul 2023 13:36:48 +0100
Alan Maguire <[e-mail]> wrote:

> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <[e-mail]>
> > 
> > Move generic function-proto find API and getting function parameter API
> > to BTF library code from trace_probe.c. This will avoid redundant efforts
> > on different feature.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
> > ---
> >  include/linux/btf.h        |    4 ++++
> >  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
> >  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
> >  3 files changed, 64 insertions(+), 35 deletions(-)
> > 
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index cac9f304e27a..98fbbcdd72ec 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -221,6 +221,10 @@ const struct btf_type *
> >  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
> >  		 u32 *type_size);
> >  const char *btf_type_str(const struct btf_type *t);
> > +const struct btf_type *btf_find_func_proto(struct btf *btf,
> > +					   const char *func_name);
> > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
> > +					   s32 *nr);
> >  
> >  #define for_each_member(i, struct_type, member)			\
> >  	for (i = 0, member = btf_type_member(struct_type);	\
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 817204d53372..e015b52956cb 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
> >  	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
> >  }
> >  
> > +/*
> > + * Find a functio proto type by name, and return it.
> > + * Return NULL if not found, or return -EINVAL if parameter is invalid.
> > + */
> > +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
> > +{
> > +	const struct btf_type *t;
> > +	s32 id;
> > +
> > +	if (!btf || !func_name)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
> 
> as mentioned in my other mail, there are cases where the function name
> may have a .isra.0 suffix, but the BTF representation will not. I looked
> at this and it seems like symbol names are validated via
> traceprobe_parse_event_name() - will this validation allow a "."-suffix
> name? I tried the following (with pahole v1.25 that emits BTF for
> schedule_work.isra.0):
> 
> [45454] FUNC 'schedule_work' type_id=45453 linkage=static

That's a good point! I'm checking fprobe, but kprobes actually
uses those suffixed names.

> 
> $ echo 'f schedule_work.isra.0 $arg*' >> dynamic_events
> bash: echo: write error: No such file or directory

So maybe fprobe doesn't accept that.

> So presuming that such "."-suffixed names are allowed, would it make
> sense to fall back to search BTF for the prefix ("schedule_work")
> instead of the full name ("schedule_work.isra.0"), as the former is what
> makes it into the BTF representation? Thanks!

OK, that's not a problem. My concern is that some "constprop" functions
will replace a part of parameter with constant value (so it can skip
passing some arguments). BTF argument may not work in such case.
Let me check it.

Thank you,

> 
> Alan
> 
> > +	if (id <= 0)
> > +		return NULL;
> > +
> > +	/* Get BTF_KIND_FUNC type */
> > +	t = btf_type_by_id(btf, id);
> > +	if (!t || !btf_type_is_func(t))
> > +		return NULL;
> > +
> > +	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> > +	t = btf_type_by_id(btf, t->type);
> > +	if (!t || !btf_type_is_func_proto(t))
> > +		return NULL;
> > +
> > +	return t;
> > +}
> > +
> > +/*
> > + * Get function parameter with the number of parameters.
> > + * This can return NULL if the function has no parameters.
> > + */
> > +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
> > +{
> > +	if (!func_proto || !nr)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	*nr = btf_type_vlen(func_proto);
> > +	if (*nr > 0)
> > +		return (const struct btf_param *)(func_proto + 1);
> > +	else
> > +		return NULL;
> > +}
> > +
> >  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
> >  {
> >  	while (type_id < btf->start_id)
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index c68a72707852..cd89fc1ebb42 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
> >  	return NULL;
> >  }
> >  
> > -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)
> > -		return ERR_PTR(-EINVAL);
> > -
> > -	id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
> > -	if (id <= 0)
> > -		return ERR_PTR(-ENOENT);
> > -
> > -	/* Get BTF_KIND_FUNC type */
> > -	t = btf_type_by_id(btf, id);
> > -	if (!t || !btf_type_is_func(t))
> > -		return ERR_PTR(-ENOENT);
> > -
> > -	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
> > -	t = btf_type_by_id(btf, t->type);
> > -	if (!t || !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,
> >  						   bool tracepoint)
> >  {
> > +	struct btf *btf = traceprobe_get_btf();
> >  	const struct btf_param *param;
> >  	const struct btf_type *t;
> >  
> > -	if (!funcname || !nr)
> > +	if (!funcname || !nr || !btf)
> >  		return ERR_PTR(-EINVAL);
> >  
> > -	t = find_btf_func_proto(funcname);
> > -	if (IS_ERR(t))
> > +	t = btf_find_func_proto(btf, funcname);
> > +	if (IS_ERR_OR_NULL(t))
> >  		return (const struct btf_param *)t;
> >  
> > -	*nr = btf_type_vlen(t);
> > -	param = (const struct btf_param *)(t + 1);
> > +	param = btf_get_func_param(t, nr);
> > +	if (IS_ERR_OR_NULL(param))
> > +		return param;
> >  
> >  	/* Hide the first 'data' argument of tracepoint */
> >  	if (tracepoint) {
> > @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
> >  	const struct btf_type *t;
> >  
> >  	if (btf && ctx->funcname) {
> > -		t = find_btf_func_proto(ctx->funcname);
> > -		if (!IS_ERR(t))
> > +		t = btf_find_func_proto(btf, ctx->funcname);
> > +		if (!IS_ERR_OR_NULL(t))
> >  			typestr = type_from_btf_id(btf, t->type);
> >  	}
> >  
> > @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
> >  
> >  static bool is_btf_retval_void(const char *funcname)
> >  {
> > +	struct btf *btf = traceprobe_get_btf();
> >  	const struct btf_type *t;
> >  
> > -	t = find_btf_func_proto(funcname);
> > -	if (IS_ERR(t))
> > +	if (!btf)
> > +		return false;
> > +
> > +	t = btf_find_func_proto(btf, funcname);
> > +	if (IS_ERR_OR_NULL(t))
> >  		return false;
> >  
> >  	return t->type == 0;
> > 
> >

Alan Maguire July 19, 2023, 3:49 p.m. UTC | #18

On 19/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> On Wed, 19 Jul 2023 13:36:48 +0100
> Alan Maguire <[e-mail]> wrote:
> 
>> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
>>> From: Masami Hiramatsu (Google) <[e-mail]>
>>>
>>> Move generic function-proto find API and getting function parameter API
>>> to BTF library code from trace_probe.c. This will avoid redundant efforts
>>> on different feature.
>>>
>>> Signed-off-by: Masami Hiramatsu (Google) <[e-mail]>
>>> ---
>>>  include/linux/btf.h        |    4 ++++
>>>  kernel/bpf/btf.c           |   45 ++++++++++++++++++++++++++++++++++++++++
>>>  kernel/trace/trace_probe.c |   50 +++++++++++++-------------------------------
>>>  3 files changed, 64 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index cac9f304e27a..98fbbcdd72ec 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -221,6 +221,10 @@ const struct btf_type *
>>>  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>>>  		 u32 *type_size);
>>>  const char *btf_type_str(const struct btf_type *t);
>>> +const struct btf_type *btf_find_func_proto(struct btf *btf,
>>> +					   const char *func_name);
>>> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
>>> +					   s32 *nr);
>>>  
>>>  #define for_each_member(i, struct_type, member)			\
>>>  	for (i = 0, member = btf_type_member(struct_type);	\
>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>> index 817204d53372..e015b52956cb 100644
>>> --- a/kernel/bpf/btf.c
>>> +++ b/kernel/bpf/btf.c
>>> @@ -1947,6 +1947,51 @@ btf_resolve_size(const struct btf *btf, const struct btf_type *type,
>>>  	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
>>>  }
>>>  
>>> +/*
>>> + * Find a functio proto type by name, and return it.
>>> + * Return NULL if not found, or return -EINVAL if parameter is invalid.
>>> + */
>>> +const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
>>> +{
>>> +	const struct btf_type *t;
>>> +	s32 id;
>>> +
>>> +	if (!btf || !func_name)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
>>
>> as mentioned in my other mail, there are cases where the function name
>> may have a .isra.0 suffix, but the BTF representation will not. I looked
>> at this and it seems like symbol names are validated via
>> traceprobe_parse_event_name() - will this validation allow a "."-suffix
>> name? I tried the following (with pahole v1.25 that emits BTF for
>> schedule_work.isra.0):
>>
>> [45454] FUNC 'schedule_work' type_id=45453 linkage=static
> 
> That's a good point! I'm checking fprobe, but kprobes actually
> uses those suffixed names.
> 
>>
>> $ echo 'f schedule_work.isra.0 $arg*' >> dynamic_events
>> bash: echo: write error: No such file or directory
> 
> So maybe fprobe doesn't accept that.
> 
>> So presuming that such "."-suffixed names are allowed, would it make
>> sense to fall back to search BTF for the prefix ("schedule_work")
>> instead of the full name ("schedule_work.isra.0"), as the former is what
>> makes it into the BTF representation? Thanks!
> 
> OK, that's not a problem. My concern is that some "constprop" functions
> will replace a part of parameter with constant value (so it can skip
> passing some arguments). BTF argument may not work in such case.
> Let me check it.
>

If the --btf_skip_inconsistent_proto option (also specified for pahole
v1.25) is working as expected, any such cases shouldn't make it into
BTF. The idea is we skip representing any static functions in BTF that

1. have multiple different function prototypes (since we don't yet have
good mechanisms to choose which one the user was referring to); or
2. use an unexpected register for a parameter. We gather that info from
DWARF and then make choices on whether to skip adding the function to
BTF or not.

See dwarf_loader.c in https://github.com/acmel/dwarves for more details
on the process used if needed.

The only exception for case 2 - where we allow unexpected registers - is
where multiple registers are used to represent a >64 bit structure
passed as a parameter by value. It might make sense to exclude such
functions from fprobe support (there's only a few of these functions in
the kernel if I remember, so it's no huge loss).

So long story short, if a function made it into in BTF, it is likely
using the registers you expect it to, unless it has a struct parameter.
It might be worth excluding such functions if you're worried about
getting unreliable data.

Thanks!

Alan

> Thank you,
> 
>>
>> Alan
>>
>>> +	if (id <= 0)
>>> +		return NULL;
>>> +
>>> +	/* Get BTF_KIND_FUNC type */
>>> +	t = btf_type_by_id(btf, id);
>>> +	if (!t || !btf_type_is_func(t))
>>> +		return NULL;
>>> +
>>> +	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
>>> +	t = btf_type_by_id(btf, t->type);
>>> +	if (!t || !btf_type_is_func_proto(t))
>>> +		return NULL;
>>> +
>>> +	return t;
>>> +}
>>> +
>>> +/*
>>> + * Get function parameter with the number of parameters.
>>> + * This can return NULL if the function has no parameters.
>>> + */
>>> +const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
>>> +{
>>> +	if (!func_proto || !nr)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	*nr = btf_type_vlen(func_proto);
>>> +	if (*nr > 0)
>>> +		return (const struct btf_param *)(func_proto + 1);
>>> +	else
>>> +		return NULL;
>>> +}
>>> +
>>>  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
>>>  {
>>>  	while (type_id < btf->start_id)
>>> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
>>> index c68a72707852..cd89fc1ebb42 100644
>>> --- a/kernel/trace/trace_probe.c
>>> +++ b/kernel/trace/trace_probe.c
>>> @@ -371,47 +371,23 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
>>>  	return NULL;
>>>  }
>>>  
>>> -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)
>>> -		return ERR_PTR(-EINVAL);
>>> -
>>> -	id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
>>> -	if (id <= 0)
>>> -		return ERR_PTR(-ENOENT);
>>> -
>>> -	/* Get BTF_KIND_FUNC type */
>>> -	t = btf_type_by_id(btf, id);
>>> -	if (!t || !btf_type_is_func(t))
>>> -		return ERR_PTR(-ENOENT);
>>> -
>>> -	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
>>> -	t = btf_type_by_id(btf, t->type);
>>> -	if (!t || !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,
>>>  						   bool tracepoint)
>>>  {
>>> +	struct btf *btf = traceprobe_get_btf();
>>>  	const struct btf_param *param;
>>>  	const struct btf_type *t;
>>>  
>>> -	if (!funcname || !nr)
>>> +	if (!funcname || !nr || !btf)
>>>  		return ERR_PTR(-EINVAL);
>>>  
>>> -	t = find_btf_func_proto(funcname);
>>> -	if (IS_ERR(t))
>>> +	t = btf_find_func_proto(btf, funcname);
>>> +	if (IS_ERR_OR_NULL(t))
>>>  		return (const struct btf_param *)t;
>>>  
>>> -	*nr = btf_type_vlen(t);
>>> -	param = (const struct btf_param *)(t + 1);
>>> +	param = btf_get_func_param(t, nr);
>>> +	if (IS_ERR_OR_NULL(param))
>>> +		return param;
>>>  
>>>  	/* Hide the first 'data' argument of tracepoint */
>>>  	if (tracepoint) {
>>> @@ -490,8 +466,8 @@ static const struct fetch_type *parse_btf_retval_type(
>>>  	const struct btf_type *t;
>>>  
>>>  	if (btf && ctx->funcname) {
>>> -		t = find_btf_func_proto(ctx->funcname);
>>> -		if (!IS_ERR(t))
>>> +		t = btf_find_func_proto(btf, ctx->funcname);
>>> +		if (!IS_ERR_OR_NULL(t))
>>>  			typestr = type_from_btf_id(btf, t->type);
>>>  	}
>>>  
>>> @@ -500,10 +476,14 @@ static const struct fetch_type *parse_btf_retval_type(
>>>  
>>>  static bool is_btf_retval_void(const char *funcname)
>>>  {
>>> +	struct btf *btf = traceprobe_get_btf();
>>>  	const struct btf_type *t;
>>>  
>>> -	t = find_btf_func_proto(funcname);
>>> -	if (IS_ERR(t))
>>> +	if (!btf)
>>> +		return false;
>>> +
>>> +	t = btf_find_func_proto(btf, funcname);
>>> +	if (IS_ERR_OR_NULL(t))
>>>  		return false;
>>>  
>>>  	return t->type == 0;
>>>
>>>
> 
>

Patch

diff --git a/include/linux/btf.h b/include/linux/btf.h
index cac9f304e27a..98fbbcdd72ec 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -221,6 +221,10 @@  const struct btf_type *
 btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 		 u32 *type_size);
 const char *btf_type_str(const struct btf_type *t);
+const struct btf_type *btf_find_func_proto(struct btf *btf,
+					   const char *func_name);
+const struct btf_param *btf_get_func_param(const struct btf_type *func_proto,
+					   s32 *nr);
 
 #define for_each_member(i, struct_type, member)			\
 	for (i = 0, member = btf_type_member(struct_type);	\
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 817204d53372..e015b52956cb 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1947,6 +1947,51 @@  btf_resolve_size(const struct btf *btf, const struct btf_type *type,
 	return __btf_resolve_size(btf, type, type_size, NULL, NULL, NULL, NULL);
 }
 
+/*
+ * Find a functio proto type by name, and return it.
+ * Return NULL if not found, or return -EINVAL if parameter is invalid.
+ */
+const struct btf_type *btf_find_func_proto(struct btf *btf, const char *func_name)
+{
+	const struct btf_type *t;
+	s32 id;
+
+	if (!btf || !func_name)
+		return ERR_PTR(-EINVAL);
+
+	id = btf_find_by_name_kind(btf, func_name, BTF_KIND_FUNC);
+	if (id <= 0)
+		return NULL;
+
+	/* Get BTF_KIND_FUNC type */
+	t = btf_type_by_id(btf, id);
+	if (!t || !btf_type_is_func(t))
+		return NULL;
+
+	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
+	t = btf_type_by_id(btf, t->type);
+	if (!t || !btf_type_is_func_proto(t))
+		return NULL;
+
+	return t;
+}
+
+/*
+ * Get function parameter with the number of parameters.
+ * This can return NULL if the function has no parameters.
+ */
+const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s32 *nr)
+{
+	if (!func_proto || !nr)
+		return ERR_PTR(-EINVAL);
+
+	*nr = btf_type_vlen(func_proto);
+	if (*nr > 0)
+		return (const struct btf_param *)(func_proto + 1);
+	else
+		return NULL;
+}
+
 static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
 {
 	while (type_id < btf->start_id)
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index c68a72707852..cd89fc1ebb42 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -371,47 +371,23 @@  static const char *type_from_btf_id(struct btf *btf, s32 id)
 	return NULL;
 }
 
-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)
-		return ERR_PTR(-EINVAL);
-
-	id = btf_find_by_name_kind(btf, funcname, BTF_KIND_FUNC);
-	if (id <= 0)
-		return ERR_PTR(-ENOENT);
-
-	/* Get BTF_KIND_FUNC type */
-	t = btf_type_by_id(btf, id);
-	if (!t || !btf_type_is_func(t))
-		return ERR_PTR(-ENOENT);
-
-	/* The type of BTF_KIND_FUNC is BTF_KIND_FUNC_PROTO */
-	t = btf_type_by_id(btf, t->type);
-	if (!t || !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,
 						   bool tracepoint)
 {
+	struct btf *btf = traceprobe_get_btf();
 	const struct btf_param *param;
 	const struct btf_type *t;
 
-	if (!funcname || !nr)
+	if (!funcname || !nr || !btf)
 		return ERR_PTR(-EINVAL);
 
-	t = find_btf_func_proto(funcname);
-	if (IS_ERR(t))
+	t = btf_find_func_proto(btf, funcname);
+	if (IS_ERR_OR_NULL(t))
 		return (const struct btf_param *)t;
 
-	*nr = btf_type_vlen(t);
-	param = (const struct btf_param *)(t + 1);
+	param = btf_get_func_param(t, nr);
+	if (IS_ERR_OR_NULL(param))
+		return param;
 
 	/* Hide the first 'data' argument of tracepoint */
 	if (tracepoint) {
@@ -490,8 +466,8 @@  static const struct fetch_type *parse_btf_retval_type(
 	const struct btf_type *t;
 
 	if (btf && ctx->funcname) {
-		t = find_btf_func_proto(ctx->funcname);
-		if (!IS_ERR(t))
+		t = btf_find_func_proto(btf, ctx->funcname);
+		if (!IS_ERR_OR_NULL(t))
 			typestr = type_from_btf_id(btf, t->type);
 	}
 
@@ -500,10 +476,14 @@  static const struct fetch_type *parse_btf_retval_type(
 
 static bool is_btf_retval_void(const char *funcname)
 {
+	struct btf *btf = traceprobe_get_btf();
 	const struct btf_type *t;
 
-	t = find_btf_func_proto(funcname);
-	if (IS_ERR(t))
+	if (!btf)
+		return false;
+
+	t = btf_find_func_proto(btf, funcname);
+	if (IS_ERR_OR_NULL(t))
 		return false;
 
 	return t->type == 0;