On Mon, 31 Jul 2023 14:59:47 -0700
Alexei Starovoitov [off-list ref] wrote:

On Mon, Jul 31, 2023 at 12:30 AM Masami Hiramatsu (Google)
[off-list ref] wrote:

quoted

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

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

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Reviewed-by: Alan Maguire <redacted>
---
 Changes in v3:
  - Remove simple input check.
  - Fix unneeded IS_ERR_OR_NULL() check for btf_type_by_id().
  - Move the code next to btf_get_func_param().
  - Use for_each_member() macro instead of for-loop.
  - Use btf_type_skip_modifiers() instead of btf_type_by_id().
 Changes in v4:
  - Use a stack for searching in anonymous members instead of nested call.
---
 include/linux/btf.h |    3 +++
 kernel/bpf/btf.c    |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 20e3a07eef8f..4b10d57ceee0 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h

@@ -226,6 +226,9 @@ const struct btf_type *btf_find_func_proto(const char *func_name,
                                           struct btf **btf_p);
 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 f7b25c615269..8d81a4ffa67b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -958,6 +958,46 @@ const struct btf_param *btf_get_func_param(const struct btf_type *func_proto, s3
                return NULL;
 }

+#define BTF_ANON_STACK_MAX     16
+
+/*
+ * 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_type *anon_stack[BTF_ANON_STACK_MAX];
+       const struct btf_member *member;
+       const char *name;
+       int i, top = 0;
+
+retry:
+       if (!btf_type_is_struct(type))
+               return ERR_PTR(-EINVAL);
+
+       for_each_member(i, type, member) {
+               if (!member->name_off) {
+                       /* Anonymous union/struct: push it for later use */
+                       type = btf_type_skip_modifiers(btf, member->type, NULL);
+                       if (type && top < BTF_ANON_STACK_MAX)
+                               anon_stack[top++] = type;
+               } else {
+                       name = btf_name_by_offset(btf, member->name_off);
+                       if (name && !strcmp(member_name, name))
+                               return member;
+               }
+       }
+       if (top > 0) {
+               /* Pop from the anonymous stack and retry */
+               type = anon_stack[--top];
+               goto retry;
+       }

Looks good, but I don't see a test case for this.
The logic is a bit tricky. I'd like to have a selftest that covers it.

Thanks, and I agree about selftest.

You probably need to drop Alan's reviewed-by, since the patch is quite
different from the time he reviewed it.

OK. BTW, I found a problem on this function. I guess the member->offset will
be the offset from the intermediate anonymous union, it is usually 0, but
I need the offset from the given structure. Thus the interface design must
be changed. Passing a 'u32 *offset' and set the correct offset in it. If
it has nested intermediate anonymous unions, that offset must also be pushed.

Assuming that is addressed. How do we merge the series?
The first 3 patches have serious conflicts with bpf trees.

Maybe send the first 3 with extra selftest for above recursion
targeting bpf-next then we can have a merge commit that Steven can pull
into tracing?

Or if we can have acks for patches 4-9 we can pull the whole set into bpf-next.

That's a good question. I don't like splitting the whole series in 2 -next
branches. So I can send this to the bpf-next.
I need to work on another series(*) on fprobes which will not have conflicts with
this series. (*Replacing pt_regs with ftrace_regs on fprobe, which will take longer
time, and need to adjust with eBPF).

Thank you,

-- 
Masami Hiramatsu (Google) [off-list ref]