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) <[email protected]>
From: Masami Hiramatsu (Google) <[email protected]>
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) <[email protected]>
---
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;
From: Masami Hiramatsu (Google) <[email protected]>
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) <[email protected]>
---
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)
From: Masami Hiramatsu (Google) <[email protected]>
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) <[email protected]>
---
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
From: Masami Hiramatsu (Google) <[email protected]>
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) <[email protected]>
---
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 */
From: Masami Hiramatsu (Google) <[email protected]>
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) <[email protected]>
---
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
From: Masami Hiramatsu (Google) <[email protected]>
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) <[email protected]>
---
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
From: Masami Hiramatsu (Google) <[email protected]>
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) <[email protected]>
---
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
On Tue, 18 Jul 2023 00:23:37 +0900
"Masami Hiramatsu (Google)" <[email protected]> wrote:
> From: Masami Hiramatsu (Google) <[email protected]>
>
> 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) <[email protected]>
> ---
> 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;
On Mon, Jul 17, 2023 at 4:46 PM Masami Hiramatsu <[email protected]> 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.
On Mon, 17 Jul 2023 14:39:14 -0400
Steven Rostedt <[email protected]> wrote:
> On Tue, 18 Jul 2023 00:23:37 +0900
> "Masami Hiramatsu (Google)" <[email protected]> wrote:
>
> > From: Masami Hiramatsu (Google) <[email protected]>
> >
> > 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) <[email protected]>
> > ---
> > 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) <[email protected]>
On Tue, 18 Jul 2023 08:46:34 +0900
Masami Hiramatsu (Google) <[email protected]> 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);
> > > +
On Mon, 17 Jul 2023 19:51:29 -0400
Steven Rostedt <[email protected]> wrote:
> On Tue, 18 Jul 2023 08:46:34 +0900
> Masami Hiramatsu (Google) <[email protected]> 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) <[email protected]>
On Mon, 17 Jul 2023 16:51:29 -0700
Alexei Starovoitov <[email protected]> wrote:
> On Mon, Jul 17, 2023 at 4:46 PM Masami Hiramatsu <[email protected]> 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) <[email protected]>
On Mon, Jul 17, 2023 at 11:24 PM Masami Hiramatsu (Google)
<[email protected]> wrote:
>
> From: Masami Hiramatsu (Google) <[email protected]>
>
> 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) <[email protected]>
> ---
> 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;
>
>
On Tue, 18 Jul 2023 10:40:09 +0800
Donglin Peng <[email protected]> wrote:
> On Mon, Jul 17, 2023 at 11:24 PM Masami Hiramatsu (Google)
> <[email protected]> wrote:
> >
> > From: Masami Hiramatsu (Google) <[email protected]>
> >
> > 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) <[email protected]>
> > ---
> > 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) <[email protected]>
On Tue, 18 Jul 2023 19:44:31 +0900
Masami Hiramatsu (Google) <[email protected]> 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) <[email protected]>
On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[email protected]> wrote:
>
> On Tue, 18 Jul 2023 19:44:31 +0900
> Masami Hiramatsu (Google) <[email protected]> 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.
On Tue, Jul 18, 2023 at 4:03 PM Masami Hiramatsu <[email protected]> wrote:
>
> On Tue, 18 Jul 2023 10:11:01 -0700
> Alexei Starovoitov <[email protected]> wrote:
>
> > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[email protected]> wrote:
> > >
> > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > Masami Hiramatsu (Google) <[email protected]> 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.
On Tue, 18 Jul 2023 10:11:01 -0700
Alexei Starovoitov <[email protected]> wrote:
> On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[email protected]> wrote:
> >
> > On Tue, 18 Jul 2023 19:44:31 +0900
> > Masami Hiramatsu (Google) <[email protected]> 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) <[email protected]>
On Wed, Jul 19, 2023 at 7:03 AM Masami Hiramatsu <[email protected]> wrote:
>
> On Tue, 18 Jul 2023 10:11:01 -0700
> Alexei Starovoitov <[email protected]> wrote:
>
> > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[email protected]> wrote:
> > >
> > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > Masami Hiramatsu (Google) <[email protected]> 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) <[email protected]>
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/[email protected]/
> 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) <[email protected]>
>
On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <[email protected]>
>
> 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) <[email protected]>
> ---
> 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;
>
>
On Wed, 19 Jul 2023 10:09:48 +0800
Donglin Peng <[email protected]> wrote:
> On Wed, Jul 19, 2023 at 7:03 AM Masami Hiramatsu <[email protected]> wrote:
> >
> > On Tue, 18 Jul 2023 10:11:01 -0700
> > Alexei Starovoitov <[email protected]> wrote:
> >
> > > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[email protected]> wrote:
> > > >
> > > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > > Masami Hiramatsu (Google) <[email protected]> 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) <[email protected]>
--
Masami Hiramatsu (Google) <[email protected]>
On Tue, 18 Jul 2023 16:12:55 -0700
Alexei Starovoitov <[email protected]> wrote:
> On Tue, Jul 18, 2023 at 4:03 PM Masami Hiramatsu <[email protected]> wrote:
> >
> > On Tue, 18 Jul 2023 10:11:01 -0700
> > Alexei Starovoitov <[email protected]> wrote:
> >
> > > On Tue, Jul 18, 2023 at 6:56 AM Masami Hiramatsu <[email protected]> wrote:
> > > >
> > > > On Tue, 18 Jul 2023 19:44:31 +0900
> > > > Masami Hiramatsu (Google) <[email protected]> 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) <[email protected]>
On Wed, 19 Jul 2023 13:36:48 +0100
Alan Maguire <[email protected]> wrote:
> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <[email protected]>
> >
> > 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) <[email protected]>
> > ---
> > 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) <[email protected]>
On Wed, 19 Jul 2023 10:02:06 +0100
Alan Maguire <[email protected]> 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/[email protected]/
>
> 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/[email protected]/
>
> > 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) <[email protected]>
> >
--
Masami Hiramatsu (Google) <[email protected]>
On 19/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> On Wed, 19 Jul 2023 13:36:48 +0100
> Alan Maguire <[email protected]> wrote:
>
>> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
>>> From: Masami Hiramatsu (Google) <[email protected]>
>>>
>>> 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) <[email protected]>
>>> ---
>>> 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;
>>>
>>>
>
>
On 19/07/2023 17:01, Masami Hiramatsu (Google) wrote:
> On Wed, 19 Jul 2023 10:02:06 +0100
> Alan Maguire <[email protected]> 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/[email protected]/
>
>>
>> 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/[email protected]/
>>
>>> 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) <[email protected]>
>>>
>
>
On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <[email protected]>
>
> 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) <[email protected]>
One issue below that I'm not totally clear on, but
Reviewed-by: Alan Maguire <[email protected]>
> ---
> 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
>
>
On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <[email protected]>
>
> 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) <[email protected]>
A few small things below, but
Reviewed-by: Alan Maguire <[email protected]>
> ---
> 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)
>
>
On 17/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> From: Masami Hiramatsu (Google) <[email protected]>
>
> 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) <[email protected]>
One suggestion below, but
Reviewed-by: Alan Maguire <[email protected]>
> ---
> 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
>
>
On Fri, 21 Jul 2023 00:00:32 +0100
Alan Maguire <[email protected]> wrote:
>
> On 17/07/2023 16:24, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <[email protected]>
> >
> > 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) <[email protected]>
>
> One suggestion below, but
>
> Reviewed-by: Alan Maguire <[email protected]>
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) <[email protected]>
On Thu, 20 Jul 2023 23:34:42 +0100
Alan Maguire <[email protected]> wrote:
> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <[email protected]>
> >
> > 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) <[email protected]>
>
> A few small things below, but
>
> Reviewed-by: Alan Maguire <[email protected]>
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) <[email protected]>
On Thu, 20 Jul 2023 23:51:17 +0100
Alan Maguire <[email protected]> wrote:
> On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <[email protected]>
> >
> > 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) <[email protected]>
>
> One issue below that I'm not totally clear on, but
>
> Reviewed-by: Alan Maguire <[email protected]>
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) <[email protected]>
On Thu, 20 Jul 2023 22:50:18 +0100
Alan Maguire <[email protected]> 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/[email protected]/
> >
> >>
> >> 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/[email protected]/
> >>
> >>> 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) <[email protected]>
> >>>
> >
> >
--
Masami Hiramatsu (Google) <[email protected]>