[RFC bpf-next 0/3] bpf: support module BTF in btf display helpers

[RFC bpf-next 0/3] bpf: support module BTF in btf display helpers

[Alan Maguire]

This series aims to add support to bpf_snprintf_btf() and 
bpf_seq_printf_btf() allowing them to store string representations
of module-specific types, as well as the kernel-specific ones
they currently support.

Patch 1 adds an additional field "const char *module" to
"struct btf_ptr", allowing the specification of a module
name along with a data pointer, BTF id, etc.  It is then 
used to look up module BTF, rather than the default
vmlinux BTF.

Patch 2 makes a small fix to libbpf to allow 
btf__type_by_name[_kind] to work with split BTF.  Without
this fix, type lookup of a module-specific type id will fail
in patch 3.

Patch 3 is a selftest that uses veth (when built as a
module) and a kprobe to display both a module-specific 
and kernel-specific type; both are arguments to veth_stats_rx().

Alan Maguire (3):
  bpf: add module support to btf display helpers
  libbpf: bpf__find_by_name[_kind] should use btf__get_nr_types()
  selftests/bpf: verify module-specific types can be shown via
    bpf_snprintf_btf

 include/linux/btf.h                                |  8 ++
 include/uapi/linux/bpf.h                           |  5 +-
 kernel/bpf/btf.c                                   | 18 ++++
 kernel/trace/bpf_trace.c                           | 42 +++++++---
 tools/include/uapi/linux/bpf.h                     |  5 +-
 tools/lib/bpf/btf.c                                |  4 +-
 .../selftests/bpf/prog_tests/snprintf_btf_mod.c    | 96 ++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/btf_ptr.h        |  1 +
 tools/testing/selftests/bpf/progs/veth_stats_rx.c  | 73 ++++++++++++++++
 9 files changed, 238 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf_btf_mod.c
 create mode 100644 tools/testing/selftests/bpf/progs/veth_stats_rx.c

-- 
1.8.3.1

[RFC bpf-next 1/3] bpf: add module support to btf display helpers

[Alan Maguire]

bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
argument that specifies type information about the type to
be displayed.  Augment this information to include a module
name, allowing such display to support module types.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 include/linux/btf.h            |  8 ++++++++
 include/uapi/linux/bpf.h       |  5 ++++-
 kernel/bpf/btf.c               | 18 ++++++++++++++++++
 kernel/trace/bpf_trace.c       | 42 ++++++++++++++++++++++++++++++++----------
 tools/include/uapi/linux/bpf.h |  5 ++++-
 5 files changed, 66 insertions(+), 12 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 2bf6418..d55ca00 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo *btf_type_var_secinfo(
 const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
 const char *btf_name_by_offset(const struct btf *btf, u32 offset);
 struct btf *btf_parse_vmlinux(void);
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+struct btf *bpf_get_btf_module(const char *name);
+#else
+static inline struct btf *bpf_get_btf_module(const char *name)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+#endif
 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
 #else
 static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 162999b..26978be 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
  *		the pointer data is carried out to avoid kernel crashes during
  *		operation.  Smaller types can use string space on the stack;
  *		larger programs can use map data to store the string
- *		representation.
+ *		representation.  Module-specific data structures can be
+ *		displayed if the module name is supplied.
  *
  *		The string can be subsequently shared with userspace via
  *		bpf_perf_event_output() or ring buffer interfaces.
@@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
  * potentially to specify additional details about the BTF pointer
  * (rather than its mode of display) - is included for future use.
  * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
+ * A module name can be specified for module-specific data.
  */
 struct btf_ptr {
 	void *ptr;
 	__u32 type_id;
 	__u32 flags;		/* BTF ptr flags; unused at present. */
+	const char *module;	/* optional module name. */
 };
 
 /*
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6b2d508..3ddd1fd 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5738,6 +5738,24 @@ struct btf_module {
 static LIST_HEAD(btf_modules);
 static DEFINE_MUTEX(btf_module_mutex);
 
+struct btf *bpf_get_btf_module(const char *name)
+{
+	struct btf *btf = ERR_PTR(-ENOENT);
+	struct btf_module *btf_mod, *tmp;
+
+	mutex_lock(&btf_module_mutex);
+	list_for_each_entry_safe(btf_mod, tmp, &btf_modules, list) {
+		if (!btf_mod->btf || strcmp(name, btf_mod->btf->name) != 0)
+			continue;
+
+		refcount_inc(&btf_mod->btf->refcnt);
+		btf = btf_mod->btf;
+		break;
+	}
+	mutex_unlock(&btf_module_mutex);
+	return btf;
+}
+
 static ssize_t
 btf_module_read(struct file *file, struct kobject *kobj,
 		struct bin_attribute *bin_attr,
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index cfce60a..a4d5a26 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -73,8 +73,7 @@ static struct bpf_raw_event_map *bpf_get_raw_tracepoint_module(const char *name)
 u64 bpf_get_stack(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
 
 static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
-				  u64 flags, const struct btf **btf,
-				  s32 *btf_id);
+				  u64 flags, struct btf **btf, s32 *btf_id);
 
 /**
  * trace_call_bpf - invoke BPF program
@@ -784,7 +783,7 @@ struct bpf_seq_printf_buf {
 BPF_CALL_4(bpf_seq_printf_btf, struct seq_file *, m, struct btf_ptr *, ptr,
 	   u32, btf_ptr_size, u64, flags)
 {
-	const struct btf *btf;
+	struct btf *btf;
 	s32 btf_id;
 	int ret;
 
@@ -792,7 +791,11 @@ struct bpf_seq_printf_buf {
 	if (ret)
 		return ret;
 
-	return btf_type_seq_show_flags(btf, btf_id, ptr->ptr, m, flags);
+	ret = btf_type_seq_show_flags(btf, btf_id, ptr->ptr, m, flags);
+	if (btf_ptr_size == sizeof(struct btf_ptr) && ptr->module)
+		btf_put(btf);
+
+	return ret;
 }
 
 static const struct bpf_func_proto bpf_seq_printf_btf_proto = {
@@ -1199,18 +1202,33 @@ static bool bpf_d_path_allowed(const struct bpf_prog *prog)
 			 BTF_F_PTR_RAW | BTF_F_ZERO)
 
 static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
-				  u64 flags, const struct btf **btf,
+				  u64 flags, struct btf **btf,
 				  s32 *btf_id)
 {
+	char modname[MODULE_NAME_LEN];
 	const struct btf_type *t;
+	int ret;
 
 	if (unlikely(flags & ~(BTF_F_ALL)))
 		return -EINVAL;
 
-	if (btf_ptr_size != sizeof(struct btf_ptr))
+	/* For backwards compatibility, ensure that we support BPF
+	 * programs compiled with older "struct btf_ptr" definitions
+	 * that lacked the "module" field.
+	 */
+	if (btf_ptr_size != sizeof(struct btf_ptr) &&
+	    btf_ptr_size != sizeof(struct btf_ptr) - sizeof(char *))
 		return -EINVAL;
 
-	*btf = bpf_get_btf_vmlinux();
+	if (btf_ptr_size == sizeof(struct btf_ptr) && ptr->module) {
+		ret = copy_from_kernel_nofault(modname, ptr->module,
+					       sizeof(modname));
+		if (ret)
+			return ret;
+
+		*btf = bpf_get_btf_module(modname);
+	} else
+		*btf = bpf_get_btf_vmlinux();
 
 	if (IS_ERR_OR_NULL(*btf))
 		return PTR_ERR(*btf);
@@ -1231,7 +1249,7 @@ static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
 BPF_CALL_5(bpf_snprintf_btf, char *, str, u32, str_size, struct btf_ptr *, ptr,
 	   u32, btf_ptr_size, u64, flags)
 {
-	const struct btf *btf;
+	struct btf *btf;
 	s32 btf_id;
 	int ret;
 
@@ -1239,8 +1257,12 @@ static int bpf_btf_printf_prepare(struct btf_ptr *ptr, u32 btf_ptr_size,
 	if (ret)
 		return ret;
 
-	return btf_type_snprintf_show(btf, btf_id, ptr->ptr, str, str_size,
-				      flags);
+	ret = btf_type_snprintf_show(btf, btf_id, ptr->ptr, str, str_size,
+				     flags);
+	if (btf_ptr_size == sizeof(struct btf_ptr) && ptr->module)
+		btf_put(btf);
+
+	return ret;
 }
 
 const struct bpf_func_proto bpf_snprintf_btf_proto = {
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 162999b..26978be 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
  *		the pointer data is carried out to avoid kernel crashes during
  *		operation.  Smaller types can use string space on the stack;
  *		larger programs can use map data to store the string
- *		representation.
+ *		representation.  Module-specific data structures can be
+ *		displayed if the module name is supplied.
  *
  *		The string can be subsequently shared with userspace via
  *		bpf_perf_event_output() or ring buffer interfaces.
@@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
  * potentially to specify additional details about the BTF pointer
  * (rather than its mode of display) - is included for future use.
  * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
+ * A module name can be specified for module-specific data.
  */
 struct btf_ptr {
 	void *ptr;
 	__u32 type_id;
 	__u32 flags;		/* BTF ptr flags; unused at present. */
+	const char *module;	/* optional module name. */
 };
 
 /*
-- 
1.8.3.1

[RFC bpf-next 2/3] libbpf: bpf__find_by_name[_kind] should use btf__get_nr_types()

[Alan Maguire]

When operating on split BTF, btf__find_by_name[_kind] will not
iterate over all types since they use btf->nr_types to show
the number of types to iterate over.  For split BTF this is
the number of types _on top of base BTF_, so it will
underestimate the number of types to iterate over, especially
for vmlinux + module BTF, where the latter is much smaller.

Use btf__get_nr_types() instead.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/btf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 2d0d064..0fccf4b 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -679,7 +679,7 @@ __s32 btf__find_by_name(const struct btf *btf, const char *type_name)
 	if (!strcmp(type_name, "void"))
 		return 0;
 
-	for (i = 1; i <= btf->nr_types; i++) {
+	for (i = 1; i <= btf__get_nr_types(btf); i++) {
 		const struct btf_type *t = btf__type_by_id(btf, i);
 		const char *name = btf__name_by_offset(btf, t->name_off);
 
@@ -698,7 +698,7 @@ __s32 btf__find_by_name_kind(const struct btf *btf, const char *type_name,
 	if (kind == BTF_KIND_UNKN || !strcmp(type_name, "void"))
 		return 0;
 
-	for (i = 1; i <= btf->nr_types; i++) {
+	for (i = 1; i <= btf__get_nr_types(btf); i++) {
 		const struct btf_type *t = btf__type_by_id(btf, i);
 		const char *name;
 
-- 
1.8.3.1

[RFC bpf-next 3/3] selftests/bpf: verify module-specific types can be shown via bpf_snprintf_btf

[Alan Maguire]

Verify that specifying a module name in "struct btf_ptr *" along
with a type id of a module-specific type will succeed.

veth_stats_rx() is chosen because its function signature consists
of a module-specific type "struct veth_stats" and a kernel-specific
one "struct net_device".

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 .../selftests/bpf/prog_tests/snprintf_btf_mod.c    | 96 ++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/btf_ptr.h        |  1 +
 tools/testing/selftests/bpf/progs/veth_stats_rx.c  | 73 ++++++++++++++++
 3 files changed, 170 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf_btf_mod.c
 create mode 100644 tools/testing/selftests/bpf/progs/veth_stats_rx.c

diff --git a/tools/testing/selftests/bpf/prog_tests/snprintf_btf_mod.c b/tools/testing/selftests/bpf/prog_tests/snprintf_btf_mod.c
new file mode 100644
index 0000000..f1b12df
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/snprintf_btf_mod.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <linux/btf.h>
+#include <bpf/btf.h>
+#include "veth_stats_rx.skel.h"
+
+#define VETH_NAME	"bpfveth0"
+
+/* Demonstrate that bpf_snprintf_btf succeeds for both module-specific
+ * and kernel-defined data structures; veth_stats_rx() is used as
+ * it has both module-specific and kernel-defined data as arguments.
+ * This test assumes that veth is built as a module and will skip if not.
+ */
+void test_snprintf_btf_mod(void)
+{
+	struct btf *vmlinux_btf = NULL, *veth_btf = NULL;
+	struct veth_stats_rx *skel = NULL;
+	struct veth_stats_rx__bss *bss;
+	int err, duration = 0;
+	__u32 id;
+
+	err = system("ip link add name " VETH_NAME " type veth");
+	if (CHECK(err, "system", "ip link add veth failed: %d\n", err))
+		return;
+
+	vmlinux_btf = btf__parse_raw("/sys/kernel/btf/vmlinux");
+	err = libbpf_get_error(vmlinux_btf);
+	if (CHECK(err, "parse vmlinux BTF", "failed parsing vmlinux BTF: %d\n",
+		  err))
+		goto cleanup;
+	veth_btf = btf__parse_raw_split("/sys/kernel/btf/veth", vmlinux_btf);
+	err = libbpf_get_error(veth_btf);
+	if (err == -ENOENT) {
+		printf("%s:SKIP:no BTF info for veth\n", __func__);
+		test__skip();
+                goto cleanup;
+	}
+
+	if (CHECK(err, "parse veth BTF", "failed parsing veth BTF: %d\n", err))
+		goto cleanup;
+
+	skel = veth_stats_rx__open();
+	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
+		goto cleanup;
+
+	err = veth_stats_rx__load(skel);
+	if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
+		goto cleanup;
+
+	bss = skel->bss;
+
+	bss->veth_stats_btf_id = btf__find_by_name(veth_btf, "veth_stats");
+
+	if (CHECK(bss->veth_stats_btf_id <= 0, "find 'struct veth_stats'",
+		  "could not find 'struct veth_stats' in veth BTF: %d",
+		  bss->veth_stats_btf_id))
+		goto cleanup;
+
+	err = veth_stats_rx__attach(skel);
+	if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+		goto cleanup;
+
+	/* generate stats event, then delete; this ensures the program
+	 * triggers prior to reading status.
+	 */
+	err = system("ethtool -S " VETH_NAME " > /dev/null");
+	if (CHECK(err, "system", "ethtool -S failed: %d\n", err))
+		goto cleanup;
+
+	system("ip link delete " VETH_NAME);
+
+	/*
+	 * Make sure veth_stats_rx program was triggered and it set
+	 * expected return values from bpf_trace_printk()s and all
+	 * tests ran.
+	 */
+	if (CHECK(bss->ret <= 0,
+		  "bpf_snprintf_btf: got return value",
+		  "ret <= 0 %ld test %d\n", bss->ret, bss->ran_subtests))
+		goto cleanup;
+
+	if (CHECK(bss->ran_subtests == 0, "check if subtests ran",
+		  "no subtests ran, did BPF program run?"))
+		goto cleanup;
+
+	if (CHECK(bss->num_subtests != bss->ran_subtests,
+		  "check all subtests ran",
+		  "only ran %d of %d tests\n", bss->num_subtests,
+		  bss->ran_subtests))
+		goto cleanup;
+
+cleanup:
+	system("ip link delete " VETH_NAME ">/dev/null 2>&1");
+	if (skel)
+		veth_stats_rx__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/btf_ptr.h b/tools/testing/selftests/bpf/progs/btf_ptr.h
index c3c9797..afef9b3 100644
--- a/tools/testing/selftests/bpf/progs/btf_ptr.h
+++ b/tools/testing/selftests/bpf/progs/btf_ptr.h
@@ -17,6 +17,7 @@ struct btf_ptr {
 	void *ptr;
 	__u32 type_id;
 	__u32 flags;
+	const char *module;
 };
 
 enum {
diff --git a/tools/testing/selftests/bpf/progs/veth_stats_rx.c b/tools/testing/selftests/bpf/progs/veth_stats_rx.c
new file mode 100644
index 0000000..6ec7ce2
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/veth_stats_rx.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020, Oracle and/or its affiliates. */
+
+#include "btf_ptr.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+
+#include <errno.h>
+
+long ret = 0;
+int num_subtests = 0;
+int ran_subtests = 0;
+s32 veth_stats_btf_id = 0;
+
+#define STRSIZE			2048
+
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x)	(sizeof(x) / sizeof((x)[0]))
+#endif
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, char[STRSIZE]);
+} strdata SEC(".maps");
+
+SEC("kprobe/veth_stats_rx")
+int veth_stats_rx(struct pt_regs *ctx)
+{
+	static __u64 flags[] = { 0, BTF_F_COMPACT, BTF_F_ZERO, BTF_F_PTR_RAW,
+				 BTF_F_NONAME, BTF_F_COMPACT | BTF_F_ZERO |
+				 BTF_F_PTR_RAW | BTF_F_NONAME };
+	static const char mod[] = "veth";
+	const char *mods[] = { mod, 0 };
+	static struct btf_ptr p = { };
+	__u32 btf_ids[] = { 0, 0 };
+	void *ptrs[] = { 0, 0 };
+	__u32 key = 0;
+	int i, j;
+	char *str;
+
+	btf_ids[0] = veth_stats_btf_id;
+	ptrs[0] = (void *)PT_REGS_PARM1_CORE(ctx);
+#if __has_builtin(__builtin_btf_type_id)
+	btf_ids[1] = bpf_core_type_id_kernel(struct net_device);
+	ptrs[1] = (void *)PT_REGS_PARM2_CORE(ctx);
+#endif
+	str = bpf_map_lookup_elem(&strdata, &key);
+	if (!str)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(btf_ids); i++) {
+		if (btf_ids[i] == 0)
+			continue;
+		p.type_id = btf_ids[i];
+		p.ptr = ptrs[i];
+		p.module = mods[i];
+		for (j = 0; j < ARRAY_SIZE(flags); j++) {
+			++num_subtests;
+			ret = bpf_snprintf_btf(str, STRSIZE, &p, sizeof(p), 0);
+			if (ret < 0)
+				bpf_printk("returned %d when writing id %d",
+					   ret, p.type_id);
+			++ran_subtests;
+		}
+	}
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
1.8.3.1

Re: [RFC bpf-next 2/3] libbpf: bpf__find_by_name[_kind] should use btf__get_nr_types()

[Andrii Nakryiko]

On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> When operating on split BTF, btf__find_by_name[_kind] will not
> iterate over all types since they use btf->nr_types to show
> the number of types to iterate over.  For split BTF this is
> the number of types _on top of base BTF_, so it will
> underestimate the number of types to iterate over, especially
> for vmlinux + module BTF, where the latter is much smaller.
>
> Use btf__get_nr_types() instead.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---

Good catch. I'm amazed I didn't fix it up when I implemented split BTF
support, I distinctly remember looking at these two APIs...

Can you please add Fixes tag and post this as a separate patch? There
is no need to wait on all the other changes.

Fixes: ba451366bf44 ("libbpf: Implement basic split BTF support")

>  tools/lib/bpf/btf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 2d0d064..0fccf4b 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -679,7 +679,7 @@ __s32 btf__find_by_name(const struct btf *btf, const char *type_name)
>         if (!strcmp(type_name, "void"))
>                 return 0;
>
> -       for (i = 1; i <= btf->nr_types; i++) {
> +       for (i = 1; i <= btf__get_nr_types(btf); i++) {

I think it's worthwhile to cache the result of btf__get_nr_types(btf)
in a local variable instead of re-calculating it thousands of times.

>                 const struct btf_type *t = btf__type_by_id(btf, i);
>                 const char *name = btf__name_by_offset(btf, t->name_off);
>
> @@ -698,7 +698,7 @@ __s32 btf__find_by_name_kind(const struct btf *btf, const char *type_name,
>         if (kind == BTF_KIND_UNKN || !strcmp(type_name, "void"))
>                 return 0;
>
> -       for (i = 1; i <= btf->nr_types; i++) {
> +       for (i = 1; i <= btf__get_nr_types(btf); i++) {

same as above


>                 const struct btf_type *t = btf__type_by_id(btf, i);
>                 const char *name;
>
> --
> 1.8.3.1
>

Re: [RFC bpf-next 1/3] bpf: add module support to btf display helpers

[Andrii Nakryiko]

On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
> argument that specifies type information about the type to
> be displayed.  Augment this information to include a module
> name, allowing such display to support module types.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  include/linux/btf.h            |  8 ++++++++
>  include/uapi/linux/bpf.h       |  5 ++++-
>  kernel/bpf/btf.c               | 18 ++++++++++++++++++
>  kernel/trace/bpf_trace.c       | 42 ++++++++++++++++++++++++++++++++----------
>  tools/include/uapi/linux/bpf.h |  5 ++++-
>  5 files changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 2bf6418..d55ca00 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo *btf_type_var_secinfo(
>  const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>  const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>  struct btf *btf_parse_vmlinux(void);
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> +struct btf *bpf_get_btf_module(const char *name);
> +#else
> +static inline struct btf *bpf_get_btf_module(const char *name)
> +{
> +       return ERR_PTR(-ENOTSUPP);
> +}
> +#endif
>  struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>  #else
>  static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 162999b..26978be 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
>   *             the pointer data is carried out to avoid kernel crashes during
>   *             operation.  Smaller types can use string space on the stack;
>   *             larger programs can use map data to store the string
> - *             representation.
> + *             representation.  Module-specific data structures can be
> + *             displayed if the module name is supplied.
>   *
>   *             The string can be subsequently shared with userspace via
>   *             bpf_perf_event_output() or ring buffer interfaces.
> @@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
>   * potentially to specify additional details about the BTF pointer
>   * (rather than its mode of display) - is included for future use.
>   * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
> + * A module name can be specified for module-specific data.
>   */
>  struct btf_ptr {
>         void *ptr;
>         __u32 type_id;
>         __u32 flags;            /* BTF ptr flags; unused at present. */
> +       const char *module;     /* optional module name. */

I think module name is a wrong API here, similarly how type name was
wrong API for specifying the type (and thus we use type_id here).
Using the module's BTF ID seems like a more suitable interface. That's
what I'm going to use for all kinds of existing BPF APIs that expect
BTF type to attach BPF programs.

Right now, we use only type_id and implicitly know that it's in
vmlinux BTF. With module BTFs, we now need a pair of BTF object ID +
BTF type ID to uniquely identify the type. vmlinux BTF now can be
specified in two different ways: either leaving BTF object ID as zero
(for simplicity and backwards compatibility) or specifying it's actual
BTF obj ID (which pretty much always should be 1, btw). This feels
like a natural extension, WDYT?

And similar to type_id, no one should expect users to specify these
IDs by hand, Clang built-in and libbpf should work together to figure
this out for the kernel to use.

BTW, with module names there is an extra problem for end users. Some
types could be either built-in or built as a module (e.g., XFS data
structures). Why would we require BPF users to care which is the case
on any given host? It feels right now that we should just extend the
existing __builtin_btf_type_id() helper to generate ldimm64
instructions that would encode both BTF type ID and BTF object ID.
This would just naturally add transparent module BTF support without
BPF programs having to do any changes.

But we need to do a bit of thinking and experimentation with Yonghong,
haven't gotten around to this yet, you are running a bit ahead of me
with module BTFs. :)

>  };
>
>  /*

[...]

>  struct btf_ptr {
>         void *ptr;
>         __u32 type_id;
>         __u32 flags;            /* BTF ptr flags; unused at present. */

Also, if flags are not used at present, can we repurpose it to just
encode btf_obj_id and avoid (at least for now) the backwards
compatibility checks based on btf_ptr size?

> +       const char *module;     /* optional module name. */
>  };
>
>  /*
> --
> 1.8.3.1
>

Re: [RFC bpf-next 3/3] selftests/bpf: verify module-specific types can be shown via bpf_snprintf_btf

[Andrii Nakryiko]

On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Verify that specifying a module name in "struct btf_ptr *" along
> with a type id of a module-specific type will succeed.
>
> veth_stats_rx() is chosen because its function signature consists
> of a module-specific type "struct veth_stats" and a kernel-specific
> one "struct net_device".
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  .../selftests/bpf/prog_tests/snprintf_btf_mod.c    | 96 ++++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/btf_ptr.h        |  1 +
>  tools/testing/selftests/bpf/progs/veth_stats_rx.c  | 73 ++++++++++++++++
>  3 files changed, 170 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/snprintf_btf_mod.c
>  create mode 100644 tools/testing/selftests/bpf/progs/veth_stats_rx.c
>

[...]

> +       err = veth_stats_rx__load(skel);
> +       if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
> +               goto cleanup;
> +
> +       bss = skel->bss;
> +
> +       bss->veth_stats_btf_id = btf__find_by_name(veth_btf, "veth_stats");

This is really awkward that this needs to be done from user-space.
Libbpf will be able to do this regardless of whether the type is in
vmlinux or kernel module. See my comments on patch #1.

> +
> +       if (CHECK(bss->veth_stats_btf_id <= 0, "find 'struct veth_stats'",
> +                 "could not find 'struct veth_stats' in veth BTF: %d",
> +                 bss->veth_stats_btf_id))
> +               goto cleanup;
> +

[...]

> +       btf_ids[0] = veth_stats_btf_id;
> +       ptrs[0] = (void *)PT_REGS_PARM1_CORE(ctx);
> +#if __has_builtin(__builtin_btf_type_id)

nit: there are a bunch of selftests that just assume we have this
built-in, so I don't think you need to guard it with #if here.

> +       btf_ids[1] = bpf_core_type_id_kernel(struct net_device);
> +       ptrs[1] = (void *)PT_REGS_PARM2_CORE(ctx);
> +#endif

[...]

Re: [RFC bpf-next 1/3] bpf: add module support to btf display helpers

[Alexei Starovoitov]

On Fri, Nov 13, 2020 at 10:59 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
> > argument that specifies type information about the type to
> > be displayed.  Augment this information to include a module
> > name, allowing such display to support module types.
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >  include/linux/btf.h            |  8 ++++++++
> >  include/uapi/linux/bpf.h       |  5 ++++-
> >  kernel/bpf/btf.c               | 18 ++++++++++++++++++
> >  kernel/trace/bpf_trace.c       | 42 ++++++++++++++++++++++++++++++++----------
> >  tools/include/uapi/linux/bpf.h |  5 ++++-
> >  5 files changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 2bf6418..d55ca00 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo *btf_type_var_secinfo(
> >  const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
> >  const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> >  struct btf *btf_parse_vmlinux(void);
> > +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> > +struct btf *bpf_get_btf_module(const char *name);
> > +#else
> > +static inline struct btf *bpf_get_btf_module(const char *name)
> > +{
> > +       return ERR_PTR(-ENOTSUPP);
> > +}
> > +#endif
> >  struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> >  #else
> >  static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 162999b..26978be 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
> >   *             the pointer data is carried out to avoid kernel crashes during
> >   *             operation.  Smaller types can use string space on the stack;
> >   *             larger programs can use map data to store the string
> > - *             representation.
> > + *             representation.  Module-specific data structures can be
> > + *             displayed if the module name is supplied.
> >   *
> >   *             The string can be subsequently shared with userspace via
> >   *             bpf_perf_event_output() or ring buffer interfaces.
> > @@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
> >   * potentially to specify additional details about the BTF pointer
> >   * (rather than its mode of display) - is included for future use.
> >   * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
> > + * A module name can be specified for module-specific data.
> >   */
> >  struct btf_ptr {
> >         void *ptr;
> >         __u32 type_id;
> >         __u32 flags;            /* BTF ptr flags; unused at present. */
> > +       const char *module;     /* optional module name. */
>
> I think module name is a wrong API here, similarly how type name was
> wrong API for specifying the type (and thus we use type_id here).
> Using the module's BTF ID seems like a more suitable interface. That's
> what I'm going to use for all kinds of existing BPF APIs that expect
> BTF type to attach BPF programs.
>
> Right now, we use only type_id and implicitly know that it's in
> vmlinux BTF. With module BTFs, we now need a pair of BTF object ID +
> BTF type ID to uniquely identify the type. vmlinux BTF now can be
> specified in two different ways: either leaving BTF object ID as zero
> (for simplicity and backwards compatibility) or specifying it's actual
> BTF obj ID (which pretty much always should be 1, btw). This feels
> like a natural extension, WDYT?
>
> And similar to type_id, no one should expect users to specify these
> IDs by hand, Clang built-in and libbpf should work together to figure
> this out for the kernel to use.
>
> BTW, with module names there is an extra problem for end users. Some
> types could be either built-in or built as a module (e.g., XFS data
> structures). Why would we require BPF users to care which is the case
> on any given host?

+1.
As much as possible libbpf should try to hide the difference between
type in a module vs type in the vmlinux, since that difference most of the
time is irrelevant from bpf prog pov.

Re: [RFC bpf-next 1/3] bpf: add module support to btf display helpers

[Yonghong Song]

On 11/14/20 8:04 AM, Alexei Starovoitov wrote:
> On Fri, Nov 13, 2020 at 10:59 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>>
>>> bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
>>> argument that specifies type information about the type to
>>> be displayed.  Augment this information to include a module
>>> name, allowing such display to support module types.
>>>
>>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>>> ---
>>>   include/linux/btf.h            |  8 ++++++++
>>>   include/uapi/linux/bpf.h       |  5 ++++-
>>>   kernel/bpf/btf.c               | 18 ++++++++++++++++++
>>>   kernel/trace/bpf_trace.c       | 42 ++++++++++++++++++++++++++++++++----------
>>>   tools/include/uapi/linux/bpf.h |  5 ++++-
>>>   5 files changed, 66 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>>> index 2bf6418..d55ca00 100644
>>> --- a/include/linux/btf.h
>>> +++ b/include/linux/btf.h
>>> @@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo *btf_type_var_secinfo(
>>>   const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
>>>   const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>>>   struct btf *btf_parse_vmlinux(void);
>>> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>>> +struct btf *bpf_get_btf_module(const char *name);
>>> +#else
>>> +static inline struct btf *bpf_get_btf_module(const char *name)
>>> +{
>>> +       return ERR_PTR(-ENOTSUPP);
>>> +}
>>> +#endif
>>>   struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
>>>   #else
>>>   static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 162999b..26978be 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
>>>    *             the pointer data is carried out to avoid kernel crashes during
>>>    *             operation.  Smaller types can use string space on the stack;
>>>    *             larger programs can use map data to store the string
>>> - *             representation.
>>> + *             representation.  Module-specific data structures can be
>>> + *             displayed if the module name is supplied.
>>>    *
>>>    *             The string can be subsequently shared with userspace via
>>>    *             bpf_perf_event_output() or ring buffer interfaces.
>>> @@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
>>>    * potentially to specify additional details about the BTF pointer
>>>    * (rather than its mode of display) - is included for future use.
>>>    * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
>>> + * A module name can be specified for module-specific data.
>>>    */
>>>   struct btf_ptr {
>>>          void *ptr;
>>>          __u32 type_id;
>>>          __u32 flags;            /* BTF ptr flags; unused at present. */
>>> +       const char *module;     /* optional module name. */
>>
>> I think module name is a wrong API here, similarly how type name was
>> wrong API for specifying the type (and thus we use type_id here).
>> Using the module's BTF ID seems like a more suitable interface. That's
>> what I'm going to use for all kinds of existing BPF APIs that expect
>> BTF type to attach BPF programs.
>>
>> Right now, we use only type_id and implicitly know that it's in
>> vmlinux BTF. With module BTFs, we now need a pair of BTF object ID +
>> BTF type ID to uniquely identify the type. vmlinux BTF now can be
>> specified in two different ways: either leaving BTF object ID as zero
>> (for simplicity and backwards compatibility) or specifying it's actual
>> BTF obj ID (which pretty much always should be 1, btw). This feels
>> like a natural extension, WDYT?
>>
>> And similar to type_id, no one should expect users to specify these
>> IDs by hand, Clang built-in and libbpf should work together to figure
>> this out for the kernel to use.
>>
>> BTW, with module names there is an extra problem for end users. Some
>> types could be either built-in or built as a module (e.g., XFS data
>> structures). Why would we require BPF users to care which is the case
>> on any given host?
> 
> +1.
> As much as possible libbpf should try to hide the difference between
> type in a module vs type in the vmlinux, since that difference most of the
> time is irrelevant from bpf prog pov.

I just crafted a llvm patch where for __builtin_btf_type_id(), a 64bit 
value is returned instead of a 32bit value. libbpf can use the lower
32bit as the btf_type_id and upper 32bit as the kernel module btf id.

    https://reviews.llvm.org/D91489

feel free to experiment with it to see whether it helps.

Re: [RFC bpf-next 1/3] bpf: add module support to btf display helpers

[Alan Maguire]

On Sat, 14 Nov 2020, Yonghong Song wrote:

> 
> 
> On 11/14/20 8:04 AM, Alexei Starovoitov wrote:
> > On Fri, Nov 13, 2020 at 10:59 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@oracle.com>
> >> wrote:
> >>>
> >>> bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
> >>> argument that specifies type information about the type to
> >>> be displayed.  Augment this information to include a module
> >>> name, allowing such display to support module types.
> >>>
> >>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> >>> ---
> >>>   include/linux/btf.h            |  8 ++++++++
> >>>   include/uapi/linux/bpf.h       |  5 ++++-
> >>>   kernel/bpf/btf.c               | 18 ++++++++++++++++++
> >>>   kernel/trace/bpf_trace.c       | 42
> >>>   ++++++++++++++++++++++++++++++++----------
> >>>   tools/include/uapi/linux/bpf.h |  5 ++++-
> >>>   5 files changed, 66 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/include/linux/btf.h b/include/linux/btf.h
> >>> index 2bf6418..d55ca00 100644
> >>> --- a/include/linux/btf.h
> >>> +++ b/include/linux/btf.h
> >>> @@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo
> >>> *btf_type_var_secinfo(
> >>>   const struct btf_type *btf_type_by_id(const struct btf *btf, u32
> >>>   type_id);
> >>>   const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> >>>   struct btf *btf_parse_vmlinux(void);
> >>> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> >>> +struct btf *bpf_get_btf_module(const char *name);
> >>> +#else
> >>> +static inline struct btf *bpf_get_btf_module(const char *name)
> >>> +{
> >>> +       return ERR_PTR(-ENOTSUPP);
> >>> +}
> >>> +#endif
> >>>   struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> >>>   #else
> >>>   static inline const struct btf_type *btf_type_by_id(const struct btf
> >>>   *btf,
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index 162999b..26978be 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
> >>>    *             the pointer data is carried out to avoid kernel crashes
> >>>    during
> >>>    *             operation.  Smaller types can use string space on the
> >>>    stack;
> >>>    *             larger programs can use map data to store the string
> >>> - *             representation.
> >>> + *             representation.  Module-specific data structures can be
> >>> + *             displayed if the module name is supplied.
> >>>    *
> >>>    *             The string can be subsequently shared with userspace via
> >>>    *             bpf_perf_event_output() or ring buffer interfaces.
> >>> @@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
> >>>    * potentially to specify additional details about the BTF pointer
> >>>    * (rather than its mode of display) - is included for future use.
> >>>    * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
> >>> + * A module name can be specified for module-specific data.
> >>>   */
> >>>   struct btf_ptr {
> >>>          void *ptr;
> >>>          __u32 type_id;
> >>>          __u32 flags;            /* BTF ptr flags; unused at present. */
> >>> +       const char *module;     /* optional module name. */
> >>
> >> I think module name is a wrong API here, similarly how type name was
> >> wrong API for specifying the type (and thus we use type_id here).
> >> Using the module's BTF ID seems like a more suitable interface. That's
> >> what I'm going to use for all kinds of existing BPF APIs that expect
> >> BTF type to attach BPF programs.
> >>
> >> Right now, we use only type_id and implicitly know that it's in
> >> vmlinux BTF. With module BTFs, we now need a pair of BTF object ID +
> >> BTF type ID to uniquely identify the type. vmlinux BTF now can be
> >> specified in two different ways: either leaving BTF object ID as zero
> >> (for simplicity and backwards compatibility) or specifying it's actual
> >> BTF obj ID (which pretty much always should be 1, btw). This feels
> >> like a natural extension, WDYT?
> >>
> >> And similar to type_id, no one should expect users to specify these
> >> IDs by hand, Clang built-in and libbpf should work together to figure
> >> this out for the kernel to use.
> >>
> >> BTW, with module names there is an extra problem for end users. Some
> >> types could be either built-in or built as a module (e.g., XFS data
> >> structures). Why would we require BPF users to care which is the case
> >> on any given host?
> > 
> > +1.
> > As much as possible libbpf should try to hide the difference between
> > type in a module vs type in the vmlinux, since that difference most of the
> > time is irrelevant from bpf prog pov.
>

All sounds good to me - I've split out the libbpf fix and 
put together an updated patchset for the helpers/test which
converts the currently unused __u32 "flags" field in
struct btf_ptr to an "obj_id" field.  If obj_id is > 1 it
is presumed to be a module ID.  I'd suggest we could move
ahead with those changes, using the more clunky methods
to retrieve the module-specific BTF id, and later fix up
the test to use Yonghong's __builtin_btf_type_id()
modification.  Does that sound reasonable?

In connection to this, I wonder if libbpf could
benefit from a simple helper btf__id() (similar
to btf__fd()), allowing easy retrieval of the
object ID associated with module BTF?  I suspect
we will always have cases in general-purpose
tracers where we need to look up BTF ids of
objects dynamically, so such a function would
help in that case.

> I just crafted a llvm patch where for __builtin_btf_type_id(), a 64bit value
> is returned instead of a 32bit value. libbpf can use the lower
> 32bit as the btf_type_id and upper 32bit as the kernel module btf id.
> 
>    https://reviews.llvm.org/D91489
> 
> feel free to experiment with it to see whether it helps.
> 
> 

Great! I'll give it a try, thanks!

Alan

Re: [RFC bpf-next 1/3] bpf: add module support to btf display helpers

[Andrii Nakryiko]

On Sun, Nov 15, 2020 at 2:53 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On Sat, 14 Nov 2020, Yonghong Song wrote:
>
> >
> >
> > On 11/14/20 8:04 AM, Alexei Starovoitov wrote:
> > > On Fri, Nov 13, 2020 at 10:59 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > >>
> > >> On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@oracle.com>
> > >> wrote:
> > >>>
> > >>> bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
> > >>> argument that specifies type information about the type to
> > >>> be displayed.  Augment this information to include a module
> > >>> name, allowing such display to support module types.
> > >>>
> > >>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > >>> ---
> > >>>   include/linux/btf.h            |  8 ++++++++
> > >>>   include/uapi/linux/bpf.h       |  5 ++++-
> > >>>   kernel/bpf/btf.c               | 18 ++++++++++++++++++
> > >>>   kernel/trace/bpf_trace.c       | 42
> > >>>   ++++++++++++++++++++++++++++++++----------
> > >>>   tools/include/uapi/linux/bpf.h |  5 ++++-
> > >>>   5 files changed, 66 insertions(+), 12 deletions(-)
> > >>>
> > >>> diff --git a/include/linux/btf.h b/include/linux/btf.h
> > >>> index 2bf6418..d55ca00 100644
> > >>> --- a/include/linux/btf.h
> > >>> +++ b/include/linux/btf.h
> > >>> @@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo
> > >>> *btf_type_var_secinfo(
> > >>>   const struct btf_type *btf_type_by_id(const struct btf *btf, u32
> > >>>   type_id);
> > >>>   const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> > >>>   struct btf *btf_parse_vmlinux(void);
> > >>> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> > >>> +struct btf *bpf_get_btf_module(const char *name);
> > >>> +#else
> > >>> +static inline struct btf *bpf_get_btf_module(const char *name)
> > >>> +{
> > >>> +       return ERR_PTR(-ENOTSUPP);
> > >>> +}
> > >>> +#endif
> > >>>   struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> > >>>   #else
> > >>>   static inline const struct btf_type *btf_type_by_id(const struct btf
> > >>>   *btf,
> > >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > >>> index 162999b..26978be 100644
> > >>> --- a/include/uapi/linux/bpf.h
> > >>> +++ b/include/uapi/linux/bpf.h
> > >>> @@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
> > >>>    *             the pointer data is carried out to avoid kernel crashes
> > >>>    during
> > >>>    *             operation.  Smaller types can use string space on the
> > >>>    stack;
> > >>>    *             larger programs can use map data to store the string
> > >>> - *             representation.
> > >>> + *             representation.  Module-specific data structures can be
> > >>> + *             displayed if the module name is supplied.
> > >>>    *
> > >>>    *             The string can be subsequently shared with userspace via
> > >>>    *             bpf_perf_event_output() or ring buffer interfaces.
> > >>> @@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
> > >>>    * potentially to specify additional details about the BTF pointer
> > >>>    * (rather than its mode of display) - is included for future use.
> > >>>    * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
> > >>> + * A module name can be specified for module-specific data.
> > >>>   */
> > >>>   struct btf_ptr {
> > >>>          void *ptr;
> > >>>          __u32 type_id;
> > >>>          __u32 flags;            /* BTF ptr flags; unused at present. */
> > >>> +       const char *module;     /* optional module name. */
> > >>
> > >> I think module name is a wrong API here, similarly how type name was
> > >> wrong API for specifying the type (and thus we use type_id here).
> > >> Using the module's BTF ID seems like a more suitable interface. That's
> > >> what I'm going to use for all kinds of existing BPF APIs that expect
> > >> BTF type to attach BPF programs.
> > >>
> > >> Right now, we use only type_id and implicitly know that it's in
> > >> vmlinux BTF. With module BTFs, we now need a pair of BTF object ID +
> > >> BTF type ID to uniquely identify the type. vmlinux BTF now can be
> > >> specified in two different ways: either leaving BTF object ID as zero
> > >> (for simplicity and backwards compatibility) or specifying it's actual
> > >> BTF obj ID (which pretty much always should be 1, btw). This feels
> > >> like a natural extension, WDYT?
> > >>
> > >> And similar to type_id, no one should expect users to specify these
> > >> IDs by hand, Clang built-in and libbpf should work together to figure
> > >> this out for the kernel to use.
> > >>
> > >> BTW, with module names there is an extra problem for end users. Some
> > >> types could be either built-in or built as a module (e.g., XFS data
> > >> structures). Why would we require BPF users to care which is the case
> > >> on any given host?
> > >
> > > +1.
> > > As much as possible libbpf should try to hide the difference between
> > > type in a module vs type in the vmlinux, since that difference most of the
> > > time is irrelevant from bpf prog pov.
> >
>
> All sounds good to me - I've split out the libbpf fix and

yeah, thanks, I've applied yesterday.

> put together an updated patchset for the helpers/test which
> converts the currently unused __u32 "flags" field in
> struct btf_ptr to an "obj_id" field.  If obj_id is > 1 it

I haven't seen your updated patches (you haven't sent them yet?). You
shouldn't assume obj_id == 1 means vmlinux, though. struct btf in
kernel would have kernel_btf == true and its name should be "vmlinux"
for vmlinux BTF, otherwise kernel_btf == true means module BTF. We can
add separate bool just to distinguish vmlinux vs module BTF, if it
makes life easier, I think.

> is presumed to be a module ID.  I'd suggest we could move
> ahead with those changes, using the more clunky methods
> to retrieve the module-specific BTF id, and later fix up
> the test to use Yonghong's __builtin_btf_type_id()
> modification.  Does that sound reasonable?

Yonghong already updated Clang. Corresponding libbpf changes are very
minimal, feel free to add them as well. If not, I'm going to do it
this week anyways. Is there any hurry to land this before the proper
module BTF lands in libbpf, though?

>
> In connection to this, I wonder if libbpf could
> benefit from a simple helper btf__id() (similar
> to btf__fd()), allowing easy retrieval of the
> object ID associated with module BTF?  I suspect
> we will always have cases in general-purpose
> tracers where we need to look up BTF ids of
> objects dynamically, so such a function would
> help in that case.

We can add that as a convenience wrapper on top of
bpf_obj_get_info_by_fd() and cache the result internally, yes.


>
> > I just crafted a llvm patch where for __builtin_btf_type_id(), a 64bit value
> > is returned instead of a 32bit value. libbpf can use the lower
> > 32bit as the btf_type_id and upper 32bit as the kernel module btf id.
> >
> >    https://reviews.llvm.org/D91489
> >
> > feel free to experiment with it to see whether it helps.
> >
> >
>
> Great! I'll give it a try, thanks!
>
> Alan