[PATCH v2 0/9] tracing: Improbe BTF support on probe events

[PATCH v2 0/9] tracing: Improbe BTF support on probe events

[Masami Hiramatsu (Google)]

Hi,

Here is the 2nd version of series to improve the BTF support on probe events.
The previous series is here:

https://lore.kernel.org/linux-trace-kernel/168699521817.528797.13179901018528120324.stgit@mhiramat.roam.corp.google.com/

In this version, I added a NULL check fix patch [1/9] (which will go to
fixes branch) and move BTF related API to kernel/bpf/btf.c [2/9] and add
a new BTF API [3/9] so that anyone can reuse it.
Also I decided to use '$retval' directly instead of 'retval' pseudo BTF
variable for field access at [5/9] because I introduced an idea to choose
function 'exit' event automatically if '$retval' is used [7/9]. With that
change, we can not use 'retval' because if a function has 'retval'
argument, we can not decide 'f func retval' is function exit or entry.
Selftest test case [8/9] and document [9/9] are also updated according to
those changes.

This series can be applied on top of "v6.5-rc2" kernel.

You can also get this series from:

git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git topic/fprobe-event-ext


Thank you,

---

Masami Hiramatsu (Google) (9):
      tracing/probes: Fix to add NULL check for BTF APIs
      bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF
      bpf/btf: Add a function to search a member of a struct/union
      tracing/probes: Support BTF based data structure field access
      tracing/probes: Support BTF field access from $retval
      tracing/probes: Add string type check with BTF
      tracing/fprobe-event: Assume fprobe is a return event by $retval
      selftests/ftrace: Add BTF fields access testcases
      Documentation: tracing: Update fprobe event example with BTF field


 Documentation/trace/fprobetrace.rst                |   50 ++
 include/linux/btf.h                                |    7 
 kernel/bpf/btf.c                                   |   83 ++++
 kernel/trace/trace_fprobe.c                        |   58 ++-
 kernel/trace/trace_probe.c                         |  402 +++++++++++++++-----
 kernel/trace/trace_probe.h                         |   12 +
 .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +
 .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    6 
 8 files changed, 503 insertions(+), 126 deletions(-)

--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH v2 1/9] tracing/probes: Fix to add NULL check for BTF APIs

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Since find_btf_func_param() abd btf_type_by_id() can return NULL,
the caller must check the return value correctly.

Fixes: b576e09701c7 ("tracing/probes: Support function parameters if BTF is available")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index b2b726bea1f9..c68a72707852 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -386,12 +386,12 @@ static const struct btf_type *find_btf_func_proto(const char *funcname)
 
 	/* Get BTF_KIND_FUNC type */
 	t = btf_type_by_id(btf, id);
-	if (!btf_type_is_func(t))
+	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 (!btf_type_is_func_proto(t))
+	if (!t || !btf_type_is_func_proto(t))
 		return ERR_PTR(-ENOENT);
 
 	return t;
@@ -443,7 +443,7 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
 	if (!ctx->params) {
 		params = find_btf_func_param(ctx->funcname, &ctx->nr_params,
 					     ctx->flags & TPARG_FL_TPOINT);
-		if (IS_ERR(params)) {
+		if (IS_ERR_OR_NULL(params)) {
 			trace_probe_log_err(ctx->offset, NO_BTF_ENTRY);
 			return PTR_ERR(params);
 		}
@@ -1273,7 +1273,7 @@ const char **traceprobe_expand_meta_args(int argc, const char *argv[],
 
 	params = find_btf_func_param(ctx->funcname, &nr_params,
 				     ctx->flags & TPARG_FL_TPOINT);
-	if (IS_ERR(params)) {
+	if (IS_ERR_OR_NULL(params)) {
 		if (args_idx != -1) {
 			/* $arg* requires BTF info */
 			trace_probe_log_err(0, NOSUP_BTFARG);

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

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

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) <mhiramat@kernel.org>
---
 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();
 	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;

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

[Steven Rostedt]

On Tue, 18 Jul 2023 00:23:37 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> 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) <mhiramat@kernel.org>
> ---
>  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;

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

[Masami Hiramatsu]

On Mon, 17 Jul 2023 14:39:14 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 18 Jul 2023 00:23:37 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > 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) <mhiramat@kernel.org>
> > ---
> >  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;
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Alexei Starovoitov]

On Mon, Jul 17, 2023 at 4:46 PM Masami Hiramatsu <mhiramat@kernel.org> 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.

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

[Masami Hiramatsu]

On Mon, 17 Jul 2023 16:51:29 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Mon, Jul 17, 2023 at 4:46 PM Masami Hiramatsu <mhiramat@kernel.org> 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!

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Steven Rostedt]

On Tue, 18 Jul 2023 08:46:34 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> 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);
> > > +

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

[Masami Hiramatsu]

On Mon, 17 Jul 2023 19:51:29 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 18 Jul 2023 08:46:34 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> 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) <mhiramat@kernel.org>

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

[Donglin Peng]

On Mon, Jul 17, 2023 at 11:24 PM Masami Hiramatsu (Google)
<mhiramat@kernel.org> wrote:
>
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> 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) <mhiramat@kernel.org>
> ---
>  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;
>
>

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

[Masami Hiramatsu]

On Tue, 18 Jul 2023 10:40:09 +0800
Donglin Peng <dolinux.peng@gmail.com> wrote:

> On Mon, Jul 17, 2023 at 11:24 PM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > 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) <mhiramat@kernel.org>
> > ---
> >  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) <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Tue, 18 Jul 2023 19:44:31 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> 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,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Alexei Starovoitov]

On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 18 Jul 2023 19:44:31 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> 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.

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

[Masami Hiramatsu]

On Tue, 18 Jul 2023 10:11:01 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Tue, 18 Jul 2023 19:44:31 +0900
> > Masami Hiramatsu (Google) <mhiramat@kernel.org> 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,
-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Alexei Starovoitov]

On Tue, Jul 18, 2023 at 4:03 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 18 Jul 2023 10:11:01 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org> 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.

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

[Masami Hiramatsu]

On Tue, 18 Jul 2023 16:12:55 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Tue, Jul 18, 2023 at 4:03 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Tue, 18 Jul 2023 10:11:01 -0700
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > >
> > > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> 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) <mhiramat@kernel.org>

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

[Donglin Peng]

On Wed, Jul 19, 2023 at 7:03 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Tue, 18 Jul 2023 10:11:01 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >
> > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > Masami Hiramatsu (Google) <mhiramat@kernel.org> 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) <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Wed, 19 Jul 2023 10:09:48 +0800
Donglin Peng <dolinux.peng@gmail.com> wrote:

> On Wed, Jul 19, 2023 at 7:03 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > On Tue, 18 Jul 2023 10:11:01 -0700
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > >
> > > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> 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) <mhiramat@kernel.org>


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Alan Maguire]

On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> 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) <mhiramat@kernel.org>
> ---
>  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;
> 
> 

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

[Masami Hiramatsu]

On Wed, 19 Jul 2023 13:36:48 +0100
Alan Maguire <alan.maguire@oracle.com> wrote:

> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > 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) <mhiramat@kernel.org>
> > ---
> >  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;
> > 
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Alan Maguire]

On 19/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> On Wed, 19 Jul 2023 13:36:48 +0100
> Alan Maguire <alan.maguire@oracle.com> wrote:
> 
>> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
>>> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>>>
>>> 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) <mhiramat@kernel.org>
>>> ---
>>>  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 v2 3/9] bpf/btf: Add a function to search a member of a struct/union

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add btf_find_struct_member() API to search a member of a given data structure
or union from the member's name.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/btf.h |    3 +++
 kernel/bpf/btf.c    |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 98fbbcdd72ec..097fe9b51562 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -225,6 +225,9 @@ 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);
+const struct btf_member *btf_find_struct_member(struct btf *btf,
+						const struct btf_type *type,
+						const char *member_name);
 
 #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 e015b52956cb..452ffb0393d6 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1992,6 +1992,44 @@ const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s3
 		return NULL;
 }
 
+/*
+ * Find a member of data structure/union by name and return it.
+ * Return NULL if not found, or -EINVAL if parameter is invalid.
+ */
+const struct btf_member *btf_find_struct_member(struct btf *btf,
+						const struct btf_type *type,
+						const char *member_name)
+{
+	const struct btf_member *members, *ret;
+	const char *name;
+	int i, vlen;
+
+	if (!btf || !member_name || !btf_type_is_struct(type))
+		return ERR_PTR(-EINVAL);
+
+	vlen = btf_type_vlen(type);
+	members = (const struct btf_member *)(type + 1);
+
+	for (i = 0; i < vlen; i++) {
+		if (!members[i].name_off) {
+			/* unnamed union: dig deeper */
+			type = btf_type_by_id(btf, members[i].type);
+			if (!IS_ERR_OR_NULL(type)) {
+				ret = btf_find_struct_member(btf, type,
+							     member_name);
+				if (!IS_ERR_OR_NULL(ret))
+					return ret;
+			}
+		} else {
+			name = btf_name_by_offset(btf, members[i].name_off);
+			if (name && !strcmp(member_name, name))
+				return &members[i];
+		}
+	}
+
+	return NULL;
+}
+
 static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
 {
 	while (type_id < btf->start_id)

Re: [PATCH v2 3/9] bpf/btf: Add a function to search a member of a struct/union

[Alan Maguire]

On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add btf_find_struct_member() API to search a member of a given data structure
> or union from the member's name.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

A few small things below, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  include/linux/btf.h |    3 +++
>  kernel/bpf/btf.c    |   38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 98fbbcdd72ec..097fe9b51562 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -225,6 +225,9 @@ 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);
> +const struct btf_member *btf_find_struct_member(struct btf *btf,
> +						const struct btf_type *type,
> +						const char *member_name);
>  
>  #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 e015b52956cb..452ffb0393d6 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -1992,6 +1992,44 @@ const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s3
>  		return NULL;
>  }
>  
> +/*
> + * Find a member of data structure/union by name and return it.
> + * Return NULL if not found, or -EINVAL if parameter is invalid.
> + */
> +const struct btf_member *btf_find_struct_member(struct btf *btf,
> +						const struct btf_type *type,
> +						const char *member_name)
> +{
> +	const struct btf_member *members, *ret;
> +	const char *name;
> +	int i, vlen;
> +
> +	if (!btf || !member_name || !btf_type_is_struct(type))
> +		return ERR_PTR(-EINVAL);
> +
> +	vlen = btf_type_vlen(type);
> +	members = (const struct btf_member *)(type + 1);
> +
> +	for (i = 0; i < vlen; i++) {

could use for_each_member() here I think, or perhaps use
btf_type_member(type) when getting member pointer above.

> +		if (!members[i].name_off) {
> +			/* unnamed union: dig deeper */
> +			type = btf_type_by_id(btf, members[i].type);
> +			if (!IS_ERR_OR_NULL(type)) {
> +				ret = btf_find_struct_member(btf, type,
> +							     member_name);

You'll need to skip modifiers before calling btf_find_struct_member()
here I think; it's possible to have a const anonymous union for example,
so to get to the union you'd need to skip the modifiers first. Otherwise
you could fail the btf_type_is_struct() test on re-entry.


> +				if (!IS_ERR_OR_NULL(ret))
> +					return ret;
> +			}
> +		} else {
> +			name = btf_name_by_offset(btf, members[i].name_off);
> +			if (name && !strcmp(member_name, name))
> +				return &members[i];
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
>  {
>  	while (type_id < btf->start_id)
> 
> 

Re: [PATCH v2 3/9] bpf/btf: Add a function to search a member of a struct/union

[Masami Hiramatsu]

On Thu, 20 Jul 2023 23:34:42 +0100
Alan Maguire <alan.maguire@oracle.com> wrote:

> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Add btf_find_struct_member() API to search a member of a given data structure
> > or union from the member's name.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> A few small things below, but
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks

> 
> > ---
> >  include/linux/btf.h |    3 +++
> >  kernel/bpf/btf.c    |   38 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 98fbbcdd72ec..097fe9b51562 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -225,6 +225,9 @@ 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);
> > +const struct btf_member *btf_find_struct_member(struct btf *btf,
> > +						const struct btf_type *type,
> > +						const char *member_name);
> >  
> >  #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 e015b52956cb..452ffb0393d6 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -1992,6 +1992,44 @@ const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s3
> >  		return NULL;
> >  }
> >  
> > +/*
> > + * Find a member of data structure/union by name and return it.
> > + * Return NULL if not found, or -EINVAL if parameter is invalid.
> > + */
> > +const struct btf_member *btf_find_struct_member(struct btf *btf,
> > +						const struct btf_type *type,
> > +						const char *member_name)
> > +{
> > +	const struct btf_member *members, *ret;
> > +	const char *name;
> > +	int i, vlen;
> > +
> > +	if (!btf || !member_name || !btf_type_is_struct(type))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	vlen = btf_type_vlen(type);
> > +	members = (const struct btf_member *)(type + 1);
> > +
> > +	for (i = 0; i < vlen; i++) {
> 
> could use for_each_member() here I think, or perhaps use
> btf_type_member(type) when getting member pointer above.

Thanks! I missed that macro.

> 
> > +		if (!members[i].name_off) {
> > +			/* unnamed union: dig deeper */
> > +			type = btf_type_by_id(btf, members[i].type);
> > +			if (!IS_ERR_OR_NULL(type)) {
> > +				ret = btf_find_struct_member(btf, type,
> > +							     member_name);
> 
> You'll need to skip modifiers before calling btf_find_struct_member()
> here I think; it's possible to have a const anonymous union for example,

Yeah, it is possible. Let me add it.

> so to get to the union you'd need to skip the modifiers first. Otherwise
> you could fail the btf_type_is_struct() test on re-entry.

Indeed. 

Thank you!

> 
> 
> > +				if (!IS_ERR_OR_NULL(ret))
> > +					return ret;
> > +			}
> > +		} else {
> > +			name = btf_name_by_offset(btf, members[i].name_off);
> > +			if (name && !strcmp(member_name, name))
> > +				return &members[i];
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  static u32 btf_resolved_type_id(const struct btf *btf, u32 type_id)
> >  {
> >  	while (type_id < btf->start_id)
> > 
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH v2 4/9] tracing/probes: Support BTF based data structure field access

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Using BTF to access the fields of a data structure. You can use this
for accessing the field with '->' or '.' operation with BTF argument.

 # echo 't sched_switch next=next->pid vruntime=next->se.vruntime' \
   > dynamic_events
 # echo 1 > events/tracepoints/sched_switch/enable
 # head -n 40 trace | tail
          <idle>-0       [000] d..3.   272.565382: sched_switch: (__probestub_sched_switch+0x4/0x10) next=26 vruntime=956533179
      kcompactd0-26      [000] d..3.   272.565406: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
          <idle>-0       [000] d..3.   273.069441: sched_switch: (__probestub_sched_switch+0x4/0x10) next=9 vruntime=956533179
     kworker/0:1-9       [000] d..3.   273.069464: sched_switch: (__probestub_sched_switch+0x4/0x10) next=26 vruntime=956579181
      kcompactd0-26      [000] d..3.   273.069480: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
          <idle>-0       [000] d..3.   273.141434: sched_switch: (__probestub_sched_switch+0x4/0x10) next=22 vruntime=956533179
    kworker/u2:1-22      [000] d..3.   273.141461: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
          <idle>-0       [000] d..3.   273.480872: sched_switch: (__probestub_sched_switch+0x4/0x10) next=22 vruntime=956585857
    kworker/u2:1-22      [000] d..3.   273.480905: sched_switch: (__probestub_sched_switch+0x4/0x10) next=70 vruntime=959533179
              sh-70      [000] d..3.   273.481102: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Use new BTF API for finding the member.
---
 kernel/trace/trace_probe.c |  229 +++++++++++++++++++++++++++++++++++++++-----
 kernel/trace/trace_probe.h |   11 ++
 2 files changed, 213 insertions(+), 27 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index cd89fc1ebb42..dd646d35637d 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -319,16 +319,14 @@ static u32 btf_type_int(const struct btf_type *t)
 	return *(u32 *)(t + 1);
 }
 
-static const char *type_from_btf_id(struct btf *btf, s32 id)
+static const char *fetch_type_from_btf_type(struct btf *btf,
+					const struct btf_type *type,
+					struct traceprobe_parse_context *ctx)
 {
-	const struct btf_type *t;
 	u32 intdata;
-	s32 tid;
 
 	/* TODO: const char * could be converted as a string */
-	t = btf_type_skip_modifiers(btf, id, &tid);
-
-	switch (BTF_INFO_KIND(t->info)) {
+	switch (BTF_INFO_KIND(type->info)) {
 	case BTF_KIND_ENUM:
 		/* enum is "int", so convert to "s32" */
 		return "s32";
@@ -341,7 +339,7 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
 		else
 			return "x32";
 	case BTF_KIND_INT:
-		intdata = btf_type_int(t);
+		intdata = btf_type_int(type);
 		if (BTF_INT_ENCODING(intdata) & BTF_INT_SIGNED) {
 			switch (BTF_INT_BITS(intdata)) {
 			case 8:
@@ -364,6 +362,10 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
 			case 64:
 				return "u64";
 			}
+			/* bitfield, size is encoded in the type */
+			ctx->last_bitsize = BTF_INT_BITS(intdata);
+			ctx->last_bitoffs += BTF_INT_OFFSET(intdata);
+			return "u64";
 		}
 	}
 	/* TODO: support other types */
@@ -401,12 +403,120 @@ static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr
 		return NULL;
 }
 
-static int parse_btf_arg(const char *varname, struct fetch_insn *code,
+/* Return 1 if the field separater is arrow operator ('->') */
+static int split_next_field(char *varname, char **next_field,
+			    struct traceprobe_parse_context *ctx)
+{
+	char *field;
+	int ret = 0;
+
+	field = strpbrk(varname, ".-");
+	if (field) {
+		if (field[0] == '-' && field[1] == '>') {
+			field[0] = '\0';
+			field += 2;
+			ret = 1;
+		} else if (field[0] == '.') {
+			field[0] = '\0';
+			field += 1;
+		} else {
+			trace_probe_log_err(ctx->offset + field - varname, BAD_HYPHEN);
+			return -EINVAL;
+		}
+		*next_field = field;
+	}
+
+	return ret;
+}
+
+/*
+ * Parse the field of data structure. The @type must be a pointer type
+ * pointing the target data structure type.
+ */
+static int parse_btf_field(char *fieldname, const struct btf_type *type,
+			   struct fetch_insn **pcode, struct fetch_insn *end,
+			   struct traceprobe_parse_context *ctx)
+{
+	struct btf *btf = traceprobe_get_btf();
+	struct fetch_insn *code = *pcode;
+	const struct btf_member *field;
+	u32 bitoffs;
+	char *next;
+	int is_ptr;
+	s32 tid;
+
+	do {
+		/* Outer loop for solving arrow operator ('->') */
+		if (BTF_INFO_KIND(type->info) != BTF_KIND_PTR) {
+			trace_probe_log_err(ctx->offset, NO_PTR_STRCT);
+			return -EINVAL;
+		}
+		/* Convert a struct pointer type to a struct type */
+		type = btf_type_skip_modifiers(btf, type->type, &tid);
+		if (!type) {
+			trace_probe_log_err(ctx->offset, BAD_BTF_TID);
+			return -EINVAL;
+		}
+
+		bitoffs = 0;
+		do {
+			/* Inner loop for solving dot operator ('.') */
+			next = NULL;
+			is_ptr = split_next_field(fieldname, &next, ctx);
+			if (is_ptr < 0)
+				return is_ptr;
+
+			field = btf_find_struct_member(btf, type, fieldname);
+			if (!field) {
+				trace_probe_log_err(ctx->offset, NO_BTF_FIELD);
+				return -ENOENT;
+			}
+
+			/* Accumulate the bit-offsets of the dot-connected fields */
+			if (btf_type_kflag(type)) {
+				bitoffs += BTF_MEMBER_BIT_OFFSET(field->offset);
+				ctx->last_bitsize = BTF_MEMBER_BITFIELD_SIZE(field->offset);
+			} else {
+				bitoffs += field->offset;
+				ctx->last_bitsize = 0;
+			}
+
+			type = btf_type_skip_modifiers(btf, field->type, &tid);
+			if (!type) {
+				trace_probe_log_err(ctx->offset, BAD_BTF_TID);
+				return -EINVAL;
+			}
+
+			ctx->offset += next - fieldname;
+			fieldname = next;
+		} while (!is_ptr && fieldname);
+
+		if (++code == end) {
+			trace_probe_log_err(ctx->offset, TOO_MANY_OPS);
+			return -EINVAL;
+		}
+		code->op = FETCH_OP_DEREF;	/* TODO: user deref support */
+		code->offset = bitoffs / 8;
+		*pcode = code;
+
+		ctx->last_bitoffs = bitoffs % 8;
+		ctx->last_type = type;
+	} while (fieldname);
+
+	return 0;
+}
+
+static int parse_btf_arg(char *varname,
+			 struct fetch_insn **pcode, struct fetch_insn *end,
 			 struct traceprobe_parse_context *ctx)
 {
 	struct btf *btf = traceprobe_get_btf();
+	struct fetch_insn *code = *pcode;
 	const struct btf_param *params;
-	int i;
+	const struct btf_type *type;
+	char *field = NULL;
+	int i, is_ptr;
+	u32 tid;
 
 	if (!btf) {
 		trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
@@ -416,6 +526,16 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
 	if (WARN_ON_ONCE(!ctx->funcname))
 		return -EINVAL;
 
+	is_ptr = split_next_field(varname, &field, ctx);
+	if (is_ptr < 0)
+		return is_ptr;
+	if (!is_ptr && field) {
+		/* dot-connected field on an argument is not supported. */
+		trace_probe_log_err(ctx->offset + field - varname,
+				    NOSUP_DAT_ARG);
+		return -EOPNOTSUPP;
+	}
+
 	if (!ctx->params) {
 		params = find_btf_func_param(ctx->funcname, &ctx->nr_params,
 					     ctx->flags & TPARG_FL_TPOINT);
@@ -436,24 +556,39 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
 				code->param = i + 1;
 			else
 				code->param = i;
-			return 0;
+
+			tid = params[i].type;
+			goto found;
 		}
 	}
 	trace_probe_log_err(ctx->offset, NO_BTFARG);
 	return -ENOENT;
+
+found:
+	type = btf_type_skip_modifiers(btf, tid, &tid);
+	if (!type) {
+		trace_probe_log_err(ctx->offset, BAD_BTF_TID);
+		return -EINVAL;
+	}
+	/* Initialize the last type information */
+	ctx->last_type = type;
+	ctx->last_bitoffs = 0;
+	ctx->last_bitsize = 0;
+	if (field) {
+		ctx->offset += field - varname;
+		return parse_btf_field(field, type, pcode, end, ctx);
+	}
+	return 0;
 }
 
-static const struct fetch_type *parse_btf_arg_type(int arg_idx,
+static const struct fetch_type *parse_btf_arg_type(
 					struct traceprobe_parse_context *ctx)
 {
 	struct btf *btf = traceprobe_get_btf();
 	const char *typestr = NULL;
 
-	if (btf && ctx->params) {
-		if (ctx->flags & TPARG_FL_TPOINT)
-			arg_idx--;
-		typestr = type_from_btf_id(btf, ctx->params[arg_idx].type);
-	}
+	if (btf && ctx->last_type)
+		typestr = fetch_type_from_btf_type(btf, ctx->last_type, ctx);
 
 	return find_fetch_type(typestr, ctx->flags);
 }
@@ -463,17 +598,43 @@ static const struct fetch_type *parse_btf_retval_type(
 {
 	struct btf *btf = traceprobe_get_btf();
 	const char *typestr = NULL;
-	const struct btf_type *t;
+	const struct btf_type *type;
+	s32 tid;
 
 	if (btf && ctx->funcname) {
-		t = btf_find_func_proto(btf, ctx->funcname);
-		if (!IS_ERR_OR_NULL(t))
-			typestr = type_from_btf_id(btf, t->type);
+		type = btf_find_func_proto(btf, ctx->funcname);
+		if (!IS_ERR_OR_NULL(type)) {
+			type = btf_type_skip_modifiers(btf, type->type, &tid);
+			if (!IS_ERR_OR_NULL(type))
+				typestr = fetch_type_from_btf_type(btf, type, ctx);
+		}
 	}
 
 	return find_fetch_type(typestr, ctx->flags);
 }
 
+static int parse_btf_bitfield(struct fetch_insn **pcode,
+			      struct traceprobe_parse_context *ctx)
+{
+	struct fetch_insn *code = *pcode;
+
+	if ((ctx->last_bitsize % 8 == 0) && ctx->last_bitoffs == 0)
+		return 0;
+
+	code++;
+	if (code->op != FETCH_OP_NOP) {
+		trace_probe_log_err(ctx->offset, TOO_MANY_OPS);
+		return -EINVAL;
+	}
+	*pcode = code;
+
+	code->op = FETCH_OP_MOD_BF;
+	code->lshift = 64 - (ctx->last_bitsize + ctx->last_bitoffs);
+	code->rshift = 64 - ctx->last_bitsize;
+	code->basesize = 64 / 8;
+	return 0;
+}
+
 static bool is_btf_retval_void(const char *funcname)
 {
 	struct btf *btf = traceprobe_get_btf();
@@ -500,14 +661,22 @@ static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
-static int parse_btf_arg(const char *varname, struct fetch_insn *code,
+static int parse_btf_arg(char *varname,
+			 struct fetch_insn **pcode, struct fetch_insn *end,
 			 struct traceprobe_parse_context *ctx)
 {
 	trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
 	return -EOPNOTSUPP;
 }
 
-#define parse_btf_arg_type(idx, ctx)		\
+static int parse_btf_bitfield(struct fetch_insn **pcode,
+			      struct traceprobe_parse_context *ctx)
+{
+	trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
+	return -EOPNOTSUPP;
+}
+
+#define parse_btf_arg_type(ctx)		\
 	find_fetch_type(NULL, ctx->flags)
 
 #define parse_btf_retval_type(ctx)		\
@@ -775,6 +944,8 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 
 			code->op = deref;
 			code->offset = offset;
+			/* Reset the last type if used */
+			ctx->last_type = NULL;
 		}
 		break;
 	case '\\':	/* Immediate value */
@@ -798,7 +969,7 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 				trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
 				return -EINVAL;
 			}
-			ret = parse_btf_arg(arg, code, ctx);
+			ret = parse_btf_arg(arg, pcode, end, ctx);
 			break;
 		}
 	}
@@ -944,6 +1115,7 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
 		goto out;
 	code[FETCH_INSN_MAX - 1].op = FETCH_OP_END;
 
+	ctx->last_type = NULL;
 	ret = parse_probe_arg(arg, parg->type, &code, &code[FETCH_INSN_MAX - 1],
 			      ctx);
 	if (ret)
@@ -951,9 +1123,9 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
 
 	/* Update storing type if BTF is available */
 	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)
+		if (ctx->last_type)
+			parg->type = parse_btf_arg_type(ctx);
+		else if (ctx->flags & TPARG_FL_RETURN)
 			parg->type = parse_btf_retval_type(ctx);
 	}
 
@@ -1028,6 +1200,11 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
 			trace_probe_log_err(ctx->offset + t - arg, BAD_BITFIELD);
 			goto fail;
 		}
+	} else if (IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS) &&
+		   ctx->last_type) {
+		ret = parse_btf_bitfield(&code, ctx);
+		if (ret)
+			goto fail;
 	}
 	ret = -EINVAL;
 	/* Loop(Array) operation */
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 01ea148723de..050909aaaa1b 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -384,6 +384,9 @@ static inline bool tparg_is_function_entry(unsigned int flags)
 struct traceprobe_parse_context {
 	struct trace_event_call *event;
 	const struct btf_param *params;
+	const struct btf_type *last_type;
+	u32 last_bitoffs;
+	u32 last_bitsize;
 	s32 nr_params;
 	const char *funcname;
 	unsigned int flags;
@@ -495,7 +498,13 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(BAD_VAR_ARGS,		"$arg* must be an independent parameter without name etc."),\
 	C(NOFENTRY_ARGS,	"$arg* can be used only on function entry"),	\
 	C(DOUBLE_ARGS,		"$arg* can be used only once in the parameters"),	\
-	C(ARGS_2LONG,		"$arg* failed because the argument list is too long"),
+	C(ARGS_2LONG,		"$arg* failed because the argument list is too long"),	\
+	C(ARGIDX_2BIG,		"$argN index is too big"),		\
+	C(NO_PTR_STRCT,		"This is not a pointer to union/structure."),	\
+	C(NOSUP_DAT_ARG,	"Non pointer structure/union argument is not supported."),\
+	C(BAD_HYPHEN,		"Failed to parse single hyphen. Forgot '>'?"),	\
+	C(NO_BTF_FIELD,		"This field is not found."),	\
+	C(BAD_BTF_TID,		"Failed to get BTF type info."),
 
 #undef C
 #define C(a, b)		TP_ERR_##a

Re: [PATCH v2 4/9] tracing/probes: Support BTF based data structure field access

[Alan Maguire]

On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Using BTF to access the fields of a data structure. You can use this
> for accessing the field with '->' or '.' operation with BTF argument.
> 
>  # echo 't sched_switch next=next->pid vruntime=next->se.vruntime' \
>    > dynamic_events
>  # echo 1 > events/tracepoints/sched_switch/enable
>  # head -n 40 trace | tail
>           <idle>-0       [000] d..3.   272.565382: sched_switch: (__probestub_sched_switch+0x4/0x10) next=26 vruntime=956533179
>       kcompactd0-26      [000] d..3.   272.565406: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
>           <idle>-0       [000] d..3.   273.069441: sched_switch: (__probestub_sched_switch+0x4/0x10) next=9 vruntime=956533179
>      kworker/0:1-9       [000] d..3.   273.069464: sched_switch: (__probestub_sched_switch+0x4/0x10) next=26 vruntime=956579181
>       kcompactd0-26      [000] d..3.   273.069480: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
>           <idle>-0       [000] d..3.   273.141434: sched_switch: (__probestub_sched_switch+0x4/0x10) next=22 vruntime=956533179
>     kworker/u2:1-22      [000] d..3.   273.141461: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
>           <idle>-0       [000] d..3.   273.480872: sched_switch: (__probestub_sched_switch+0x4/0x10) next=22 vruntime=956585857
>     kworker/u2:1-22      [000] d..3.   273.480905: sched_switch: (__probestub_sched_switch+0x4/0x10) next=70 vruntime=959533179
>               sh-70      [000] d..3.   273.481102: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

One issue below that I'm not totally clear on, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  Changes in v2:
>   - Use new BTF API for finding the member.
> ---
>  kernel/trace/trace_probe.c |  229 +++++++++++++++++++++++++++++++++++++++-----
>  kernel/trace/trace_probe.h |   11 ++
>  2 files changed, 213 insertions(+), 27 deletions(-)
> 
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index cd89fc1ebb42..dd646d35637d 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -319,16 +319,14 @@ static u32 btf_type_int(const struct btf_type *t)
>  	return *(u32 *)(t + 1);
>  }
>  
> -static const char *type_from_btf_id(struct btf *btf, s32 id)
> +static const char *fetch_type_from_btf_type(struct btf *btf,
> +					const struct btf_type *type,
> +					struct traceprobe_parse_context *ctx)
>  {
> -	const struct btf_type *t;
>  	u32 intdata;
> -	s32 tid;
>  
>  	/* TODO: const char * could be converted as a string */
> -	t = btf_type_skip_modifiers(btf, id, &tid);
> -
> -	switch (BTF_INFO_KIND(t->info)) {
> +	switch (BTF_INFO_KIND(type->info)) {
>  	case BTF_KIND_ENUM:
>  		/* enum is "int", so convert to "s32" */
>  		return "s32";
> @@ -341,7 +339,7 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
>  		else
>  			return "x32";
>  	case BTF_KIND_INT:
> -		intdata = btf_type_int(t);
> +		intdata = btf_type_int(type);
>  		if (BTF_INT_ENCODING(intdata) & BTF_INT_SIGNED) {
>  			switch (BTF_INT_BITS(intdata)) {
>  			case 8:
> @@ -364,6 +362,10 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
>  			case 64:
>  				return "u64";
>  			}
> +			/* bitfield, size is encoded in the type */
> +			ctx->last_bitsize = BTF_INT_BITS(intdata);
> +			ctx->last_bitoffs += BTF_INT_OFFSET(intdata);
> +			return "u64";
>  		}
>  	}
>  	/* TODO: support other types */
> @@ -401,12 +403,120 @@ static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr
>  		return NULL;
>  }
>  
> -static int parse_btf_arg(const char *varname, struct fetch_insn *code,
> +/* Return 1 if the field separater is arrow operator ('->') */
> +static int split_next_field(char *varname, char **next_field,
> +			    struct traceprobe_parse_context *ctx)
> +{
> +	char *field;
> +	int ret = 0;
> +
> +	field = strpbrk(varname, ".-");
> +	if (field) {
> +		if (field[0] == '-' && field[1] == '>') {
> +			field[0] = '\0';
> +			field += 2;
> +			ret = 1;
> +		} else if (field[0] == '.') {
> +			field[0] = '\0';
> +			field += 1;
> +		} else {
> +			trace_probe_log_err(ctx->offset + field - varname, BAD_HYPHEN);
> +			return -EINVAL;
> +		}
> +		*next_field = field;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Parse the field of data structure. The @type must be a pointer type
> + * pointing the target data structure type.
> + */
> +static int parse_btf_field(char *fieldname, const struct btf_type *type,
> +			   struct fetch_insn **pcode, struct fetch_insn *end,
> +			   struct traceprobe_parse_context *ctx)
> +{
> +	struct btf *btf = traceprobe_get_btf();
> +	struct fetch_insn *code = *pcode;
> +	const struct btf_member *field;
> +	u32 bitoffs;
> +	char *next;
> +	int is_ptr;
> +	s32 tid;
> +
> +	do {
> +		/* Outer loop for solving arrow operator ('->') */
> +		if (BTF_INFO_KIND(type->info) != BTF_KIND_PTR) {
> +			trace_probe_log_err(ctx->offset, NO_PTR_STRCT);
> +			return -EINVAL;
> +		}
> +		/* Convert a struct pointer type to a struct type */
> +		type = btf_type_skip_modifiers(btf, type->type, &tid);
> +		if (!type) {
> +			trace_probe_log_err(ctx->offset, BAD_BTF_TID);
> +			return -EINVAL;
> +		}
> +
> +		bitoffs = 0;
> +		do {
> +			/* Inner loop for solving dot operator ('.') */

one thing that's not totally clear to me is what combinations of '->'
and '.' are supported. It looks like parse_btf_arg() handles the outer
'->', but the comment seems to suggest that we expect only '.'
so foo->bar.baz, not foo->bar->baz.

> +			next = NULL;
> +			is_ptr = split_next_field(fieldname, &next, ctx);
> +			if (is_ptr < 0)
> +				return is_ptr;
> +

So if the above is right and we want to reject multiple pointer fields
like foo->bar->baz, shouldn't we error out if is_ptr == 1 here?

> +			field = btf_find_struct_member(btf, type, fieldname);
> +			if (!field) {
> +				trace_probe_log_err(ctx->offset, NO_BTF_FIELD);
> +				return -ENOENT;one
> +			}
> +
> +			/* Accumulate the bit-offsets of the dot-connected fields */
> +			if (btf_type_kflag(type)) {
> +				bitoffs += BTF_MEMBER_BIT_OFFSET(field->offset);
> +				ctx->last_bitsize = BTF_MEMBER_BITFIELD_SIZE(field->offset);
> +			} else {
> +				bitoffs += field->offset;
> +				ctx->last_bitsize = 0;
> +			}
> +
> +			type = btf_type_skip_modifiers(btf, field->type, &tid);
> +			if (!type) {
> +				trace_probe_log_err(ctx->offset, BAD_BTF_TID);
> +				return -EINVAL;
> +			}
> +
> +			ctx->offset += next - fieldname;
> +			fieldname = next;
> +		} while (!is_ptr && fieldname);
> +
> +		if (++code == end) {
> +			trace_probe_log_err(ctx->offset, TOO_MANY_OPS);
> +			return -EINVAL;
> +		}
> +		code->op = FETCH_OP_DEREF;	/* TODO: user deref support */
> +		code->offset = bitoffs / 8;
> +		*pcode = code;
> +
> +		ctx->last_bitoffs = bitoffs % 8;
> +		ctx->last_type = type;
> +	} while (fieldname);
> +
> +	return 0;
> +}
> +
> +static int parse_btf_arg(char *varname,
> +			 struct fetch_insn **pcode, struct fetch_insn *end,
>  			 struct traceprobe_parse_context *ctx)
>  {
>  	struct btf *btf = traceprobe_get_btf();
> +	struct fetch_insn *code = *pcode;
>  	const struct btf_param *params;
> -	int i;
> +	const struct btf_type *type;
> +	char *field = NULL;
> +	int i, is_ptr;
> +	u32 tid;
>  
>  	if (!btf) {
>  		trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
> @@ -416,6 +526,16 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
>  	if (WARN_ON_ONCE(!ctx->funcname))
>  		return -EINVAL;
>  
> +	is_ptr = split_next_field(varname, &field, ctx);
> +	if (is_ptr < 0)
> +		return is_ptr;
> +	if (!is_ptr && field) {
> +		/* dot-connected field on an argument is not supported. */
> +		trace_probe_log_err(ctx->offset + field - varname,
> +				    NOSUP_DAT_ARG);
> +		return -EOPNOTSUPP;
> +	}
> +
>  	if (!ctx->params) {
>  		params = find_btf_func_param(ctx->funcname, &ctx->nr_params,
>  					     ctx->flags & TPARG_FL_TPOINT);
> @@ -436,24 +556,39 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
>  				code->param = i + 1;
>  			else
>  				code->param = i;
> -			return 0;
> +
> +			tid = params[i].type;
> +			goto found;
>  		}
>  	}
>  	trace_probe_log_err(ctx->offset, NO_BTFARG);
>  	return -ENOENT;
> +
> +found:
> +	type = btf_type_skip_modifiers(btf, tid, &tid);
> +	if (!type) {
> +		trace_probe_log_err(ctx->offset, BAD_BTF_TID);
> +		return -EINVAL;
> +	}
> +	/* Initialize the last type information */
> +	ctx->last_type = type;
> +	ctx->last_bitoffs = 0;
> +	ctx->last_bitsize = 0;
> +	if (field) {
> +		ctx->offset += field - varname;
> +		return parse_btf_field(field, type, pcode, end, ctx);
> +	}
> +	return 0;
>  }
>  
> -static const struct fetch_type *parse_btf_arg_type(int arg_idx,
> +static const struct fetch_type *parse_btf_arg_type(
>  					struct traceprobe_parse_context *ctx)
>  {
>  	struct btf *btf = traceprobe_get_btf();
>  	const char *typestr = NULL;
>  
> -	if (btf && ctx->params) {
> -		if (ctx->flags & TPARG_FL_TPOINT)
> -			arg_idx--;
> -		typestr = type_from_btf_id(btf, ctx->params[arg_idx].type);
> -	}
> +	if (btf && ctx->last_type)
> +		typestr = fetch_type_from_btf_type(btf, ctx->last_type, ctx);
>  
>  	return find_fetch_type(typestr, ctx->flags);
>  }
> @@ -463,17 +598,43 @@ static const struct fetch_type *parse_btf_retval_type(
>  {
>  	struct btf *btf = traceprobe_get_btf();
>  	const char *typestr = NULL;
> -	const struct btf_type *t;
> +	const struct btf_type *type;
> +	s32 tid;
>  
>  	if (btf && ctx->funcname) {
> -		t = btf_find_func_proto(btf, ctx->funcname);
> -		if (!IS_ERR_OR_NULL(t))
> -			typestr = type_from_btf_id(btf, t->type);
> +		type = btf_find_func_proto(btf, ctx->funcname);
> +		if (!IS_ERR_OR_NULL(type)) {
> +			type = btf_type_skip_modifiers(btf, type->type, &tid);
> +			if (!IS_ERR_OR_NULL(type))
> +				typestr = fetch_type_from_btf_type(btf, type, ctx);
> +		}
>  	}
>  
>  	return find_fetch_type(typestr, ctx->flags);
>  }
>  
> +static int parse_btf_bitfield(struct fetch_insn **pcode,
> +			      struct traceprobe_parse_context *ctx)
> +{
> +	struct fetch_insn *code = *pcode;
> +
> +	if ((ctx->last_bitsize % 8 == 0) && ctx->last_bitoffs == 0)
> +		return 0;
> +
> +	code++;
> +	if (code->op != FETCH_OP_NOP) {
> +		trace_probe_log_err(ctx->offset, TOO_MANY_OPS);
> +		return -EINVAL;
> +	}
> +	*pcode = code;
> +
> +	code->op = FETCH_OP_MOD_BF;
> +	code->lshift = 64 - (ctx->last_bitsize + ctx->last_bitoffs);
> +	code->rshift = 64 - ctx->last_bitsize;
> +	code->basesize = 64 / 8;
> +	return 0;
> +}
> +
>  static bool is_btf_retval_void(const char *funcname)
>  {
>  	struct btf *btf = traceprobe_get_btf();
> @@ -500,14 +661,22 @@ static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr
>  	return ERR_PTR(-EOPNOTSUPP);
>  }
>  
> -static int parse_btf_arg(const char *varname, struct fetch_insn *code,
> +static int parse_btf_arg(char *varname,
> +			 struct fetch_insn **pcode, struct fetch_insn *end,
>  			 struct traceprobe_parse_context *ctx)
>  {
>  	trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
>  	return -EOPNOTSUPP;
>  }
>  
> -#define parse_btf_arg_type(idx, ctx)		\
> +static int parse_btf_bitfield(struct fetch_insn **pcode,
> +			      struct traceprobe_parse_context *ctx)
> +{
> +	trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
> +	return -EOPNOTSUPP;
> +}
> +
> +#define parse_btf_arg_type(ctx)		\
>  	find_fetch_type(NULL, ctx->flags)
>  
>  #define parse_btf_retval_type(ctx)		\
> @@ -775,6 +944,8 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
>  
>  			code->op = deref;
>  			code->offset = offset;
> +			/* Reset the last type if used */
> +			ctx->last_type = NULL;
>  		}
>  		break;
>  	case '\\':	/* Immediate value */
> @@ -798,7 +969,7 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
>  				trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
>  				return -EINVAL;
>  			}
> -			ret = parse_btf_arg(arg, code, ctx);
> +			ret = parse_btf_arg(arg, pcode, end, ctx);
>  			break;
>  		}
>  	}
> @@ -944,6 +1115,7 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
>  		goto out;
>  	code[FETCH_INSN_MAX - 1].op = FETCH_OP_END;
>  
> +	ctx->last_type = NULL;
>  	ret = parse_probe_arg(arg, parg->type, &code, &code[FETCH_INSN_MAX - 1],
>  			      ctx);
>  	if (ret)
> @@ -951,9 +1123,9 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
>  
>  	/* Update storing type if BTF is available */
>  	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)
> +		if (ctx->last_type)
> +			parg->type = parse_btf_arg_type(ctx);
> +		else if (ctx->flags & TPARG_FL_RETURN)
>  			parg->type = parse_btf_retval_type(ctx);
>  	}
>  
> @@ -1028,6 +1200,11 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
>  			trace_probe_log_err(ctx->offset + t - arg, BAD_BITFIELD);
>  			goto fail;
>  		}
> +	} else if (IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS) &&
> +		   ctx->last_type) {
> +		ret = parse_btf_bitfield(&code, ctx);
> +		if (ret)
> +			goto fail;
>  	}
>  	ret = -EINVAL;
>  	/* Loop(Array) operation */
> diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> index 01ea148723de..050909aaaa1b 100644
> --- a/kernel/trace/trace_probe.h
> +++ b/kernel/trace/trace_probe.h
> @@ -384,6 +384,9 @@ static inline bool tparg_is_function_entry(unsigned int flags)
>  struct traceprobe_parse_context {
>  	struct trace_event_call *event;
>  	const struct btf_param *params;
> +	const struct btf_type *last_type;
> +	u32 last_bitoffs;
> +	u32 last_bitsize;
>  	s32 nr_params;
>  	const char *funcname;
>  	unsigned int flags;
> @@ -495,7 +498,13 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
>  	C(BAD_VAR_ARGS,		"$arg* must be an independent parameter without name etc."),\
>  	C(NOFENTRY_ARGS,	"$arg* can be used only on function entry"),	\
>  	C(DOUBLE_ARGS,		"$arg* can be used only once in the parameters"),	\
> -	C(ARGS_2LONG,		"$arg* failed because the argument list is too long"),
> +	C(ARGS_2LONG,		"$arg* failed because the argument list is too long"),	\
> +	C(ARGIDX_2BIG,		"$argN index is too big"),		\
> +	C(NO_PTR_STRCT,		"This is not a pointer to union/structure."),	\
> +	C(NOSUP_DAT_ARG,	"Non pointer structure/union argument is not supported."),\
> +	C(BAD_HYPHEN,		"Failed to parse single hyphen. Forgot '>'?"),	\
> +	C(NO_BTF_FIELD,		"This field is not found."),	\
> +	C(BAD_BTF_TID,		"Failed to get BTF type info."),
>  
>  #undef C
>  #define C(a, b)		TP_ERR_##a
> 
> 

Re: [PATCH v2 4/9] tracing/probes: Support BTF based data structure field access

[Masami Hiramatsu]

On Thu, 20 Jul 2023 23:51:17 +0100
Alan Maguire <alan.maguire@oracle.com> wrote:

> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Using BTF to access the fields of a data structure. You can use this
> > for accessing the field with '->' or '.' operation with BTF argument.
> > 
> >  # echo 't sched_switch next=next->pid vruntime=next->se.vruntime' \
> >    > dynamic_events
> >  # echo 1 > events/tracepoints/sched_switch/enable
> >  # head -n 40 trace | tail
> >           <idle>-0       [000] d..3.   272.565382: sched_switch: (__probestub_sched_switch+0x4/0x10) next=26 vruntime=956533179
> >       kcompactd0-26      [000] d..3.   272.565406: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
> >           <idle>-0       [000] d..3.   273.069441: sched_switch: (__probestub_sched_switch+0x4/0x10) next=9 vruntime=956533179
> >      kworker/0:1-9       [000] d..3.   273.069464: sched_switch: (__probestub_sched_switch+0x4/0x10) next=26 vruntime=956579181
> >       kcompactd0-26      [000] d..3.   273.069480: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
> >           <idle>-0       [000] d..3.   273.141434: sched_switch: (__probestub_sched_switch+0x4/0x10) next=22 vruntime=956533179
> >     kworker/u2:1-22      [000] d..3.   273.141461: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
> >           <idle>-0       [000] d..3.   273.480872: sched_switch: (__probestub_sched_switch+0x4/0x10) next=22 vruntime=956585857
> >     kworker/u2:1-22      [000] d..3.   273.480905: sched_switch: (__probestub_sched_switch+0x4/0x10) next=70 vruntime=959533179
> >               sh-70      [000] d..3.   273.481102: sched_switch: (__probestub_sched_switch+0x4/0x10) next=0 vruntime=0
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> One issue below that I'm not totally clear on, but
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks,

> > ---
> >  Changes in v2:
> >   - Use new BTF API for finding the member.
> > ---
> >  kernel/trace/trace_probe.c |  229 +++++++++++++++++++++++++++++++++++++++-----
> >  kernel/trace/trace_probe.h |   11 ++
> >  2 files changed, 213 insertions(+), 27 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index cd89fc1ebb42..dd646d35637d 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -319,16 +319,14 @@ static u32 btf_type_int(const struct btf_type *t)
> >  	return *(u32 *)(t + 1);
> >  }
> >  
> > -static const char *type_from_btf_id(struct btf *btf, s32 id)
> > +static const char *fetch_type_from_btf_type(struct btf *btf,
> > +					const struct btf_type *type,
> > +					struct traceprobe_parse_context *ctx)
> >  {
> > -	const struct btf_type *t;
> >  	u32 intdata;
> > -	s32 tid;
> >  
> >  	/* TODO: const char * could be converted as a string */
> > -	t = btf_type_skip_modifiers(btf, id, &tid);
> > -
> > -	switch (BTF_INFO_KIND(t->info)) {
> > +	switch (BTF_INFO_KIND(type->info)) {
> >  	case BTF_KIND_ENUM:
> >  		/* enum is "int", so convert to "s32" */
> >  		return "s32";
> > @@ -341,7 +339,7 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
> >  		else
> >  			return "x32";
> >  	case BTF_KIND_INT:
> > -		intdata = btf_type_int(t);
> > +		intdata = btf_type_int(type);
> >  		if (BTF_INT_ENCODING(intdata) & BTF_INT_SIGNED) {
> >  			switch (BTF_INT_BITS(intdata)) {
> >  			case 8:
> > @@ -364,6 +362,10 @@ static const char *type_from_btf_id(struct btf *btf, s32 id)
> >  			case 64:
> >  				return "u64";
> >  			}
> > +			/* bitfield, size is encoded in the type */
> > +			ctx->last_bitsize = BTF_INT_BITS(intdata);
> > +			ctx->last_bitoffs += BTF_INT_OFFSET(intdata);
> > +			return "u64";
> >  		}
> >  	}
> >  	/* TODO: support other types */
> > @@ -401,12 +403,120 @@ static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr
> >  		return NULL;
> >  }
> >  
> > -static int parse_btf_arg(const char *varname, struct fetch_insn *code,
> > +/* Return 1 if the field separater is arrow operator ('->') */
> > +static int split_next_field(char *varname, char **next_field,
> > +			    struct traceprobe_parse_context *ctx)
> > +{
> > +	char *field;
> > +	int ret = 0;
> > +
> > +	field = strpbrk(varname, ".-");
> > +	if (field) {
> > +		if (field[0] == '-' && field[1] == '>') {
> > +			field[0] = '\0';
> > +			field += 2;
> > +			ret = 1;
> > +		} else if (field[0] == '.') {
> > +			field[0] = '\0';
> > +			field += 1;
> > +		} else {
> > +			trace_probe_log_err(ctx->offset + field - varname, BAD_HYPHEN);
> > +			return -EINVAL;
> > +		}
> > +		*next_field = field;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Parse the field of data structure. The @type must be a pointer type
> > + * pointing the target data structure type.
> > + */
> > +static int parse_btf_field(char *fieldname, const struct btf_type *type,
> > +			   struct fetch_insn **pcode, struct fetch_insn *end,
> > +			   struct traceprobe_parse_context *ctx)
> > +{
> > +	struct btf *btf = traceprobe_get_btf();
> > +	struct fetch_insn *code = *pcode;
> > +	const struct btf_member *field;
> > +	u32 bitoffs;
> > +	char *next;
> > +	int is_ptr;
> > +	s32 tid;
> > +
> > +	do {
> > +		/* Outer loop for solving arrow operator ('->') */
> > +		if (BTF_INFO_KIND(type->info) != BTF_KIND_PTR) {
> > +			trace_probe_log_err(ctx->offset, NO_PTR_STRCT);
> > +			return -EINVAL;
> > +		}
> > +		/* Convert a struct pointer type to a struct type */
> > +		type = btf_type_skip_modifiers(btf, type->type, &tid);
> > +		if (!type) {
> > +			trace_probe_log_err(ctx->offset, BAD_BTF_TID);
> > +			return -EINVAL;
> > +		}
> > +
> > +		bitoffs = 0;
> > +		do {
> > +			/* Inner loop for solving dot operator ('.') */
> 
> one thing that's not totally clear to me is what combinations of '->'
> and '.' are supported. It looks like parse_btf_arg() handles the outer
> '->', but the comment seems to suggest that we expect only '.'
> so foo->bar.baz, not foo->bar->baz.

Currently what this is not supported is "argX.foo". In "foo->bar->baz"
case, inner loop will pass once and outer loop pass twice.

> 
> > +			next = NULL;
> > +			is_ptr = split_next_field(fieldname, &next, ctx);
> > +			if (is_ptr < 0)
> > +				return is_ptr;
> > +
> 
> So if the above is right and we want to reject multiple pointer fields
> like foo->bar->baz, shouldn't we error out if is_ptr == 1 here?

No, "foo->bar->baz" is supported.

> 
> > +			field = btf_find_struct_member(btf, type, fieldname);
> > +			if (!field) {
> > +				trace_probe_log_err(ctx->offset, NO_BTF_FIELD);
> > +				return -ENOENT;
> > +			}
> > +
> > +			/* Accumulate the bit-offsets of the dot-connected fields */
> > +			if (btf_type_kflag(type)) {
> > +				bitoffs += BTF_MEMBER_BIT_OFFSET(field->offset);
> > +				ctx->last_bitsize = BTF_MEMBER_BITFIELD_SIZE(field->offset);
> > +			} else {
> > +				bitoffs += field->offset;
> > +				ctx->last_bitsize = 0;
> > +			}

This just add the (bit)offset of the given member,

> > +
> > +			type = btf_type_skip_modifiers(btf, field->type, &tid);
> > +			if (!type) {
> > +				trace_probe_log_err(ctx->offset, BAD_BTF_TID);
> > +				return -EINVAL;
> > +			}
> > +
> > +			ctx->offset += next - fieldname;
> > +			fieldname = next;
> > +		} while (!is_ptr && fieldname);

If we find the 2nd or subsequent '->', exit the inner loop because
`is_ptr == 1` here.

> > +
> > +		if (++code == end) {
> > +			trace_probe_log_err(ctx->offset, TOO_MANY_OPS);
> > +			return -EINVAL;
> > +		}
> > +		code->op = FETCH_OP_DEREF;	/* TODO: user deref support */
> > +		code->offset = bitoffs / 8;
> > +		*pcode = code;

Here, we need to add FETCH_OP_DEREF with the accumulated bit offset.
This dereference the given pointer (foo, at the first loop) with
member offset (bar, at the first loop.)
In the second loop, this member offset is baz from bar.
This operation means "*(ptr + offsetof(member))". Thus, if we have
"foo->bar.baz", it is converted as "*(foo + offsetof(bar.baz))".
The inner loop is "offsetof(bar.baz)" part, and outer loop is 
"*(ptr + inner-loop-result)" part.

So, "foo->bar->baz" is converted to 
"*( *(foo + offsetof(bar)) + offsetof(baz) )". Thus this needs to do
the outer loop twice.

Thank you,

> > +
> > +		ctx->last_bitoffs = bitoffs % 8;
> > +		ctx->last_type = type;
> > +	} while (fieldname);
> > +
> > +	return 0;
> > +}
> > +
> > +static int parse_btf_arg(char *varname,
> > +			 struct fetch_insn **pcode, struct fetch_insn *end,
> >  			 struct traceprobe_parse_context *ctx)
> >  {
> >  	struct btf *btf = traceprobe_get_btf();
> > +	struct fetch_insn *code = *pcode;
> >  	const struct btf_param *params;
> > -	int i;
> > +	const struct btf_type *type;
> > +	char *field = NULL;
> > +	int i, is_ptr;
> > +	u32 tid;
> >  
> >  	if (!btf) {
> >  		trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
> > @@ -416,6 +526,16 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
> >  	if (WARN_ON_ONCE(!ctx->funcname))
> >  		return -EINVAL;
> >  
> > +	is_ptr = split_next_field(varname, &field, ctx);
> > +	if (is_ptr < 0)
> > +		return is_ptr;
> > +	if (!is_ptr && field) {
> > +		/* dot-connected field on an argument is not supported. */
> > +		trace_probe_log_err(ctx->offset + field - varname,
> > +				    NOSUP_DAT_ARG);
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> >  	if (!ctx->params) {
> >  		params = find_btf_func_param(ctx->funcname, &ctx->nr_params,
> >  					     ctx->flags & TPARG_FL_TPOINT);
> > @@ -436,24 +556,39 @@ static int parse_btf_arg(const char *varname, struct fetch_insn *code,
> >  				code->param = i + 1;
> >  			else
> >  				code->param = i;
> > -			return 0;
> > +
> > +			tid = params[i].type;
> > +			goto found;
> >  		}
> >  	}
> >  	trace_probe_log_err(ctx->offset, NO_BTFARG);
> >  	return -ENOENT;
> > +
> > +found:
> > +	type = btf_type_skip_modifiers(btf, tid, &tid);
> > +	if (!type) {
> > +		trace_probe_log_err(ctx->offset, BAD_BTF_TID);
> > +		return -EINVAL;
> > +	}
> > +	/* Initialize the last type information */
> > +	ctx->last_type = type;
> > +	ctx->last_bitoffs = 0;
> > +	ctx->last_bitsize = 0;
> > +	if (field) {
> > +		ctx->offset += field - varname;
> > +		return parse_btf_field(field, type, pcode, end, ctx);
> > +	}
> > +	return 0;
> >  }
> >  
> > -static const struct fetch_type *parse_btf_arg_type(int arg_idx,
> > +static const struct fetch_type *parse_btf_arg_type(
> >  					struct traceprobe_parse_context *ctx)
> >  {
> >  	struct btf *btf = traceprobe_get_btf();
> >  	const char *typestr = NULL;
> >  
> > -	if (btf && ctx->params) {
> > -		if (ctx->flags & TPARG_FL_TPOINT)
> > -			arg_idx--;
> > -		typestr = type_from_btf_id(btf, ctx->params[arg_idx].type);
> > -	}
> > +	if (btf && ctx->last_type)
> > +		typestr = fetch_type_from_btf_type(btf, ctx->last_type, ctx);
> >  
> >  	return find_fetch_type(typestr, ctx->flags);
> >  }
> > @@ -463,17 +598,43 @@ static const struct fetch_type *parse_btf_retval_type(
> >  {
> >  	struct btf *btf = traceprobe_get_btf();
> >  	const char *typestr = NULL;
> > -	const struct btf_type *t;
> > +	const struct btf_type *type;
> > +	s32 tid;
> >  
> >  	if (btf && ctx->funcname) {
> > -		t = btf_find_func_proto(btf, ctx->funcname);
> > -		if (!IS_ERR_OR_NULL(t))
> > -			typestr = type_from_btf_id(btf, t->type);
> > +		type = btf_find_func_proto(btf, ctx->funcname);
> > +		if (!IS_ERR_OR_NULL(type)) {
> > +			type = btf_type_skip_modifiers(btf, type->type, &tid);
> > +			if (!IS_ERR_OR_NULL(type))
> > +				typestr = fetch_type_from_btf_type(btf, type, ctx);
> > +		}
> >  	}
> >  
> >  	return find_fetch_type(typestr, ctx->flags);
> >  }
> >  
> > +static int parse_btf_bitfield(struct fetch_insn **pcode,
> > +			      struct traceprobe_parse_context *ctx)
> > +{
> > +	struct fetch_insn *code = *pcode;
> > +
> > +	if ((ctx->last_bitsize % 8 == 0) && ctx->last_bitoffs == 0)
> > +		return 0;
> > +
> > +	code++;
> > +	if (code->op != FETCH_OP_NOP) {
> > +		trace_probe_log_err(ctx->offset, TOO_MANY_OPS);
> > +		return -EINVAL;
> > +	}
> > +	*pcode = code;
> > +
> > +	code->op = FETCH_OP_MOD_BF;
> > +	code->lshift = 64 - (ctx->last_bitsize + ctx->last_bitoffs);
> > +	code->rshift = 64 - ctx->last_bitsize;
> > +	code->basesize = 64 / 8;
> > +	return 0;
> > +}
> > +
> >  static bool is_btf_retval_void(const char *funcname)
> >  {
> >  	struct btf *btf = traceprobe_get_btf();
> > @@ -500,14 +661,22 @@ static const struct btf_param *find_btf_func_param(const char *funcname, s32 *nr
> >  	return ERR_PTR(-EOPNOTSUPP);
> >  }
> >  
> > -static int parse_btf_arg(const char *varname, struct fetch_insn *code,
> > +static int parse_btf_arg(char *varname,
> > +			 struct fetch_insn **pcode, struct fetch_insn *end,
> >  			 struct traceprobe_parse_context *ctx)
> >  {
> >  	trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
> >  	return -EOPNOTSUPP;
> >  }
> >  
> > -#define parse_btf_arg_type(idx, ctx)		\
> > +static int parse_btf_bitfield(struct fetch_insn **pcode,
> > +			      struct traceprobe_parse_context *ctx)
> > +{
> > +	trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +#define parse_btf_arg_type(ctx)		\
> >  	find_fetch_type(NULL, ctx->flags)
> >  
> >  #define parse_btf_retval_type(ctx)		\
> > @@ -775,6 +944,8 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
> >  
> >  			code->op = deref;
> >  			code->offset = offset;
> > +			/* Reset the last type if used */
> > +			ctx->last_type = NULL;
> >  		}
> >  		break;
> >  	case '\\':	/* Immediate value */
> > @@ -798,7 +969,7 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
> >  				trace_probe_log_err(ctx->offset, NOSUP_BTFARG);
> >  				return -EINVAL;
> >  			}
> > -			ret = parse_btf_arg(arg, code, ctx);
> > +			ret = parse_btf_arg(arg, pcode, end, ctx);
> >  			break;
> >  		}
> >  	}
> > @@ -944,6 +1115,7 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
> >  		goto out;
> >  	code[FETCH_INSN_MAX - 1].op = FETCH_OP_END;
> >  
> > +	ctx->last_type = NULL;
> >  	ret = parse_probe_arg(arg, parg->type, &code, &code[FETCH_INSN_MAX - 1],
> >  			      ctx);
> >  	if (ret)
> > @@ -951,9 +1123,9 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
> >  
> >  	/* Update storing type if BTF is available */
> >  	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)
> > +		if (ctx->last_type)
> > +			parg->type = parse_btf_arg_type(ctx);
> > +		else if (ctx->flags & TPARG_FL_RETURN)
> >  			parg->type = parse_btf_retval_type(ctx);
> >  	}
> >  
> > @@ -1028,6 +1200,11 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
> >  			trace_probe_log_err(ctx->offset + t - arg, BAD_BITFIELD);
> >  			goto fail;
> >  		}
> > +	} else if (IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS) &&
> > +		   ctx->last_type) {
> > +		ret = parse_btf_bitfield(&code, ctx);
> > +		if (ret)
> > +			goto fail;
> >  	}
> >  	ret = -EINVAL;
> >  	/* Loop(Array) operation */
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 01ea148723de..050909aaaa1b 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -384,6 +384,9 @@ static inline bool tparg_is_function_entry(unsigned int flags)
> >  struct traceprobe_parse_context {
> >  	struct trace_event_call *event;
> >  	const struct btf_param *params;
> > +	const struct btf_type *last_type;
> > +	u32 last_bitoffs;
> > +	u32 last_bitsize;
> >  	s32 nr_params;
> >  	const char *funcname;
> >  	unsigned int flags;
> > @@ -495,7 +498,13 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
> >  	C(BAD_VAR_ARGS,		"$arg* must be an independent parameter without name etc."),\
> >  	C(NOFENTRY_ARGS,	"$arg* can be used only on function entry"),	\
> >  	C(DOUBLE_ARGS,		"$arg* can be used only once in the parameters"),	\
> > -	C(ARGS_2LONG,		"$arg* failed because the argument list is too long"),
> > +	C(ARGS_2LONG,		"$arg* failed because the argument list is too long"),	\
> > +	C(ARGIDX_2BIG,		"$argN index is too big"),		\
> > +	C(NO_PTR_STRCT,		"This is not a pointer to union/structure."),	\
> > +	C(NOSUP_DAT_ARG,	"Non pointer structure/union argument is not supported."),\
> > +	C(BAD_HYPHEN,		"Failed to parse single hyphen. Forgot '>'?"),	\
> > +	C(NO_BTF_FIELD,		"This field is not found."),	\
> > +	C(BAD_BTF_TID,		"Failed to get BTF type info."),
> >  
> >  #undef C
> >  #define C(a, b)		TP_ERR_##a
> > 
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH v2 5/9] tracing/probes: Support BTF field access from $retval

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Support BTF argument on '$retval' for function return events including
kretprobe and fprobe for accessing the return value.
This also allows user to access its fields if the return value is a
pointer of a data structure.

E.g.
 # echo 'f getname_flags%return +0($retval->name):string' \
   > dynamic_events
 # echo 1 > events/fprobes/getname_flags__exit/enable
 # ls > /dev/null
 # head -n 40 trace | tail
              ls-87      [000] ...1.  8067.616101: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./function_profile_enabled"
              ls-87      [000] ...1.  8067.616108: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./trace_stat"
              ls-87      [000] ...1.  8067.616115: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./set_graph_notrace"
              ls-87      [000] ...1.  8067.616122: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./set_graph_function"
              ls-87      [000] ...1.  8067.616129: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./set_ftrace_notrace"
              ls-87      [000] ...1.  8067.616135: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./set_ftrace_filter"
              ls-87      [000] ...1.  8067.616143: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./touched_functions"
              ls-87      [000] ...1.  8067.616237: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./enabled_functions"
              ls-87      [000] ...1.  8067.616245: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./available_filter_functions"
              ls-87      [000] ...1.  8067.616253: getname_flags__exit: (vfs_fstatat+0x3c/0x70 <- getname_flags) arg1="./set_ftrace_notrace_pid"


Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Use '$retval' instead of 'retval' because it is confusing.
---
 kernel/trace/trace_probe.c |   98 +++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 61 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index dd646d35637d..4442ff9c2728 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -536,6 +536,22 @@ static int parse_btf_arg(char *varname,
 		return -EOPNOTSUPP;
 	}
 
+	if (ctx->flags & TPARG_FL_RETURN) {
+		if (strcmp(varname, "$retval") != 0) {
+			trace_probe_log_err(ctx->offset, NO_BTFARG);
+			return -ENOENT;
+		}
+		/* Check whether the function return type is not void */
+		type = btf_find_func_proto(btf, ctx->funcname);
+		if (!IS_ERR_OR_NULL(type) && type->type == 0) {
+			trace_probe_log_err(ctx->offset, NO_RETVAL);
+			return -ENOENT;
+		}
+		code->op = FETCH_OP_RETVAL;
+		tid = type->type;
+		goto found;
+	}
+
 	if (!ctx->params) {
 		params = find_btf_func_param(ctx->funcname, &ctx->nr_params,
 					     ctx->flags & TPARG_FL_TPOINT);
@@ -556,7 +572,6 @@ static int parse_btf_arg(char *varname,
 				code->param = i + 1;
 			else
 				code->param = i;
-
 			tid = params[i].type;
 			goto found;
 		}
@@ -581,7 +596,7 @@ static int parse_btf_arg(char *varname,
 	return 0;
 }
 
-static const struct fetch_type *parse_btf_arg_type(
+static const struct fetch_type *find_fetch_type_from_btf_type(
 					struct traceprobe_parse_context *ctx)
 {
 	struct btf *btf = traceprobe_get_btf();
@@ -593,26 +608,6 @@ static const struct fetch_type *parse_btf_arg_type(
 	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 *type;
-	s32 tid;
-
-	if (btf && ctx->funcname) {
-		type = btf_find_func_proto(btf, ctx->funcname);
-		if (!IS_ERR_OR_NULL(type)) {
-			type = btf_type_skip_modifiers(btf, type->type, &tid);
-			if (!IS_ERR_OR_NULL(type))
-				typestr = fetch_type_from_btf_type(btf, type, ctx);
-		}
-	}
-
-	return find_fetch_type(typestr, ctx->flags);
-}
-
 static int parse_btf_bitfield(struct fetch_insn **pcode,
 			      struct traceprobe_parse_context *ctx)
 {
@@ -635,20 +630,6 @@ static int parse_btf_bitfield(struct fetch_insn **pcode,
 	return 0;
 }
 
-static bool is_btf_retval_void(const char *funcname)
-{
-	struct btf *btf = traceprobe_get_btf();
-	const struct btf_type *t;
-
-	if (!btf)
-		return false;
-
-	t = btf_find_func_proto(btf, funcname);
-	if (IS_ERR_OR_NULL(t))
-		return false;
-
-	return t->type == 0;
-}
 #else
 static struct btf *traceprobe_get_btf(void)
 {
@@ -676,24 +657,23 @@ static int parse_btf_bitfield(struct fetch_insn **pcode,
 	return -EOPNOTSUPP;
 }
 
-#define parse_btf_arg_type(ctx)		\
-	find_fetch_type(NULL, ctx->flags)
-
-#define parse_btf_retval_type(ctx)		\
+#define find_fetch_type_from_btf_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))
 
-static int parse_probe_vars(char *arg, const struct fetch_type *t,
-			    struct fetch_insn *code,
+/* Parse $vars. @orig_arg points '$', which syncs to @ctx->offset */
+static int parse_probe_vars(char *orig_arg, const struct fetch_type *t,
+			    struct fetch_insn **pcode,
+			    struct fetch_insn *end,
 			    struct traceprobe_parse_context *ctx)
 {
-	unsigned long param;
+	struct fetch_insn *code = *pcode;
 	int err = TP_ERR_BAD_VAR;
+	char *arg = orig_arg + 1;
+	unsigned long param;
 	int ret = 0;
 	int len;
 
@@ -712,18 +692,17 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
 		goto inval;
 	}
 
-	if (strcmp(arg, "retval") == 0) {
-		if (ctx->flags & TPARG_FL_RETURN) {
-			if ((ctx->flags & TPARG_FL_KERNEL) &&
-			    is_btf_retval_void(ctx->funcname)) {
-				err = TP_ERR_NO_RETVAL;
-				goto inval;
-			}
+	if (str_has_prefix(arg, "retval")) {
+		if (!(ctx->flags & TPARG_FL_RETURN)) {
+			err = TP_ERR_RETVAL_ON_PROBE;
+			goto inval;
+		}
+		if (!(ctx->flags & TPARG_FL_KERNEL) ||
+		    !IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS)) {
 			code->op = FETCH_OP_RETVAL;
 			return 0;
 		}
-		err = TP_ERR_RETVAL_ON_PROBE;
-		goto inval;
+		return parse_btf_arg(orig_arg, pcode, end, ctx);
 	}
 
 	len = str_has_prefix(arg, "stack");
@@ -825,7 +804,7 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 
 	switch (arg[0]) {
 	case '$':
-		ret = parse_probe_vars(arg + 1, type, code, ctx);
+		ret = parse_probe_vars(arg, type, pcode, end, ctx);
 		break;
 
 	case '%':	/* named register */
@@ -1122,12 +1101,9 @@ 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) {
-		if (ctx->last_type)
-			parg->type = parse_btf_arg_type(ctx);
-		else if (ctx->flags & TPARG_FL_RETURN)
-			parg->type = parse_btf_retval_type(ctx);
-	}
+	if (IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS) &&
+	    !t && ctx->last_type)
+		parg->type = find_fetch_type_from_btf_type(ctx);
 
 	ret = -EINVAL;
 	/* Store operation */

[PATCH v2 6/9] tracing/probes: Add string type check with BTF

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add a string type checking with BTF information if possible.
This will check whether the given BTF argument (and field) is
signed char array or pointer to signed char. If not, it reject
the 'string' type. If it is pointer to signed char, it adds
a dereference opration so that it can correctly fetch the
string data from memory.

 # echo 'f getname_flags%return retval->name:string' >> dynamic_events
 # echo 't sched_switch next->comm:string' >> dynamic_events

The above cases, 'struct filename::name' is 'char *' and
'struct task_struct::comm' is 'char []'. But in both case,
user can specify ':string' to fetch the string data.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |   89 +++++++++++++++++++++++++++++++++++++++++++-
 kernel/trace/trace_probe.h |    3 +
 2 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 4442ff9c2728..4d1dccb8724b 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -319,6 +319,77 @@ static u32 btf_type_int(const struct btf_type *t)
 	return *(u32 *)(t + 1);
 }
 
+static bool btf_type_is_char_ptr(struct btf *btf, const struct btf_type *type)
+{
+	const struct btf_type *real_type;
+	u32 intdata;
+	s32 tid;
+
+	real_type = btf_type_skip_modifiers(btf, type->type, &tid);
+	if (!real_type)
+		return false;
+
+	if (BTF_INFO_KIND(real_type->info) != BTF_KIND_INT)
+		return false;
+
+	intdata = btf_type_int(real_type);
+	return !(BTF_INT_ENCODING(intdata) & BTF_INT_SIGNED)
+		&& BTF_INT_BITS(intdata) == 8;
+}
+
+static bool btf_type_is_char_array(struct btf *btf, const struct btf_type *type)
+{
+	const struct btf_type *real_type;
+	const struct btf_array *array;
+	u32 intdata;
+	s32 tid;
+
+	if (BTF_INFO_KIND(type->info) != BTF_KIND_ARRAY)
+		return false;
+
+	array = (const struct btf_array *)(type + 1);
+
+	real_type = btf_type_skip_modifiers(btf, array->type, &tid);
+
+	intdata = btf_type_int(real_type);
+	return !(BTF_INT_ENCODING(intdata) & BTF_INT_SIGNED)
+		&& BTF_INT_BITS(intdata) == 8;
+}
+
+static int check_prepare_btf_string_fetch(char *typename,
+				struct fetch_insn **pcode,
+				struct traceprobe_parse_context *ctx)
+{
+	struct btf *btf = traceprobe_get_btf();
+
+	if (!btf || !ctx->last_type)
+		return 0;
+
+	/* char [] does not need any change. */
+	if (btf_type_is_char_array(btf, ctx->last_type))
+		return 0;
+
+	/* char * requires dereference the pointer. */
+	if (btf_type_is_char_ptr(btf, ctx->last_type)) {
+		struct fetch_insn *code = *pcode + 1;
+
+		if (code->op == FETCH_OP_END) {
+			trace_probe_log_err(ctx->offset, TOO_MANY_OPS);
+			return -E2BIG;
+		}
+		if (typename[0] == 'u')
+			code->op = FETCH_OP_UDEREF;
+		else
+			code->op = FETCH_OP_DEREF;
+		code->offset = 0;
+		*pcode = code;
+		return 0;
+	}
+	/* Other types are not available for string */
+	trace_probe_log_err(ctx->offset, BAD_TYPE4STR);
+	return -EINVAL;
+}
+
 static const char *fetch_type_from_btf_type(struct btf *btf,
 					const struct btf_type *type,
 					struct traceprobe_parse_context *ctx)
@@ -660,6 +731,13 @@ static int parse_btf_bitfield(struct fetch_insn **pcode,
 #define find_fetch_type_from_btf_type(ctx)		\
 	find_fetch_type(NULL, ctx->flags)
 
+static int check_prepare_btf_string_fetch(char *typename,
+				struct fetch_insn **pcode,
+				struct traceprobe_parse_context *ctx)
+{
+	return 0;
+}
+
 #endif
 
 #define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
@@ -1102,8 +1180,15 @@ static int traceprobe_parse_probe_arg_body(const char *argv, ssize_t *size,
 
 	/* Update storing type if BTF is available */
 	if (IS_ENABLED(CONFIG_PROBE_EVENTS_BTF_ARGS) &&
-	    !t && ctx->last_type)
-		parg->type = find_fetch_type_from_btf_type(ctx);
+	    ctx->last_type) {
+		if (!t) {
+			parg->type = find_fetch_type_from_btf_type(ctx);
+		} else if (strstr(t, "string")) {
+			ret = check_prepare_btf_string_fetch(t, &code, ctx);
+			if (ret)
+				goto fail;
+		}
+	}
 
 	ret = -EINVAL;
 	/* Store operation */
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 050909aaaa1b..604d6fb9c5ff 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -504,7 +504,8 @@ extern int traceprobe_define_arg_fields(struct trace_event_call *event_call,
 	C(NOSUP_DAT_ARG,	"Non pointer structure/union argument is not supported."),\
 	C(BAD_HYPHEN,		"Failed to parse single hyphen. Forgot '>'?"),	\
 	C(NO_BTF_FIELD,		"This field is not found."),	\
-	C(BAD_BTF_TID,		"Failed to get BTF type info."),
+	C(BAD_BTF_TID,		"Failed to get BTF type info."),\
+	C(BAD_TYPE4STR,		"This type does not fit for string."),
 
 #undef C
 #define C(a, b)		TP_ERR_##a

[PATCH v2 7/9] tracing/fprobe-event: Assume fprobe is a return event by $retval

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Assume the fprobe event is a return event if there is $retval is
used in the probe's argument without %return. e.g.

echo 'f:myevent vfs_read $retval' >> dynamic_events

then 'myevent' is a return probe event.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_fprobe.c                        |   58 +++++++++++++++-----
 .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    2 -
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index dfe2e546acdc..3ac0e04071aa 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -898,6 +898,46 @@ static struct tracepoint *find_tracepoint(const char *tp_name)
 	return data.tpoint;
 }
 
+static int parse_symbol_and_return(int argc, const char *argv[],
+				   char **symbol, bool *is_return,
+				   bool is_tracepoint)
+{
+	char *tmp = strchr(argv[1], '%');
+	int i;
+
+	if (tmp) {
+		int len = tmp - argv[1];
+
+		if (!is_tracepoint && !strcmp(tmp, "%return")) {
+			*is_return = true;
+		} else {
+			trace_probe_log_err(len, BAD_ADDR_SUFFIX);
+			return -EINVAL;
+		}
+		*symbol = kmemdup_nul(argv[1], len, GFP_KERNEL);
+	} else
+		*symbol = kstrdup(argv[1], GFP_KERNEL);
+	if (!*symbol)
+		return -ENOMEM;
+
+	if (*is_return)
+		return 0;
+
+	/* If there is $retval, this should be a return fprobe. */
+	for (i = 2; i < argc; i++) {
+		tmp = strstr(argv[i], "$retval");
+		if (tmp && !isalnum(tmp[7]) && tmp[7] != '_') {
+			*is_return = true;
+			/*
+			 * NOTE: Don't check is_tracepoint here, because it will
+			 * be checked when the argument is parsed.
+			 */
+			break;
+		}
+	}
+	return 0;
+}
+
 static int __trace_fprobe_create(int argc, const char *argv[])
 {
 	/*
@@ -927,7 +967,7 @@ static int __trace_fprobe_create(int argc, const char *argv[])
 	struct trace_fprobe *tf = NULL;
 	int i, len, new_argc = 0, ret = 0;
 	bool is_return = false;
-	char *symbol = NULL, *tmp = NULL;
+	char *symbol = NULL;
 	const char *event = NULL, *group = FPROBE_EVENT_SYSTEM;
 	const char **new_argv = NULL;
 	int maxactive = 0;
@@ -983,20 +1023,10 @@ static int __trace_fprobe_create(int argc, const char *argv[])
 	trace_probe_log_set_index(1);
 
 	/* a symbol(or tracepoint) must be specified */
-	symbol = kstrdup(argv[1], GFP_KERNEL);
-	if (!symbol)
-		return -ENOMEM;
+	ret = parse_symbol_and_return(argc, argv, &symbol, &is_return, is_tracepoint);
+	if (ret < 0)
+		goto parse_error;
 
-	tmp = strchr(symbol, '%');
-	if (tmp) {
-		if (!is_tracepoint && !strcmp(tmp, "%return")) {
-			*tmp = '\0';
-			is_return = true;
-		} else {
-			trace_probe_log_err(tmp - symbol, BAD_ADDR_SUFFIX);
-			goto parse_error;
-		}
-	}
 	if (!is_return && maxactive) {
 		trace_probe_log_set_index(0);
 		trace_probe_log_err(1, BAD_MAXACT_TYPE);
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
index 812f5b3f6055..72563b2e0812 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
@@ -30,11 +30,11 @@ check_error 'f:^ vfs_read'		# NO_EVENT_NAME
 check_error 'f:foo/^12345678901234567890123456789012345678901234567890123456789012345 vfs_read'	# EVENT_TOO_LONG
 check_error 'f:foo/^bar.1 vfs_read'	# BAD_EVENT_NAME
 
-check_error 'f vfs_read ^$retval'	# RETVAL_ON_PROBE
 check_error 'f vfs_read ^$stack10000'	# BAD_STACK_NUM
 
 check_error 'f vfs_read ^$arg10000'	# BAD_ARG_NUM
 
+check_error 'f vfs_read $retval ^$arg1' # BAD_VAR
 check_error 'f vfs_read ^$none_var'	# BAD_VAR
 check_error 'f vfs_read ^'$REG		# BAD_VAR
 

[PATCH v2 8/9] selftests/ftrace: Add BTF fields access testcases

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add test cases for accessing the data structure fields using BTF info.
This includes the field access from parameters and retval, and accessing
string information.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v2:
 - Use '$retval' instead of 'retval'.
 - Add a test that use both '$retval' and '$arg1' for fprobe.
---
 .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +++++++++++
 .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    4 ++++
 2 files changed, 15 insertions(+)

diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
index b89de1771655..93b94468967b 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
@@ -21,6 +21,8 @@ echo 0 > events/enable
 echo > dynamic_events
 
 TP=kfree
+TP2=kmem_cache_alloc
+TP3=getname_flags
 
 if [ "$FPROBES" ] ; then
 echo "f:fpevent $TP object" >> dynamic_events
@@ -33,6 +35,7 @@ echo > dynamic_events
 
 echo "f:fpevent $TP "'$arg1' >> dynamic_events
 grep -q "fpevent.*object=object" dynamic_events
+
 echo > dynamic_events
 
 echo "f:fpevent $TP "'$arg*' >> dynamic_events
@@ -45,6 +48,14 @@ fi
 
 echo > dynamic_events
 
+echo "t:tpevent $TP2 name=s->name:string" >> dynamic_events
+echo "f:fpevent ${TP3}%return path=\$retval->name:string" >> dynamic_events
+
+grep -q "tpevent.*name=s->name:string" dynamic_events
+grep -q "fpevent.*path=\$retval->name:string" dynamic_events
+
+echo > dynamic_events
+
 if [ "$KPROBES" ] ; then
 echo "p:kpevent $TP object" >> dynamic_events
 grep -q "kpevent.*object=object" dynamic_events
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
index 72563b2e0812..49758f77c923 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
@@ -103,6 +103,10 @@ check_error 'f vfs_read%return ^$arg*'		# NOFENTRY_ARGS
 check_error 'f vfs_read ^hoge'			# NO_BTFARG
 check_error 'f kfree ^$arg10'			# NO_BTFARG (exceed the number of parameters)
 check_error 'f kfree%return ^$retval'		# NO_RETVAL
+check_error 'f vfs_read%return $retval->^foo'	# NO_PTR_STRCT
+check_error 'f vfs_read file->^foo'		# NO_BTF_FIELD
+check_error 'f vfs_read file^-.foo'		# BAD_HYPHEN
+check_error 'f vfs_read ^file:string'		# BAD_TYPE4STR
 else
 check_error 'f vfs_read ^$arg*'			# NOSUP_BTFARG
 check_error 't kfree ^$arg*'			# NOSUP_BTFARG

Re: [PATCH v2 8/9] selftests/ftrace: Add BTF fields access testcases

[Alan Maguire]

On 17/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add test cases for accessing the data structure fields using BTF info.
> This includes the field access from parameters and retval, and accessing
> string information.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

One suggestion below, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
> Changes in v2:
>  - Use '$retval' instead of 'retval'.
>  - Add a test that use both '$retval' and '$arg1' for fprobe.
> ---
>  .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +++++++++++
>  .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    4 ++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
> index b89de1771655..93b94468967b 100644
> --- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
> +++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
> @@ -21,6 +21,8 @@ echo 0 > events/enable
>  echo > dynamic_events
>  
>  TP=kfree
> +TP2=kmem_cache_alloc
> +TP3=getname_flags
>  
>  if [ "$FPROBES" ] ; then
>  echo "f:fpevent $TP object" >> dynamic_events
> @@ -33,6 +35,7 @@ echo > dynamic_events
>  
>  echo "f:fpevent $TP "'$arg1' >> dynamic_events
>  grep -q "fpevent.*object=object" dynamic_events
> +
>  echo > dynamic_events
>  
>  echo "f:fpevent $TP "'$arg*' >> dynamic_events
> @@ -45,6 +48,14 @@ fi
>  
>  echo > dynamic_events
>  
> +echo "t:tpevent $TP2 name=s->name:string" >> dynamic_events
> +echo "f:fpevent ${TP3}%return path=\$retval->name:string" >> dynamic_events
> +

could we test a numeric value like kmem_cache_alloc object size?
also if combos of -> and . are allowed, would be good to test one of
those too.

> +grep -q "tpevent.*name=s->name:string" dynamic_events
> +grep -q "fpevent.*path=\$retval->name:string" dynamic_events
> +
> +echo > dynamic_events
> +
>  if [ "$KPROBES" ] ; then
>  echo "p:kpevent $TP object" >> dynamic_events
>  grep -q "kpevent.*object=object" dynamic_events
> diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
> index 72563b2e0812..49758f77c923 100644
> --- a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
> +++ b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
> @@ -103,6 +103,10 @@ check_error 'f vfs_read%return ^$arg*'		# NOFENTRY_ARGS
>  check_error 'f vfs_read ^hoge'			# NO_BTFARG
>  check_error 'f kfree ^$arg10'			# NO_BTFARG (exceed the number of parameters)
>  check_error 'f kfree%return ^$retval'		# NO_RETVAL
> +check_error 'f vfs_read%return $retval->^foo'	# NO_PTR_STRCT
> +check_error 'f vfs_read file->^foo'		# NO_BTF_FIELD
> +check_error 'f vfs_read file^-.foo'		# BAD_HYPHEN
> +check_error 'f vfs_read ^file:string'		# BAD_TYPE4STR
>  else
>  check_error 'f vfs_read ^$arg*'			# NOSUP_BTFARG
>  check_error 't kfree ^$arg*'			# NOSUP_BTFARG
> 
> 

Re: [PATCH v2 8/9] selftests/ftrace: Add BTF fields access testcases

[Masami Hiramatsu]

On Fri, 21 Jul 2023 00:00:32 +0100
Alan Maguire <alan.maguire@oracle.com> wrote:

> 
> On 17/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Add test cases for accessing the data structure fields using BTF info.
> > This includes the field access from parameters and retval, and accessing
> > string information.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> One suggestion below, but
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks

> 
> > ---
> > Changes in v2:
> >  - Use '$retval' instead of 'retval'.
> >  - Add a test that use both '$retval' and '$arg1' for fprobe.
> > ---
> >  .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +++++++++++
> >  .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    4 ++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
> > index b89de1771655..93b94468967b 100644
> > --- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_btfarg.tc
> > @@ -21,6 +21,8 @@ echo 0 > events/enable
> >  echo > dynamic_events
> >  
> >  TP=kfree
> > +TP2=kmem_cache_alloc
> > +TP3=getname_flags
> >  
> >  if [ "$FPROBES" ] ; then
> >  echo "f:fpevent $TP object" >> dynamic_events
> > @@ -33,6 +35,7 @@ echo > dynamic_events
> >  
> >  echo "f:fpevent $TP "'$arg1' >> dynamic_events
> >  grep -q "fpevent.*object=object" dynamic_events
> > +
> >  echo > dynamic_events
> >  
> >  echo "f:fpevent $TP "'$arg*' >> dynamic_events
> > @@ -45,6 +48,14 @@ fi
> >  
> >  echo > dynamic_events
> >  
> > +echo "t:tpevent $TP2 name=s->name:string" >> dynamic_events
> > +echo "f:fpevent ${TP3}%return path=\$retval->name:string" >> dynamic_events
> > +
> 
> could we test a numeric value like kmem_cache_alloc object size?
> also if combos of -> and . are allowed, would be good to test one of
> those too.

OK, that's a good point! I'll add it.

> 
> > +grep -q "tpevent.*name=s->name:string" dynamic_events
> > +grep -q "fpevent.*path=\$retval->name:string" dynamic_events
> > +
> > +echo > dynamic_events
> > +
> >  if [ "$KPROBES" ] ; then
> >  echo "p:kpevent $TP object" >> dynamic_events
> >  grep -q "kpevent.*object=object" dynamic_events
> > diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
> > index 72563b2e0812..49758f77c923 100644
> > --- a/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/dynevent/fprobe_syntax_errors.tc
> > @@ -103,6 +103,10 @@ check_error 'f vfs_read%return ^$arg*'		# NOFENTRY_ARGS
> >  check_error 'f vfs_read ^hoge'			# NO_BTFARG
> >  check_error 'f kfree ^$arg10'			# NO_BTFARG (exceed the number of parameters)
> >  check_error 'f kfree%return ^$retval'		# NO_RETVAL
> > +check_error 'f vfs_read%return $retval->^foo'	# NO_PTR_STRCT
> > +check_error 'f vfs_read file->^foo'		# NO_BTF_FIELD
> > +check_error 'f vfs_read file^-.foo'		# BAD_HYPHEN
> > +check_error 'f vfs_read ^file:string'		# BAD_TYPE4STR
> >  else
> >  check_error 'f vfs_read ^$arg*'			# NOSUP_BTFARG
> >  check_error 't kfree ^$arg*'			# NOSUP_BTFARG
> > 
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[PATCH v2 9/9] Documentation: tracing: Update fprobe event example with BTF field

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Update fprobe event example with BTF data structure field specification.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Remove 'retval' and use '$retval'.
---
 Documentation/trace/fprobetrace.rst |   50 ++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/Documentation/trace/fprobetrace.rst b/Documentation/trace/fprobetrace.rst
index 7297f9478459..e9e764fadf14 100644
--- a/Documentation/trace/fprobetrace.rst
+++ b/Documentation/trace/fprobetrace.rst
@@ -79,9 +79,9 @@ automatically set by the given name. ::
  f:fprobes/myprobe vfs_read count=count pos=pos
 
 It also chooses the fetch type from BTF information. For example, in the above
-example, the ``count`` is unsigned long, and the ``pos`` is a pointer. Thus, both
-are converted to 64bit unsigned long, but only ``pos`` has "%Lx" print-format as
-below ::
+example, the ``count`` is unsigned long, and the ``pos`` is a pointer. Thus,
+both are converted to 64bit unsigned long, but only ``pos`` has "%Lx"
+print-format as below ::
 
  # cat events/fprobes/myprobe/format
  name: myprobe
@@ -105,9 +105,33 @@ is expanded to all function arguments of the function or the tracepoint. ::
  # cat dynamic_events
  f:fprobes/myprobe vfs_read file=file buf=buf count=count pos=pos
 
-BTF also affects the ``$retval``. If user doesn't set any type, the retval type is
-automatically picked from the BTF. If the function returns ``void``, ``$retval``
-is rejected.
+BTF also affects the ``$retval``. If user doesn't set any type, the retval
+type is automatically picked from the BTF. If the function returns ``void``,
+``$retval`` is rejected.
+
+You can access the data fields of a data structure using allow operator ``->``
+(for pointer type) and dot operator ``.`` (for data structure type.)::
+
+# echo 't sched_switch preempt prev_pid=prev->pid next_pid=next->pid' >> dynamic_events
+
+This data field access is available for the return value via ``$retval``,
+e.g. ``$retval->name``.
+
+For these BTF arguments and fields, ``:string`` and ``:ustring`` change the
+behavior. If these are used for BTF argument or field, it checks whether
+the BTF type of the argument or the data field is ``char *`` or ``char []``,
+or not.  If not, it rejects applying the string types. Also, with the BTF
+support, you don't need a memory dereference operator (``+0(PTR)``) for
+accessing the string pointed by a ``PTR``. It automatically adds the memory
+dereference operator according to the BTF type. e.g. ::
+
+# echo 't sched_switch prev->comm:string' >> dynamic_events
+# echo 'f getname_flags%return $retval->name:string' >> dynamic_events
+
+The ``prev->comm`` is an embedded char array in the data structure, and
+``$retval->name`` is a char pointer in the data structure. But in both
+cases, you can use ``:string`` type to get the string.
+
 
 Usage examples
 --------------
@@ -161,10 +185,10 @@ parameters. This means you can access any field values in the task
 structure pointed by the ``prev`` and ``next`` arguments.
 
 For example, usually ``task_struct::start_time`` is not traced, but with this
-traceprobe event, you can trace it as below.
+traceprobe event, you can trace that field as below.
 ::
 
-  # echo 't sched_switch comm=+1896(next):string start_time=+1728(next):u64' > dynamic_events
+  # echo 't sched_switch comm=next->comm:string next->start_time' > dynamic_events
   # head -n 20 trace | tail
  #           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
  #              | |         |   |||||     |         |
@@ -176,13 +200,3 @@ traceprobe event, you can trace it as below.
            <idle>-0       [000] d..3.  5606.690317: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="kworker/0:1" usage=1 start_time=137000000
       kworker/0:1-14      [000] d..3.  5606.690339: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="swapper/0" usage=2 start_time=0
            <idle>-0       [000] d..3.  5606.692368: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="kworker/0:1" usage=1 start_time=137000000
-
-Currently, to find the offset of a specific field in the data structure,
-you need to build kernel with debuginfo and run `perf probe` command with
-`-D` option. e.g.
-::
-
- # perf probe -D "__probestub_sched_switch next->comm:string next->start_time"
- p:probe/__probestub_sched_switch __probestub_sched_switch+0 comm=+1896(%cx):string start_time=+1728(%cx):u64
-
-And replace the ``%cx`` with the ``next``.

Re: [PATCH v2 9/9] Documentation: tracing: Update fprobe event example with BTF field

[Alan Maguire]

On 17/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Update fprobe event example with BTF data structure field specification.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

One suggestion below, but

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  Changes in v2:
>   - Remove 'retval' and use '$retval'.
> ---
>  Documentation/trace/fprobetrace.rst |   50 ++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/trace/fprobetrace.rst b/Documentation/trace/fprobetrace.rst
> index 7297f9478459..e9e764fadf14 100644
> --- a/Documentation/trace/fprobetrace.rst
> +++ b/Documentation/trace/fprobetrace.rst
> @@ -79,9 +79,9 @@ automatically set by the given name. ::
>   f:fprobes/myprobe vfs_read count=count pos=pos
>  
>  It also chooses the fetch type from BTF information. For example, in the above
> -example, the ``count`` is unsigned long, and the ``pos`` is a pointer. Thus, both
> -are converted to 64bit unsigned long, but only ``pos`` has "%Lx" print-format as
> -below ::
> +example, the ``count`` is unsigned long, and the ``pos`` is a pointer. Thus,
> +both are converted to 64bit unsigned long, but only ``pos`` has "%Lx"
> +print-format as below ::
>  
>   # cat events/fprobes/myprobe/format
>   name: myprobe
> @@ -105,9 +105,33 @@ is expanded to all function arguments of the function or the tracepoint. ::
>   # cat dynamic_events
>   f:fprobes/myprobe vfs_read file=file buf=buf count=count pos=pos
>  
> -BTF also affects the ``$retval``. If user doesn't set any type, the retval type is
> -automatically picked from the BTF. If the function returns ``void``, ``$retval``
> -is rejected.
> +BTF also affects the ``$retval``. If user doesn't set any type, the retval
> +type is automatically picked from the BTF. If the function returns ``void``,
> +``$retval`` is rejected.
> +
> +You can access the data fields of a data structure using allow operator ``->``
> +(for pointer type) and dot operator ``.`` (for data structure type.)::
> +
> +# echo 't sched_switch preempt prev_pid=prev->pid next_pid=next->pid' >> dynamic_events
> +

Could you describe what field access combinations are supported here;
i.e. foo->bar[.baz]?

> +This data field access is available for the return value via ``$retval``,
> +e.g. ``$retval->name``.
> +
> +For these BTF arguments and fields, ``:string`` and ``:ustring`` change the
> +behavior. If these are used for BTF argument or field, it checks whether
> +the BTF type of the argument or the data field is ``char *`` or ``char []``,
> +or not.  If not, it rejects applying the string types. Also, with the BTF
> +support, you don't need a memory dereference operator (``+0(PTR)``) for
> +accessing the string pointed by a ``PTR``. It automatically adds the memory
> +dereference operator according to the BTF type. e.g. ::
> +
> +# echo 't sched_switch prev->comm:string' >> dynamic_events
> +# echo 'f getname_flags%return $retval->name:string' >> dynamic_events
> +
> +The ``prev->comm`` is an embedded char array in the data structure, and
> +``$retval->name`` is a char pointer in the data structure. But in both
> +cases, you can use ``:string`` type to get the string.
> +
>  
>  Usage examples
>  --------------
> @@ -161,10 +185,10 @@ parameters. This means you can access any field values in the task
>  structure pointed by the ``prev`` and ``next`` arguments.
>  
>  For example, usually ``task_struct::start_time`` is not traced, but with this
> -traceprobe event, you can trace it as below.
> +traceprobe event, you can trace that field as below.
>  ::
>  
> -  # echo 't sched_switch comm=+1896(next):string start_time=+1728(next):u64' > dynamic_events
> +  # echo 't sched_switch comm=next->comm:string next->start_time' > dynamic_events
>    # head -n 20 trace | tail
>   #           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
>   #              | |         |   |||||     |         |
> @@ -176,13 +200,3 @@ traceprobe event, you can trace it as below.
>             <idle>-0       [000] d..3.  5606.690317: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="kworker/0:1" usage=1 start_time=137000000
>        kworker/0:1-14      [000] d..3.  5606.690339: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="swapper/0" usage=2 start_time=0
>             <idle>-0       [000] d..3.  5606.692368: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="kworker/0:1" usage=1 start_time=137000000
> -
> -Currently, to find the offset of a specific field in the data structure,
> -you need to build kernel with debuginfo and run `perf probe` command with
> -`-D` option. e.g.
> -::
> -
> - # perf probe -D "__probestub_sched_switch next->comm:string next->start_time"
> - p:probe/__probestub_sched_switch __probestub_sched_switch+0 comm=+1896(%cx):string start_time=+1728(%cx):u64
> -
> -And replace the ``%cx`` with the ``next``.
> 
> 

Re: [PATCH v2 9/9] Documentation: tracing: Update fprobe event example with BTF field

[Masami Hiramatsu]

On Thu, 20 Jul 2023 23:53:43 +0100
Alan Maguire <alan.maguire@oracle.com> wrote:

> On 17/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Update fprobe event example with BTF data structure field specification.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> One suggestion below, but
> 
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

Thanks,

> 
> > ---
> >  Changes in v2:
> >   - Remove 'retval' and use '$retval'.
> > ---
> >  Documentation/trace/fprobetrace.rst |   50 ++++++++++++++++++++++-------------
> >  1 file changed, 32 insertions(+), 18 deletions(-)
> > 
> > diff --git a/Documentation/trace/fprobetrace.rst b/Documentation/trace/fprobetrace.rst
> > index 7297f9478459..e9e764fadf14 100644
> > --- a/Documentation/trace/fprobetrace.rst
> > +++ b/Documentation/trace/fprobetrace.rst
> > @@ -79,9 +79,9 @@ automatically set by the given name. ::
> >   f:fprobes/myprobe vfs_read count=count pos=pos
> >  
> >  It also chooses the fetch type from BTF information. For example, in the above
> > -example, the ``count`` is unsigned long, and the ``pos`` is a pointer. Thus, both
> > -are converted to 64bit unsigned long, but only ``pos`` has "%Lx" print-format as
> > -below ::
> > +example, the ``count`` is unsigned long, and the ``pos`` is a pointer. Thus,
> > +both are converted to 64bit unsigned long, but only ``pos`` has "%Lx"
> > +print-format as below ::
> >  
> >   # cat events/fprobes/myprobe/format
> >   name: myprobe
> > @@ -105,9 +105,33 @@ is expanded to all function arguments of the function or the tracepoint. ::
> >   # cat dynamic_events
> >   f:fprobes/myprobe vfs_read file=file buf=buf count=count pos=pos
> >  
> > -BTF also affects the ``$retval``. If user doesn't set any type, the retval type is
> > -automatically picked from the BTF. If the function returns ``void``, ``$retval``
> > -is rejected.
> > +BTF also affects the ``$retval``. If user doesn't set any type, the retval
> > +type is automatically picked from the BTF. If the function returns ``void``,
> > +``$retval`` is rejected.
> > +
> > +You can access the data fields of a data structure using allow operator ``->``
> > +(for pointer type) and dot operator ``.`` (for data structure type.)::
> > +
> > +# echo 't sched_switch preempt prev_pid=prev->pid next_pid=next->pid' >> dynamic_events
> > +
> 
> Could you describe what field access combinations are supported here;
> i.e. foo->bar[.baz]?

OK, I'll add below.

The field access operators, ``->`` and ``.`` can be combined for accessing deeper
members and other stucture members pointed by the member. e.g. ``foo->bar.baz->qux``
If there is non-name union member, you can directly access it as C does. For example::

 struct {
 	union {
 	int a;
 	int b;
 	};
 } *foo;

To access ``a`` and ``b``, use ``foo->a`` and ``foo->b`` in this case.

Thank you,


> 
> > +This data field access is available for the return value via ``$retval``,
> > +e.g. ``$retval->name``.
> > +
> > +For these BTF arguments and fields, ``:string`` and ``:ustring`` change the
> > +behavior. If these are used for BTF argument or field, it checks whether
> > +the BTF type of the argument or the data field is ``char *`` or ``char []``,
> > +or not.  If not, it rejects applying the string types. Also, with the BTF
> > +support, you don't need a memory dereference operator (``+0(PTR)``) for
> > +accessing the string pointed by a ``PTR``. It automatically adds the memory
> > +dereference operator according to the BTF type. e.g. ::
> > +
> > +# echo 't sched_switch prev->comm:string' >> dynamic_events
> > +# echo 'f getname_flags%return $retval->name:string' >> dynamic_events
> > +
> > +The ``prev->comm`` is an embedded char array in the data structure, and
> > +``$retval->name`` is a char pointer in the data structure. But in both
> > +cases, you can use ``:string`` type to get the string.
> > +
> >  
> >  Usage examples
> >  --------------
> > @@ -161,10 +185,10 @@ parameters. This means you can access any field values in the task
> >  structure pointed by the ``prev`` and ``next`` arguments.
> >  
> >  For example, usually ``task_struct::start_time`` is not traced, but with this
> > -traceprobe event, you can trace it as below.
> > +traceprobe event, you can trace that field as below.
> >  ::
> >  
> > -  # echo 't sched_switch comm=+1896(next):string start_time=+1728(next):u64' > dynamic_events
> > +  # echo 't sched_switch comm=next->comm:string next->start_time' > dynamic_events
> >    # head -n 20 trace | tail
> >   #           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
> >   #              | |         |   |||||     |         |
> > @@ -176,13 +200,3 @@ traceprobe event, you can trace it as below.
> >             <idle>-0       [000] d..3.  5606.690317: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="kworker/0:1" usage=1 start_time=137000000
> >        kworker/0:1-14      [000] d..3.  5606.690339: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="swapper/0" usage=2 start_time=0
> >             <idle>-0       [000] d..3.  5606.692368: sched_switch: (__probestub_sched_switch+0x4/0x10) comm="kworker/0:1" usage=1 start_time=137000000
> > -
> > -Currently, to find the offset of a specific field in the data structure,
> > -you need to build kernel with debuginfo and run `perf probe` command with
> > -`-D` option. e.g.
> > -::
> > -
> > - # perf probe -D "__probestub_sched_switch next->comm:string next->start_time"
> > - p:probe/__probestub_sched_switch __probestub_sched_switch+0 comm=+1896(%cx):string start_time=+1728(%cx):u64
> > -
> > -And replace the ``%cx`` with the ``next``.
> > 
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 0/9] tracing: Improbe BTF support on probe events

[Alan Maguire]

On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> Hi,
> 
> Here is the 2nd version of series to improve the BTF support on probe events.
> The previous series is here:
> 
> https://lore.kernel.org/linux-trace-kernel/168699521817.528797.13179901018528120324.stgit@mhiramat.roam.corp.google.com/
> 
> In this version, I added a NULL check fix patch [1/9] (which will go to
> fixes branch) and move BTF related API to kernel/bpf/btf.c [2/9] and add
> a new BTF API [3/9] so that anyone can reuse it.
> Also I decided to use '$retval' directly instead of 'retval' pseudo BTF
> variable for field access at [5/9] because I introduced an idea to choose
> function 'exit' event automatically if '$retval' is used [7/9]. With that
> change, we can not use 'retval' because if a function has 'retval'
> argument, we can not decide 'f func retval' is function exit or entry.

this is fantastic work! (FWIW I ran into the retval argument issue with
ksnoop as well; I got around it by using "return" to signify the return
value since as a reserved word it won't clash with a variable name.
However in the trace subsystem context retval is used extensively so
makes sense to stick with that).

One thing we should probably figure out is a common approach to handling
ambiguous static functions that will work across ftrace and BPF.  A few
edge cases that are worth figuring out:

1. a static function with the same name exists in multiple modules,
either with different or identical function signatures
2. a static function has .isra.0 and other gcc suffixes applied to
static functions during optimization

As Alexei mentioned, we're still working on 1, so it would be good
to figure out a naming scheme that works well in both ftrace and BPF
contexts. There are a few hundred of these ambiguous functions. My
reading of the fprobe docs seems to suggest that there is no mechanism
to specify a specific module for a given symbol (as in ftrace filters),
is that right?

Jiri led a session on this topic at LSF/MM/BPF ; perhaps we should
carve out some time at Plumbers to discuss this?

With respect to 2, pahole v1.25 will generate representations for these
"."-suffixed functions in BTF via --btf_gen_optimized [1]. (BTF
representation is skipped if the optimizations impact on the registers
used for function arguments; if these don't match calling conventions
due to optimized-out params, we don't represent the function in BTF,
as the tracing expectations are violated).

However the BTF function name - in line with DWARF representation -
will not have the .isra suffix. So the thing to bear in mind is if
you use the function name with suffix as the fprobe function name,
a BTF lookup of that exact ("foo.isra.0") name will not find anything,
while a lookup of "foo" will succeed. I'll add some specifics in your
patch doing the lookups, but just wanted to highlight the issue at
the top-level.

Thanks!

Alan

[1]
https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/

> Selftest test case [8/9] and document [9/9] are also updated according to
> those changes.
> 
> This series can be applied on top of "v6.5-rc2" kernel.
> 
> You can also get this series from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git topic/fprobe-event-ext
> 
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (Google) (9):
>       tracing/probes: Fix to add NULL check for BTF APIs
>       bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF
>       bpf/btf: Add a function to search a member of a struct/union
>       tracing/probes: Support BTF based data structure field access
>       tracing/probes: Support BTF field access from $retval
>       tracing/probes: Add string type check with BTF
>       tracing/fprobe-event: Assume fprobe is a return event by $retval
>       selftests/ftrace: Add BTF fields access testcases
>       Documentation: tracing: Update fprobe event example with BTF field
> 
> 
>  Documentation/trace/fprobetrace.rst                |   50 ++
>  include/linux/btf.h                                |    7 
>  kernel/bpf/btf.c                                   |   83 ++++
>  kernel/trace/trace_fprobe.c                        |   58 ++-
>  kernel/trace/trace_probe.c                         |  402 +++++++++++++++-----
>  kernel/trace/trace_probe.h                         |   12 +
>  .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +
>  .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    6 
>  8 files changed, 503 insertions(+), 126 deletions(-)
> 
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 

Re: [PATCH v2 0/9] tracing: Improbe BTF support on probe events

[Masami Hiramatsu]

On Wed, 19 Jul 2023 10:02:06 +0100
Alan Maguire <alan.maguire@oracle.com> wrote:

> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > Hi,
> > 
> > Here is the 2nd version of series to improve the BTF support on probe events.
> > The previous series is here:
> > 
> > https://lore.kernel.org/linux-trace-kernel/168699521817.528797.13179901018528120324.stgit@mhiramat.roam.corp.google.com/
> > 
> > In this version, I added a NULL check fix patch [1/9] (which will go to
> > fixes branch) and move BTF related API to kernel/bpf/btf.c [2/9] and add
> > a new BTF API [3/9] so that anyone can reuse it.
> > Also I decided to use '$retval' directly instead of 'retval' pseudo BTF
> > variable for field access at [5/9] because I introduced an idea to choose
> > function 'exit' event automatically if '$retval' is used [7/9]. With that
> > change, we can not use 'retval' because if a function has 'retval'
> > argument, we can not decide 'f func retval' is function exit or entry.
> 
> this is fantastic work! (FWIW I ran into the retval argument issue with
> ksnoop as well; I got around it by using "return" to signify the return
> value since as a reserved word it won't clash with a variable name.
> However in the trace subsystem context retval is used extensively so
> makes sense to stick with that).

Thanks!

> 
> One thing we should probably figure out is a common approach to handling
> ambiguous static functions that will work across ftrace and BPF.  A few
> edge cases that are worth figuring out:
> 
> 1. a static function with the same name exists in multiple modules,
> either with different or identical function signatures
> 2. a static function has .isra.0 and other gcc suffixes applied to
> static functions during optimization
> 
> As Alexei mentioned, we're still working on 1, so it would be good
> to figure out a naming scheme that works well in both ftrace and BPF
> contexts. There are a few hundred of these ambiguous functions. My
> reading of the fprobe docs seems to suggest that there is no mechanism
> to specify a specific module for a given symbol (as in ftrace filters),
> is that right?

Yes, it doesn't have module specificaiton at this moment. I'll considering
to fix this. BTW, for the same-name functions, we are discussing another
approach. We also need to sync this with BTF. 

https://lore.kernel.org/all/20230714150326.1152359-1-alessandro.carminati@gmail.com/

> 
> Jiri led a session on this topic at LSF/MM/BPF ; perhaps we should
> carve out some time at Plumbers to discuss this?

Yeah, good idea.

> 
> With respect to 2, pahole v1.25 will generate representations for these
> "."-suffixed functions in BTF via --btf_gen_optimized [1]. (BTF
> representation is skipped if the optimizations impact on the registers
> used for function arguments; if these don't match calling conventions
> due to optimized-out params, we don't represent the function in BTF,
> as the tracing expectations are violated).

Correct. But can't we know which argument is skipped by the optimization
from the DWARF? At least the function parameters will be changed.

> However the BTF function name - in line with DWARF representation -
> will not have the .isra suffix. So the thing to bear in mind is if
> you use the function name with suffix as the fprobe function name,
> a BTF lookup of that exact ("foo.isra.0") name will not find anything,
> while a lookup of "foo" will succeed. I'll add some specifics in your
> patch doing the lookups, but just wanted to highlight the issue at
> the top-level.

So, what about adding an index sorted list of the address and BTF entry
index as an expansion of the BTF? It allowed us to easily map the suffixed
symbol address (we can get it from kallsyms) to BTF quickly.
So the module will have

[BTF data][array length][BTF index array]

Index array member will be like this.

struct btf_index {
	u32	offset;	// offset from the start text
	u32	id:		// BTF type id
};

We can do binary search the function type id from the symbol address.

Thank you,

> 
> Thanks!
> 
> Alan
> 
> [1]
> https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> 
> > Selftest test case [8/9] and document [9/9] are also updated according to
> > those changes.
> > 
> > This series can be applied on top of "v6.5-rc2" kernel.
> > 
> > You can also get this series from:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git topic/fprobe-event-ext
> > 
> > 
> > Thank you,
> > 
> > ---
> > 
> > Masami Hiramatsu (Google) (9):
> >       tracing/probes: Fix to add NULL check for BTF APIs
> >       bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF
> >       bpf/btf: Add a function to search a member of a struct/union
> >       tracing/probes: Support BTF based data structure field access
> >       tracing/probes: Support BTF field access from $retval
> >       tracing/probes: Add string type check with BTF
> >       tracing/fprobe-event: Assume fprobe is a return event by $retval
> >       selftests/ftrace: Add BTF fields access testcases
> >       Documentation: tracing: Update fprobe event example with BTF field
> > 
> > 
> >  Documentation/trace/fprobetrace.rst                |   50 ++
> >  include/linux/btf.h                                |    7 
> >  kernel/bpf/btf.c                                   |   83 ++++
> >  kernel/trace/trace_fprobe.c                        |   58 ++-
> >  kernel/trace/trace_probe.c                         |  402 +++++++++++++++-----
> >  kernel/trace/trace_probe.h                         |   12 +
> >  .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +
> >  .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    6 
> >  8 files changed, 503 insertions(+), 126 deletions(-)
> > 
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 0/9] tracing: Improbe BTF support on probe events

[Alan Maguire]

On 19/07/2023 17:01, Masami Hiramatsu (Google) wrote:
> On Wed, 19 Jul 2023 10:02:06 +0100
> Alan Maguire <alan.maguire@oracle.com> wrote:
> 
>> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
>>> Hi,
>>>
>>> Here is the 2nd version of series to improve the BTF support on probe events.
>>> The previous series is here:
>>>
>>> https://lore.kernel.org/linux-trace-kernel/168699521817.528797.13179901018528120324.stgit@mhiramat.roam.corp.google.com/
>>>
>>> In this version, I added a NULL check fix patch [1/9] (which will go to
>>> fixes branch) and move BTF related API to kernel/bpf/btf.c [2/9] and add
>>> a new BTF API [3/9] so that anyone can reuse it.
>>> Also I decided to use '$retval' directly instead of 'retval' pseudo BTF
>>> variable for field access at [5/9] because I introduced an idea to choose
>>> function 'exit' event automatically if '$retval' is used [7/9]. With that
>>> change, we can not use 'retval' because if a function has 'retval'
>>> argument, we can not decide 'f func retval' is function exit or entry.
>>
>> this is fantastic work! (FWIW I ran into the retval argument issue with
>> ksnoop as well; I got around it by using "return" to signify the return
>> value since as a reserved word it won't clash with a variable name.
>> However in the trace subsystem context retval is used extensively so
>> makes sense to stick with that).
> 
> Thanks!
> 
>>
>> One thing we should probably figure out is a common approach to handling
>> ambiguous static functions that will work across ftrace and BPF.  A few
>> edge cases that are worth figuring out:
>>
>> 1. a static function with the same name exists in multiple modules,
>> either with different or identical function signatures
>> 2. a static function has .isra.0 and other gcc suffixes applied to
>> static functions during optimization
>>
>> As Alexei mentioned, we're still working on 1, so it would be good
>> to figure out a naming scheme that works well in both ftrace and BPF
>> contexts. There are a few hundred of these ambiguous functions. My
>> reading of the fprobe docs seems to suggest that there is no mechanism
>> to specify a specific module for a given symbol (as in ftrace filters),
>> is that right?
> 
> Yes, it doesn't have module specificaiton at this moment. I'll considering
> to fix this. BTW, for the same-name functions, we are discussing another
> approach. We also need to sync this with BTF. 
> 
> https://lore.kernel.org/all/20230714150326.1152359-1-alessandro.carminati@gmail.com/
> 
>>
>> Jiri led a session on this topic at LSF/MM/BPF ; perhaps we should
>> carve out some time at Plumbers to discuss this?
> 
> Yeah, good idea.
> 
>>
>> With respect to 2, pahole v1.25 will generate representations for these
>> "."-suffixed functions in BTF via --btf_gen_optimized [1]. (BTF
>> representation is skipped if the optimizations impact on the registers
>> used for function arguments; if these don't match calling conventions
>> due to optimized-out params, we don't represent the function in BTF,
>> as the tracing expectations are violated).
> 
> Correct. But can't we know which argument is skipped by the optimization
> from the DWARF? At least the function parameters will be changed.
>

Yep; we use the expected registers to spot cases where something
has been optimized out.

>> However the BTF function name - in line with DWARF representation -
>> will not have the .isra suffix. So the thing to bear in mind is if
>> you use the function name with suffix as the fprobe function name,
>> a BTF lookup of that exact ("foo.isra.0") name will not find anything,
>> while a lookup of "foo" will succeed. I'll add some specifics in your
>> patch doing the lookups, but just wanted to highlight the issue at
>> the top-level.
> 
> So, what about adding an index sorted list of the address and BTF entry
> index as an expansion of the BTF? It allowed us to easily map the suffixed
> symbol address (we can get it from kallsyms) to BTF quickly.
> So the module will have
> 
> [BTF data][array length][BTF index array]
> 
> Index array member will be like this.
> 
> struct btf_index {
> 	u32	offset;	// offset from the start text
> 	u32	id:		// BTF type id
> };
> 
> We can do binary search the function type id from the symbol address.
> 

Yeah, I wonder if a representation that bridged between kallsyms and BTF
might be valuable? I don't _think_ it's as much of an issue for your
case though since you only need to do the BTF lookup once on fprobe
setup, right? Thanks!

Alan



> Thank you,
> 
>>
>> Thanks!
>>
>> Alan
>>
>> [1]
>> https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
>>
>>> Selftest test case [8/9] and document [9/9] are also updated according to
>>> those changes.
>>>
>>> This series can be applied on top of "v6.5-rc2" kernel.
>>>
>>> You can also get this series from:
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git topic/fprobe-event-ext
>>>
>>>
>>> Thank you,
>>>
>>> ---
>>>
>>> Masami Hiramatsu (Google) (9):
>>>       tracing/probes: Fix to add NULL check for BTF APIs
>>>       bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF
>>>       bpf/btf: Add a function to search a member of a struct/union
>>>       tracing/probes: Support BTF based data structure field access
>>>       tracing/probes: Support BTF field access from $retval
>>>       tracing/probes: Add string type check with BTF
>>>       tracing/fprobe-event: Assume fprobe is a return event by $retval
>>>       selftests/ftrace: Add BTF fields access testcases
>>>       Documentation: tracing: Update fprobe event example with BTF field
>>>
>>>
>>>  Documentation/trace/fprobetrace.rst                |   50 ++
>>>  include/linux/btf.h                                |    7 
>>>  kernel/bpf/btf.c                                   |   83 ++++
>>>  kernel/trace/trace_fprobe.c                        |   58 ++-
>>>  kernel/trace/trace_probe.c                         |  402 +++++++++++++++-----
>>>  kernel/trace/trace_probe.h                         |   12 +
>>>  .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +
>>>  .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    6 
>>>  8 files changed, 503 insertions(+), 126 deletions(-)
>>>
>>> --
>>> Masami Hiramatsu (Google) <mhiramat@kernel.org>
>>>
> 
> 

Re: [PATCH v2 0/9] tracing: Improbe BTF support on probe events

[Masami Hiramatsu]

On Thu, 20 Jul 2023 22:50:18 +0100
Alan Maguire <alan.maguire@oracle.com> wrote:


> >> One thing we should probably figure out is a common approach to handling
> >> ambiguous static functions that will work across ftrace and BPF.  A few
> >> edge cases that are worth figuring out:
> >>
> >> 1. a static function with the same name exists in multiple modules,
> >> either with different or identical function signatures
> >> 2. a static function has .isra.0 and other gcc suffixes applied to
> >> static functions during optimization
> >>
> >> As Alexei mentioned, we're still working on 1, so it would be good
> >> to figure out a naming scheme that works well in both ftrace and BPF
> >> contexts. There are a few hundred of these ambiguous functions. My
> >> reading of the fprobe docs seems to suggest that there is no mechanism
> >> to specify a specific module for a given symbol (as in ftrace filters),
> >> is that right?
> > 
> > Yes, it doesn't have module specificaiton at this moment. I'll considering
> > to fix this. BTW, for the same-name functions, we are discussing another
> > approach. We also need to sync this with BTF. 
> > 
> > https://lore.kernel.org/all/20230714150326.1152359-1-alessandro.carminati@gmail.com/
> > 
> >>
> >> Jiri led a session on this topic at LSF/MM/BPF ; perhaps we should
> >> carve out some time at Plumbers to discuss this?
> > 
> > Yeah, good idea.
> > 
> >>
> >> With respect to 2, pahole v1.25 will generate representations for these
> >> "."-suffixed functions in BTF via --btf_gen_optimized [1]. (BTF
> >> representation is skipped if the optimizations impact on the registers
> >> used for function arguments; if these don't match calling conventions
> >> due to optimized-out params, we don't represent the function in BTF,
> >> as the tracing expectations are violated).
> > 
> > Correct. But can't we know which argument is skipped by the optimization
> > from the DWARF? At least the function parameters will be changed.
> >
> 
> Yep; we use the expected registers to spot cases where something
> has been optimized out.

I guess DWARF knows which register is optimized out and then BTF also
knows that?
Let me update my pahole too.

> >> However the BTF function name - in line with DWARF representation -
> >> will not have the .isra suffix. So the thing to bear in mind is if
> >> you use the function name with suffix as the fprobe function name,
> >> a BTF lookup of that exact ("foo.isra.0") name will not find anything,
> >> while a lookup of "foo" will succeed. I'll add some specifics in your
> >> patch doing the lookups, but just wanted to highlight the issue at
> >> the top-level.
> > 
> > So, what about adding an index sorted list of the address and BTF entry
> > index as an expansion of the BTF? It allowed us to easily map the suffixed
> > symbol address (we can get it from kallsyms) to BTF quickly.
> > So the module will have
> > 
> > [BTF data][array length][BTF index array]
> > 
> > Index array member will be like this.
> > 
> > struct btf_index {
> > 	u32	offset;	// offset from the start text
> > 	u32	id:		// BTF type id
> > };
> > 
> > We can do binary search the function type id from the symbol address.
> > 
> 
> Yeah, I wonder if a representation that bridged between kallsyms and BTF
> might be valuable? I don't _think_ it's as much of an issue for your
> case though since you only need to do the BTF lookup once on fprobe
> setup, right? Thanks!

Yes, but I'm thinking fprobe to support some sort of 'symbol+offset' format
to specify one of the "same-name" symbols. In that case, it is important to
identify which address the BTF type is pointed.

Thank you!

> 
> Alan
> 
> 
> 
> > Thank you,
> > 
> >>
> >> Thanks!
> >>
> >> Alan
> >>
> >> [1]
> >> https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/
> >>
> >>> Selftest test case [8/9] and document [9/9] are also updated according to
> >>> those changes.
> >>>
> >>> This series can be applied on top of "v6.5-rc2" kernel.
> >>>
> >>> You can also get this series from:
> >>>
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git topic/fprobe-event-ext
> >>>
> >>>
> >>> Thank you,
> >>>
> >>> ---
> >>>
> >>> Masami Hiramatsu (Google) (9):
> >>>       tracing/probes: Fix to add NULL check for BTF APIs
> >>>       bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF
> >>>       bpf/btf: Add a function to search a member of a struct/union
> >>>       tracing/probes: Support BTF based data structure field access
> >>>       tracing/probes: Support BTF field access from $retval
> >>>       tracing/probes: Add string type check with BTF
> >>>       tracing/fprobe-event: Assume fprobe is a return event by $retval
> >>>       selftests/ftrace: Add BTF fields access testcases
> >>>       Documentation: tracing: Update fprobe event example with BTF field
> >>>
> >>>
> >>>  Documentation/trace/fprobetrace.rst                |   50 ++
> >>>  include/linux/btf.h                                |    7 
> >>>  kernel/bpf/btf.c                                   |   83 ++++
> >>>  kernel/trace/trace_fprobe.c                        |   58 ++-
> >>>  kernel/trace/trace_probe.c                         |  402 +++++++++++++++-----
> >>>  kernel/trace/trace_probe.h                         |   12 +
> >>>  .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +
> >>>  .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    6 
> >>>  8 files changed, 503 insertions(+), 126 deletions(-)
> >>>
> >>> --
> >>> Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >>>
> > 
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>