[RFC bpf-next v3 00/11] Registrating struct_ops types from modules

[RFC bpf-next v3 00/11] Registrating struct_ops types from modules

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Given the current constraints of the current implementation,
struct_ops cannot be registered dynamically. This presents a
significant limitation for modules like coming fuse-bpf, which seeks
to implement a new struct_ops type. To address this issue, a new API
is introduced that allows the registration of new struct_ops types
from modules.

Previously, struct_ops types were defined in bpf_struct_ops_types.h
and collected as a static array. The new API lets callers add new
struct_ops types dynamically. The static array has been removed and
replaced by the per-btf struct_ops_tab.

The struct_ops subsystem relies on BTF to determine the layout of
values in a struct_ops map and identify the subsystem that the
struct_ops map registers to. However, the kernel BTF does not include
the type information of struct_ops types defined by a module. The
struct_ops subsystem requires knowledge of the corresponding module
for a given struct_ops map and the utilization of BTF information from
that module. We empower libbpf to determine the correct module for
accessing the BTF information and pass an identity (FD) of the module
btf to the kernel. The kernel looks up type information and registered
struct_ops types directly from the given btf.

If a module exits while one or more struct_ops maps still refer to a
struct_ops type defined by the module, it can lead to unforeseen
complications. Therefore, it is crucial to ensure that a module
remains intact as long as any struct_ops map is still linked to a
struct_ops type defined by the module. To achieve this, every
struct_ops map holds a reference to the module for each struct_ops
map.

Changes from v2:

 - Remove struct_ops array, and add a per-btf (module) struct_ops_tab
   to collect registered struct_ops types.

 - Validate value_type by checking member names and types.

---
v2: https://lore.kernel.org/all/20230913061449.1918219-1-thinker.li@gmail.com/

Kui-Feng Lee (11):
  bpf: refactory struct_ops type initialization to a function.
  bpf: add struct_ops_tab to btf.
  bpf: add register and unregister functions for struct_ops.
  bpf: attach a module BTF to a bpf_struct_ops
  bpf: hold module for bpf_struct_ops_map.
  bpf: validate value_type
  bpf, net: switch to storing struct_ops in btf
  bpf: pass attached BTF to find correct type info of struct_ops progs.
  libbpf: Find correct module BTFs for struct_ops maps and progs.
  bpf: export btf_ctx_access to modules.
  selftests/bpf: test case for register_bpf_struct_ops().

 include/linux/bpf.h                           |  15 +-
 include/linux/btf.h                           |  36 ++
 include/uapi/linux/bpf.h                      |   4 +
 kernel/bpf/bpf_struct_ops.c                   | 378 ++++++++++++------
 kernel/bpf/bpf_struct_ops_types.h             |  12 -
 kernel/bpf/btf.c                              |  87 +++-
 kernel/bpf/syscall.c                          |   2 +-
 kernel/bpf/verifier.c                         |   4 +-
 net/bpf/bpf_dummy_struct_ops.c                |  12 +-
 net/ipv4/bpf_tcp_ca.c                         |  20 +-
 tools/include/uapi/linux/bpf.h                |   4 +
 tools/lib/bpf/bpf.c                           |   3 +-
 tools/lib/bpf/bpf.h                           |   4 +-
 tools/lib/bpf/libbpf.c                        | 121 +++---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  63 +++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |   5 +
 .../bpf/prog_tests/test_struct_ops_module.c   |  43 ++
 .../selftests/bpf/progs/struct_ops_module.c   |  30 ++
 18 files changed, 638 insertions(+), 205 deletions(-)
 delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_module.c

-- 
2.34.1

[RFC bpf-next v3 01/11] bpf: refactory struct_ops type initialization to a function.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Move most of code to bpf_struct_ops_init_one() that can be use to
initialize new struct_ops types registered dynamically.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/bpf_struct_ops.c | 157 +++++++++++++++++++-----------------
 1 file changed, 83 insertions(+), 74 deletions(-)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index db6176fb64dc..627cf1ea840a 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -110,102 +110,111 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
 
 static const struct btf_type *module_type;
 
-void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
+static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
+				    struct btf *btf,
+				    struct bpf_verifier_log *log)
 {
-	s32 type_id, value_id, module_id;
 	const struct btf_member *member;
-	struct bpf_struct_ops *st_ops;
 	const struct btf_type *t;
+	s32 type_id, value_id;
 	char value_name[128];
 	const char *mname;
-	u32 i, j;
+	int i;
 
-	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
-#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
+	if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
+	    sizeof(value_name)) {
+		pr_warn("struct_ops name %s is too long\n",
+			st_ops->name);
+		return;
+	}
+	sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
 
-	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
-	if (module_id < 0) {
-		pr_warn("Cannot find struct module in btf_vmlinux\n");
+	value_id = btf_find_by_name_kind(btf, value_name,
+					 BTF_KIND_STRUCT);
+	if (value_id < 0) {
+		pr_warn("Cannot find struct %s in btf_vmlinux\n",
+			value_name);
 		return;
 	}
-	module_type = btf_type_by_id(btf, module_id);
 
-	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
-		st_ops = bpf_struct_ops[i];
+	type_id = btf_find_by_name_kind(btf, st_ops->name,
+					BTF_KIND_STRUCT);
+	if (type_id < 0) {
+		pr_warn("Cannot find struct %s in btf_vmlinux\n",
+			st_ops->name);
+		return;
+	}
+	t = btf_type_by_id(btf, type_id);
+	if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
+		pr_warn("Cannot support #%u members in struct %s\n",
+			btf_type_vlen(t), st_ops->name);
+		return;
+	}
 
-		if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
-		    sizeof(value_name)) {
-			pr_warn("struct_ops name %s is too long\n",
+	for_each_member(i, t, member) {
+		const struct btf_type *func_proto;
+
+		mname = btf_name_by_offset(btf, member->name_off);
+		if (!*mname) {
+			pr_warn("anon member in struct %s is not supported\n",
 				st_ops->name);
-			continue;
+			break;
 		}
-		sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
 
-		value_id = btf_find_by_name_kind(btf, value_name,
-						 BTF_KIND_STRUCT);
-		if (value_id < 0) {
-			pr_warn("Cannot find struct %s in btf_vmlinux\n",
-				value_name);
-			continue;
+		if (__btf_member_bitfield_size(t, member)) {
+			pr_warn("bit field member %s in struct %s is not supported\n",
+				mname, st_ops->name);
+			break;
 		}
 
-		type_id = btf_find_by_name_kind(btf, st_ops->name,
-						BTF_KIND_STRUCT);
-		if (type_id < 0) {
-			pr_warn("Cannot find struct %s in btf_vmlinux\n",
-				st_ops->name);
-			continue;
-		}
-		t = btf_type_by_id(btf, type_id);
-		if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
-			pr_warn("Cannot support #%u members in struct %s\n",
-				btf_type_vlen(t), st_ops->name);
-			continue;
+		func_proto = btf_type_resolve_func_ptr(btf,
+						       member->type,
+						       NULL);
+		if (func_proto &&
+		    btf_distill_func_proto(log, btf,
+					   func_proto, mname,
+					   &st_ops->func_models[i])) {
+			pr_warn("Error in parsing func ptr %s in struct %s\n",
+				mname, st_ops->name);
+			break;
 		}
+	}
 
-		for_each_member(j, t, member) {
-			const struct btf_type *func_proto;
+	if (i == btf_type_vlen(t)) {
+		if (st_ops->init(btf)) {
+			pr_warn("Error in init bpf_struct_ops %s\n",
+				st_ops->name);
+		} else {
+			st_ops->type_id = type_id;
+			st_ops->type = t;
+			st_ops->value_id = value_id;
+			st_ops->value_type = btf_type_by_id(btf,
+							    value_id);
+		}
+	}
+}
 
-			mname = btf_name_by_offset(btf, member->name_off);
-			if (!*mname) {
-				pr_warn("anon member in struct %s is not supported\n",
-					st_ops->name);
-				break;
-			}
+void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
+{
+	struct bpf_struct_ops *st_ops;
+	s32 module_id;
+	u32 i;
 
-			if (__btf_member_bitfield_size(t, member)) {
-				pr_warn("bit field member %s in struct %s is not supported\n",
-					mname, st_ops->name);
-				break;
-			}
+	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
+#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
+#include "bpf_struct_ops_types.h"
+#undef BPF_STRUCT_OPS_TYPE
 
-			func_proto = btf_type_resolve_func_ptr(btf,
-							       member->type,
-							       NULL);
-			if (func_proto &&
-			    btf_distill_func_proto(log, btf,
-						   func_proto, mname,
-						   &st_ops->func_models[j])) {
-				pr_warn("Error in parsing func ptr %s in struct %s\n",
-					mname, st_ops->name);
-				break;
-			}
-		}
+	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
+	if (module_id < 0) {
+		pr_warn("Cannot find struct module in btf_vmlinux\n");
+		return;
+	}
+	module_type = btf_type_by_id(btf, module_id);
 
-		if (j == btf_type_vlen(t)) {
-			if (st_ops->init(btf)) {
-				pr_warn("Error in init bpf_struct_ops %s\n",
-					st_ops->name);
-			} else {
-				st_ops->type_id = type_id;
-				st_ops->type = t;
-				st_ops->value_id = value_id;
-				st_ops->value_type = btf_type_by_id(btf,
-								    value_id);
-			}
-		}
+	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
+		st_ops = bpf_struct_ops[i];
+		bpf_struct_ops_init_one(st_ops, btf, log);
 	}
 }
 
-- 
2.34.1

[RFC bpf-next v3 02/11] bpf: add struct_ops_tab to btf.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

struct_ops_tab will be used to restore registered struct_ops.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/btf.h |  9 +++++
 kernel/bpf/btf.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 928113a80a95..5fabe23aedd2 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -571,4 +571,13 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type
 	return btf_type_is_struct(t);
 }
 
+struct bpf_struct_ops;
+
+int btf_add_struct_ops_btf(struct bpf_struct_ops *st_ops,
+			   struct btf *btf);
+int btf_add_struct_ops(struct bpf_struct_ops *st_ops,
+		       struct module *owner);
+const struct bpf_struct_ops **
+btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
+
 #endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f93e835d90af..3fb9964f8672 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -241,6 +241,12 @@ struct btf_id_dtor_kfunc_tab {
 	struct btf_id_dtor_kfunc dtors[];
 };
 
+struct btf_struct_ops_tab {
+	u32 cnt;
+	u32 capacity;
+	struct bpf_struct_ops *ops[];
+};
+
 struct btf {
 	void *data;
 	struct btf_type **types;
@@ -258,6 +264,7 @@ struct btf {
 	struct btf_kfunc_set_tab *kfunc_set_tab;
 	struct btf_id_dtor_kfunc_tab *dtor_kfunc_tab;
 	struct btf_struct_metas *struct_meta_tab;
+	struct btf_struct_ops_tab *struct_ops_tab;
 
 	/* split BTF support */
 	struct btf *base_btf;
@@ -1688,11 +1695,20 @@ static void btf_free_struct_meta_tab(struct btf *btf)
 	btf->struct_meta_tab = NULL;
 }
 
+static void btf_free_struct_ops_tab(struct btf *btf)
+{
+	struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
+
+	kfree(tab);
+	btf->struct_ops_tab = NULL;
+}
+
 static void btf_free(struct btf *btf)
 {
 	btf_free_struct_meta_tab(btf);
 	btf_free_dtor_kfunc_tab(btf);
 	btf_free_kfunc_set_tab(btf);
+	btf_free_struct_ops_tab(btf);
 	kvfree(btf->types);
 	kvfree(btf->resolved_sizes);
 	kvfree(btf->resolved_ids);
@@ -8601,3 +8617,71 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
 
 	return !strncmp(reg_name, arg_name, cmp_len);
 }
+
+int btf_add_struct_ops_btf(struct bpf_struct_ops *st_ops, struct btf *btf)
+{
+	struct btf_struct_ops_tab *tab;
+	int i;
+
+	/* Assume this function is called for a module when the module is
+	 * loading.
+	 */
+
+	tab = btf->struct_ops_tab;
+	if (!tab) {
+		tab = kzalloc(sizeof(*tab) +
+			      sizeof(struct bpf_struct_ops *) * 4,
+			      GFP_KERNEL);
+		if (!tab)
+			return -ENOMEM;
+		tab->capacity = 4;
+		btf->struct_ops_tab = tab;
+	}
+
+	for (i = 0; i < tab->cnt; i++)
+		if (tab->ops[i] == st_ops)
+			return -EEXIST;
+
+	if (tab->cnt == tab->capacity) {
+		struct btf_struct_ops_tab *new_tab;
+
+		new_tab = krealloc(tab, sizeof(*tab) +
+				   sizeof(struct bpf_struct_ops *) *
+				   tab->capacity * 2, GFP_KERNEL);
+		if (!new_tab)
+			return -ENOMEM;
+		tab = new_tab;
+		tab->capacity *= 2;
+		btf->struct_ops_tab = tab;
+	}
+
+	btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++] = st_ops;
+
+	return 0;
+}
+
+int btf_add_struct_ops(struct bpf_struct_ops *st_ops, struct module *owner)
+{
+	struct btf *btf = btf_get_module_btf(owner);
+	int ret;
+
+	if (!btf)
+		return -ENOENT;
+
+	ret = btf_add_struct_ops_btf(st_ops, btf);
+
+	btf_put(btf);
+
+	return ret;
+}
+
+const struct bpf_struct_ops **btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
+{
+	if (!btf)
+		return NULL;
+	if (!btf->struct_ops_tab)
+		return NULL;
+
+	*ret_cnt = btf->struct_ops_tab->cnt;
+	return (const struct bpf_struct_ops **)btf->struct_ops_tab->ops;
+}
-- 
2.34.1

[RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Provide registration functions to add/remove struct_ops types.

Currently, it does linear search to find a struct_ops type. It should be
fine for now since we don't have many struct_ops types.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/bpf.h         |  9 +++++++++
 include/linux/btf.h         | 27 +++++++++++++++++++++++++++
 kernel/bpf/bpf_struct_ops.c | 11 -----------
 kernel/bpf/btf.c            |  2 +-
 4 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 30063a760b5a..67554f2f81b7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1634,6 +1634,11 @@ struct bpf_struct_ops {
 	u32 value_id;
 };
 
+struct bpf_struct_ops_mod {
+	struct module *owner;
+	struct bpf_struct_ops *st_ops;
+};
+
 #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
 #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
 const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
@@ -3205,4 +3210,8 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
 	return prog->aux->func_idx != 0;
 }
 
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod);
+#endif
+
 #endif /* _LINUX_BPF_H */
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 5fabe23aedd2..8d50e46b21bc 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -12,6 +12,8 @@
 #include <uapi/linux/bpf.h>
 
 #define BTF_TYPE_EMIT(type) ((void)(type *)0)
+#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0);	\
+		((void)(struct bpf_struct_ops_##type *)0); }
 #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
 
 /* These need to be macros, as the expressions are used in assembler input */
@@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
 bool btf_is_kernel(const struct btf *btf);
 bool btf_is_module(const struct btf *btf);
 struct module *btf_try_get_module(const struct btf *btf);
+struct btf *btf_get_module_btf(const struct module *module);
 u32 btf_nr_types(const struct btf *btf);
 bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
 			   const struct btf_member *m,
@@ -580,4 +583,28 @@ int btf_add_struct_ops(struct bpf_struct_ops *st_ops,
 const struct bpf_struct_ops **
 btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
 
+enum bpf_struct_ops_state {
+	BPF_STRUCT_OPS_STATE_INIT,
+	BPF_STRUCT_OPS_STATE_INUSE,
+	BPF_STRUCT_OPS_STATE_TOBEFREE,
+	BPF_STRUCT_OPS_STATE_READY,
+};
+
+#define BPF_STRUCT_OPS_COMMON_VALUE			\
+	refcount_t refcnt;				\
+	enum bpf_struct_ops_state state
+
+/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
+ * the map's value exposed to the userspace and its btf-type-id is
+ * stored at the map->btf_vmlinux_value_type_id.
+ *
+ */
+#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name)			\
+extern struct bpf_struct_ops bpf_##_name;			\
+								\
+struct bpf_struct_ops_##_name {					\
+	BPF_STRUCT_OPS_COMMON_VALUE;				\
+	struct _name data ____cacheline_aligned_in_smp;		\
+};
+
 #endif
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 627cf1ea840a..cd688e9033b5 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -13,17 +13,6 @@
 #include <linux/btf_ids.h>
 #include <linux/rcupdate_wait.h>
 
-enum bpf_struct_ops_state {
-	BPF_STRUCT_OPS_STATE_INIT,
-	BPF_STRUCT_OPS_STATE_INUSE,
-	BPF_STRUCT_OPS_STATE_TOBEFREE,
-	BPF_STRUCT_OPS_STATE_READY,
-};
-
-#define BPF_STRUCT_OPS_COMMON_VALUE			\
-	refcount_t refcnt;				\
-	enum bpf_struct_ops_state state
-
 struct bpf_struct_ops_value {
 	BPF_STRUCT_OPS_COMMON_VALUE;
 	char data[] ____cacheline_aligned_in_smp;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 3fb9964f8672..73d19ef99306 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct btf *btf)
 /* Returns struct btf corresponding to the struct module.
  * This function can return NULL or ERR_PTR.
  */
-static struct btf *btf_get_module_btf(const struct module *module)
+struct btf *btf_get_module_btf(const struct module *module)
 {
 #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
 	struct btf_module *btf_mod, *tmp;
-- 
2.34.1

[RFC bpf-next v3 04/11] bpf: attach a module BTF to a bpf_struct_ops

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Every struct_ops type should has an associated module BTF to provide type
information since we are going to allow modules to define and register new
struct_ops types. New types may exist only in module itself, and the kernel
BTF (vmlinux) doesn't know it at all. The attached module BTF here is going
to be used to get correct btf and resolve type IDs of a struct_ops map.

However, it doesn't use the attached module BTF until we are ready to
switch to registration function in subsequent patches.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/bpf.h         |  5 +++--
 kernel/bpf/bpf_struct_ops.c | 27 ++++++++++++++++++---------
 kernel/bpf/verifier.c       |  2 +-
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 67554f2f81b7..0776cb584b3f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1626,6 +1626,7 @@ struct bpf_struct_ops {
 	void (*unreg)(void *kdata);
 	int (*update)(void *kdata, void *old_kdata);
 	int (*validate)(void *kdata);
+	const struct btf *btf;
 	const struct btf_type *type;
 	const struct btf_type *value_type;
 	const char *name;
@@ -1641,7 +1642,7 @@ struct bpf_struct_ops_mod {
 
 #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
 #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
-const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
+const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf);
 void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
 bool bpf_struct_ops_get(const void *kdata);
 void bpf_struct_ops_put(const void *kdata);
@@ -1684,7 +1685,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
 			    union bpf_attr __user *uattr);
 #endif
 #else
-static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
+static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf)
 {
 	return NULL;
 }
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index cd688e9033b5..7c2ef53687ef 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -174,6 +174,10 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 			pr_warn("Error in init bpf_struct_ops %s\n",
 				st_ops->name);
 		} else {
+			/* XXX: We need a owner (module) here to company
+			 * with type_id and value_id.
+			 */
+			st_ops->btf = btf;
 			st_ops->type_id = type_id;
 			st_ops->type = t;
 			st_ops->value_id = value_id;
@@ -210,7 +214,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 extern struct btf *btf_vmlinux;
 
 static const struct bpf_struct_ops *
-bpf_struct_ops_find_value(u32 value_id)
+bpf_struct_ops_find_value(u32 value_id, struct btf *btf)
 {
 	unsigned int i;
 
@@ -225,7 +229,7 @@ bpf_struct_ops_find_value(u32 value_id)
 	return NULL;
 }
 
-const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
+const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf)
 {
 	unsigned int i;
 
@@ -305,7 +309,7 @@ static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map)
 	}
 }
 
-static int check_zero_holes(const struct btf_type *t, void *data)
+static int check_zero_holes(const struct btf *btf, const struct btf_type *t, void *data)
 {
 	const struct btf_member *member;
 	u32 i, moff, msize, prev_mend = 0;
@@ -317,8 +321,8 @@ static int check_zero_holes(const struct btf_type *t, void *data)
 		    memchr_inv(data + prev_mend, 0, moff - prev_mend))
 			return -EINVAL;
 
-		mtype = btf_type_by_id(btf_vmlinux, member->type);
-		mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
+		mtype = btf_type_by_id(btf, member->type);
+		mtype = btf_resolve_size(btf, mtype, &msize);
 		if (IS_ERR(mtype))
 			return PTR_ERR(mtype);
 		prev_mend = moff + msize;
@@ -371,7 +375,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	const struct bpf_struct_ops *st_ops = st_map->st_ops;
 	struct bpf_struct_ops_value *uvalue, *kvalue;
 	const struct btf_member *member;
-	const struct btf_type *t = st_ops->type;
+	const struct btf_type *t;
 	struct bpf_tramp_links *tlinks;
 	void *udata, *kdata;
 	int prog_fd, err;
@@ -381,15 +385,20 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	if (flags)
 		return -EINVAL;
 
+	if (!st_ops)
+		return -EINVAL;
+
+	t = st_ops->type;
+
 	if (*(u32 *)key != 0)
 		return -E2BIG;
 
-	err = check_zero_holes(st_ops->value_type, value);
+	err = check_zero_holes(st_ops->btf, st_ops->value_type, value);
 	if (err)
 		return err;
 
 	uvalue = value;
-	err = check_zero_holes(t, uvalue->data);
+	err = check_zero_holes(st_ops->btf, t, uvalue->data);
 	if (err)
 		return err;
 
@@ -660,7 +669,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 	struct bpf_map *map;
 	int ret;
 
-	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
+	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
 	if (!st_ops)
 		return ERR_PTR(-ENOTSUPP);
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a7178ecf676d..99b45501951c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19631,7 +19631,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	}
 
 	btf_id = prog->aux->attach_btf_id;
-	st_ops = bpf_struct_ops_find(btf_id);
+	st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
 	if (!st_ops) {
 		verbose(env, "attach_btf_id %u is not a supported struct\n",
 			btf_id);
-- 
2.34.1

[RFC bpf-next v3 05/11] bpf: hold module for bpf_struct_ops_map.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Ensure a module doesn't go away when a struct_ops object is still alive,
being a struct_ops type that is registered by the module.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/bpf.h         | 1 +
 kernel/bpf/bpf_struct_ops.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 0776cb584b3f..faaec20156f1 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
 	int (*update)(void *kdata, void *old_kdata);
 	int (*validate)(void *kdata);
 	const struct btf *btf;
+	struct module *owner;
 	const struct btf_type *type;
 	const struct btf_type *value_type;
 	const char *name;
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 7c2ef53687ef..ef8a1edec891 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -632,6 +632,8 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
 
 static void bpf_struct_ops_map_free(struct bpf_map *map)
 {
+	struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
+
 	/* The struct_ops's function may switch to another struct_ops.
 	 *
 	 * For example, bpf_tcp_cc_x->init() may switch to
@@ -649,6 +651,7 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
 	 */
 	synchronize_rcu_mult(call_rcu, call_rcu_tasks);
 
+	module_put(st_map->st_ops->owner);
 	__bpf_struct_ops_map_free(map);
 }
 
@@ -673,6 +676,9 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 	if (!st_ops)
 		return ERR_PTR(-ENOTSUPP);
 
+	if (!try_module_get(st_ops->owner))
+		return ERR_PTR(-EINVAL);
+
 	vt = st_ops->value_type;
 	if (attr->value_size != vt->size)
 		return ERR_PTR(-EINVAL);
-- 
2.34.1

[RFC bpf-next v3 06/11] bpf: validate value_type

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

A value_type should has three members; refcnt, state, and data.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/bpf_struct_ops.c | 75 +++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index ef8a1edec891..fb684d2ee99d 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -99,6 +99,79 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
 
 static const struct btf_type *module_type;
 
+static bool check_value_member(struct btf *btf,
+			       const struct btf_member *member,
+			       int index,
+			       const char *value_name,
+			       const char *name, const char *type_name,
+			       u16 kind)
+{
+	const char *mname, *mtname;
+	const struct btf_type *mt;
+	s32 mtype_id;
+
+	mname = btf_name_by_offset(btf, member->name_off);
+	if (!*mname) {
+		pr_warn("The member %d of %s should have a name\n",
+			index, value_name);
+		return false;
+	}
+	if (strcmp(mname, name)) {
+		pr_warn("The member %d of %s should be refcnt\n",
+			index, value_name);
+		return false;
+	}
+	mtype_id = member->type;
+	mt = btf_type_by_id(btf, mtype_id);
+	mtname = btf_name_by_offset(btf, mt->name_off);
+	if (!*mtname) {
+		pr_warn("The type of the member %d of %s should have a name\n",
+			index, value_name);
+		return false;
+	}
+	if (strcmp(mtname, type_name)) {
+		pr_warn("The type of the member %d of %s should be refcount_t\n",
+			index, value_name);
+		return false;
+	}
+	if (btf_kind(mt) != kind) {
+		pr_warn("The type of the member %d of %s should be %d\n",
+			index, value_name, btf_kind(mt));
+		return false;
+	}
+
+	return true;
+}
+
+static bool is_valid_value_type(struct btf *btf, s32 value_id,
+				const char *type_name, const char *value_name)
+{
+	const struct btf_member *member;
+	const struct btf_type *vt;
+
+	vt = btf_type_by_id(btf, value_id);
+	if (btf_vlen(vt) != 3) {
+		pr_warn("The number of %s's members should be 3, but we get %d\n",
+			value_name, btf_vlen(vt));
+		return false;
+	}
+	member = btf_type_member(vt);
+	if (!check_value_member(btf, member, 0, value_name,
+				"refcnt", "refcount_t", BTF_KIND_TYPEDEF))
+		return false;
+	member++;
+	if (!check_value_member(btf, member, 1, value_name,
+				"state", "bpf_struct_ops_state",
+				BTF_KIND_ENUM))
+		return false;
+	member++;
+	if (!check_value_member(btf, member, 2, value_name,
+				"data", type_name, BTF_KIND_STRUCT))
+		return false;
+
+	return true;
+}
+
 static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 				    struct btf *btf,
 				    struct bpf_verifier_log *log)
@@ -125,6 +198,8 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 			value_name);
 		return;
 	}
+	if (!is_valid_value_type(btf, value_id, st_ops->name, value_name))
+		return;
 
 	type_id = btf_find_by_name_kind(btf, st_ops->name,
 					BTF_KIND_STRUCT);
-- 
2.34.1

[RFC bpf-next v3 07/11] bpf, net: switch to storing struct_ops in btf

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Use struct_ops registered and stored in module btf instead of static ones.

Both bpf_dummy_ops and bpf_tcp_ca switches to calling the registration
function instead of listed in bpf_struct_ops_types.h.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/bpf_struct_ops.c       | 114 ++++++++++++++++++------------
 kernel/bpf/bpf_struct_ops_types.h |  12 ----
 net/bpf/bpf_dummy_struct_ops.c    |  12 +++-
 net/ipv4/bpf_tcp_ca.c             |  20 +++++-
 4 files changed, 94 insertions(+), 64 deletions(-)
 delete mode 100644 kernel/bpf/bpf_struct_ops_types.h

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index fb684d2ee99d..8b5c859377e9 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -59,35 +59,6 @@ static DEFINE_MUTEX(update_mutex);
 #define VALUE_PREFIX "bpf_struct_ops_"
 #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
 
-/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
- * the map's value exposed to the userspace and its btf-type-id is
- * stored at the map->btf_vmlinux_value_type_id.
- *
- */
-#define BPF_STRUCT_OPS_TYPE(_name)				\
-extern struct bpf_struct_ops bpf_##_name;			\
-								\
-struct bpf_struct_ops_##_name {						\
-	BPF_STRUCT_OPS_COMMON_VALUE;				\
-	struct _name data ____cacheline_aligned_in_smp;		\
-};
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-
-enum {
-#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-	__NR_BPF_STRUCT_OPS_TYPE,
-};
-
-static struct bpf_struct_ops * const bpf_struct_ops[] = {
-#define BPF_STRUCT_OPS_TYPE(_name)				\
-	[BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-};
-
 const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
 };
 
@@ -264,14 +235,11 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 
 void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 {
-	struct bpf_struct_ops *st_ops;
+#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
+	extern struct bpf_struct_ops_mod bpf_testmod_struct_ops;
+	int ret;
+#endif
 	s32 module_id;
-	u32 i;
-
-	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
-#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
 
 	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
 	if (module_id < 0) {
@@ -280,43 +248,95 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 	}
 	module_type = btf_type_by_id(btf, module_id);
 
-	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
-		st_ops = bpf_struct_ops[i];
-		bpf_struct_ops_init_one(st_ops, btf, log);
+#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
+	ret = register_bpf_struct_ops(&bpf_testmod_struct_ops);
+	if (ret)
+		pr_warn("Cannot register bpf_testmod_struct_ops\n");
+#endif
+}
+
+int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
+{
+	struct bpf_struct_ops *st_ops = mod->st_ops;
+	struct bpf_verifier_log *log;
+	struct btf *btf;
+	int err;
+
+	if (mod->st_ops == NULL ||
+	    mod->owner == NULL)
+		return -EINVAL;
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+	if (!log) {
+		err = -ENOMEM;
+		goto errout;
+	}
+
+	log->level = BPF_LOG_KERNEL;
+
+	btf = btf_get_module_btf(mod->owner);
+	if (!btf) {
+		err = -EINVAL;
+		goto errout;
 	}
+
+	bpf_struct_ops_init_one(st_ops, btf, log);
+
+	btf_put(btf);
+
+	st_ops->owner = mod->owner;
+	err = btf_add_struct_ops(st_ops, st_ops->owner);
+
+errout:
+	kfree(log);
+
+	return err;
 }
+EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
 
 extern struct btf *btf_vmlinux;
 
 static const struct bpf_struct_ops *
 bpf_struct_ops_find_value(u32 value_id, struct btf *btf)
 {
+	const struct bpf_struct_ops *st_ops = NULL;
+	const struct bpf_struct_ops **st_ops_list;
 	unsigned int i;
+	u32 cnt = 0;
 
 	if (!value_id || !btf_vmlinux)
 		return NULL;
 
-	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
-		if (bpf_struct_ops[i]->value_id == value_id)
-			return bpf_struct_ops[i];
+	st_ops_list = btf_get_struct_ops(btf, &cnt);
+	for (i = 0; i < cnt; i++) {
+		if (st_ops_list[i]->value_id == value_id) {
+			st_ops = st_ops_list[i];
+			break;
+		}
 	}
 
-	return NULL;
+	return st_ops;
 }
 
 const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf)
 {
+	const struct bpf_struct_ops *st_ops = NULL;
+	const struct bpf_struct_ops **st_ops_list;
 	unsigned int i;
+	u32 cnt;
 
 	if (!type_id || !btf_vmlinux)
 		return NULL;
 
-	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
-		if (bpf_struct_ops[i]->type_id == type_id)
-			return bpf_struct_ops[i];
+	st_ops_list = btf_get_struct_ops(btf, &cnt);
+	for (i = 0; i < cnt; i++) {
+		if (st_ops_list[i]->type_id == type_id) {
+			st_ops = st_ops_list[i];
+			break;
+		}
 	}
 
-	return NULL;
+	return st_ops;
 }
 
 static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,
diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
deleted file mode 100644
index 5678a9ddf817..000000000000
--- a/kernel/bpf/bpf_struct_ops_types.h
+++ /dev/null
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* internal file - do not include directly */
-
-#ifdef CONFIG_BPF_JIT
-#ifdef CONFIG_NET
-BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
-#endif
-#ifdef CONFIG_INET
-#include <net/tcp.h>
-BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
-#endif
-#endif
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 5918d1b32e19..9cb982c67c4c 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -7,7 +7,7 @@
 #include <linux/bpf.h>
 #include <linux/btf.h>
 
-extern struct bpf_struct_ops bpf_bpf_dummy_ops;
+static struct bpf_struct_ops bpf_bpf_dummy_ops;
 
 /* A common type for test_N with return value in bpf_dummy_ops */
 typedef int (*dummy_ops_test_ret_fn)(struct bpf_dummy_ops_state *state, ...);
@@ -218,9 +218,12 @@ static int bpf_dummy_reg(void *kdata)
 
 static void bpf_dummy_unreg(void *kdata)
 {
+	BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops);
 }
 
-struct bpf_struct_ops bpf_bpf_dummy_ops = {
+DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_dummy_ops);
+
+static struct bpf_struct_ops bpf_bpf_dummy_ops = {
 	.verifier_ops = &bpf_dummy_verifier_ops,
 	.init = bpf_dummy_init,
 	.check_member = bpf_dummy_ops_check_member,
@@ -229,3 +232,8 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = {
 	.unreg = bpf_dummy_unreg,
 	.name = "bpf_dummy_ops",
 };
+
+struct bpf_struct_ops_mod bpf_testmod_struct_ops = {
+	.st_ops = &bpf_bpf_dummy_ops,
+	.owner = THIS_MODULE,
+};
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 39dcccf0f174..9947323f3e22 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -12,7 +12,7 @@
 #include <net/bpf_sk_storage.h>
 
 /* "extern" is to avoid sparse warning.  It is only used in bpf_struct_ops.c. */
-extern struct bpf_struct_ops bpf_tcp_congestion_ops;
+static struct bpf_struct_ops bpf_tcp_congestion_ops;
 
 static u32 unsupported_ops[] = {
 	offsetof(struct tcp_congestion_ops, get_info),
@@ -271,7 +271,9 @@ static int bpf_tcp_ca_validate(void *kdata)
 	return tcp_validate_congestion_control(kdata);
 }
 
-struct bpf_struct_ops bpf_tcp_congestion_ops = {
+DEFINE_STRUCT_OPS_VALUE_TYPE(tcp_congestion_ops);
+
+static struct bpf_struct_ops bpf_tcp_congestion_ops = {
 	.verifier_ops = &bpf_tcp_ca_verifier_ops,
 	.reg = bpf_tcp_ca_reg,
 	.unreg = bpf_tcp_ca_unreg,
@@ -283,8 +285,20 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = {
 	.name = "tcp_congestion_ops",
 };
 
+static struct bpf_struct_ops_mod bpf_tcp_ca_ops_mod = {
+	.st_ops = &bpf_tcp_congestion_ops,
+	.owner = THIS_MODULE,
+};
+
 static int __init bpf_tcp_ca_kfunc_init(void)
 {
-	return register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set);
+	int ret;
+
+	BTF_STRUCT_OPS_TYPE_EMIT(tcp_congestion_ops);
+
+	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set);
+	ret = ret ?: register_bpf_struct_ops(&bpf_tcp_ca_ops_mod);
+
+	return ret;
 }
 late_initcall(bpf_tcp_ca_kfunc_init);
-- 
2.34.1

[RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

The type info of a struct_ops type may be in a module.  So, we need to know
which module BTF to look for type information.  The later patches will make
libbpf to attach module BTFs to programs. This patch passes attached BTF
from syscall to bpf_struct_ops subsystem to make sure attached BTF is
available when the bpf_struct_ops subsystem is ready to use it.

bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along
with the bpf_attr loading the program. attach_btf is used to find the btf
type of attach_btf_id. attach_btf_id is used to identify the traced
function for a trace program.  For struct_ops programs, it is used to
identify the struct_ops type of the struct_ops object a program attached
to.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/uapi/linux/bpf.h       |  4 ++++
 kernel/bpf/bpf_struct_ops.c    | 12 +++++++++++-
 kernel/bpf/syscall.c           |  2 +-
 kernel/bpf/verifier.c          |  4 +++-
 tools/include/uapi/linux/bpf.h |  4 ++++
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 73b155e52204..178d6fa45fa0 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1390,6 +1390,10 @@ union bpf_attr {
 		 * to using 5 hash functions).
 		 */
 		__u64	map_extra;
+
+		__u32   mod_btf_fd;	/* fd pointing to a BTF type data
+					 * for btf_vmlinux_value_type_id.
+					 */
 	};
 
 	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 8b5c859377e9..d5600d9ad302 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -765,9 +765,19 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 	struct bpf_struct_ops_map *st_map;
 	const struct btf_type *t, *vt;
 	struct bpf_map *map;
+	struct btf *btf;
 	int ret;
 
-	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
+	/* XXX: We need a module name or ID to find a BTF type. */
+	/* XXX: should use btf from attr->btf_fd */
+	if (attr->mod_btf_fd) {
+		btf = btf_get_by_fd(attr->mod_btf_fd);
+		if (IS_ERR(btf))
+			return ERR_PTR(PTR_ERR(btf));
+	} else {
+		btf = btf_vmlinux;
+	}
+	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf);
 	if (!st_ops)
 		return ERR_PTR(-ENOTSUPP);
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 85c1d908f70f..fed3870fec7a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1097,7 +1097,7 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
 	return ret;
 }
 
-#define BPF_MAP_CREATE_LAST_FIELD map_extra
+#define BPF_MAP_CREATE_LAST_FIELD mod_btf_fd
 /* called via syscall */
 static int map_create(union bpf_attr *attr)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 99b45501951c..11f85dbc911b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19623,6 +19623,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	const struct btf_member *member;
 	struct bpf_prog *prog = env->prog;
 	u32 btf_id, member_idx;
+	struct btf *btf;
 	const char *mname;
 
 	if (!prog->gpl_compatible) {
@@ -19630,8 +19631,9 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 		return -EINVAL;
 	}
 
+	btf = prog->aux->attach_btf;
 	btf_id = prog->aux->attach_btf_id;
-	st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
+	st_ops = bpf_struct_ops_find(btf_id, btf);
 	if (!st_ops) {
 		verbose(env, "attach_btf_id %u is not a supported struct\n",
 			btf_id);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 73b155e52204..178d6fa45fa0 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1390,6 +1390,10 @@ union bpf_attr {
 		 * to using 5 hash functions).
 		 */
 		__u64	map_extra;
+
+		__u32   mod_btf_fd;	/* fd pointing to a BTF type data
+					 * for btf_vmlinux_value_type_id.
+					 */
 	};
 
 	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
-- 
2.34.1

[RFC bpf-next v3 09/11] libbpf: Find correct module BTFs for struct_ops maps and progs.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Find module BTFs for struct_ops maps and progs and pass them to the kernel.
It ensures the kernel resolve type IDs from correct module BTFs.

For the map of a struct_ops object, mod_btf is added to bpf_map to keep a
reference to the module BTF. It's FD is passed to the kernel as mod_btf_fd
when it is created.

For a prog attaching to a struct_ops object, attach_btf_obj_fd of bpf_prog
is the FD pointing to a module BTF in the kernel.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/lib/bpf/bpf.c    |   3 +-
 tools/lib/bpf/bpf.h    |   4 +-
 tools/lib/bpf/libbpf.c | 121 ++++++++++++++++++++++++-----------------
 3 files changed, 75 insertions(+), 53 deletions(-)

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index b0f1913763a3..df4b7570ad92 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -169,7 +169,7 @@ int bpf_map_create(enum bpf_map_type map_type,
 		   __u32 max_entries,
 		   const struct bpf_map_create_opts *opts)
 {
-	const size_t attr_sz = offsetofend(union bpf_attr, map_extra);
+	const size_t attr_sz = offsetofend(union bpf_attr, mod_btf_fd);
 	union bpf_attr attr;
 	int fd;
 
@@ -191,6 +191,7 @@ int bpf_map_create(enum bpf_map_type map_type,
 	attr.btf_key_type_id = OPTS_GET(opts, btf_key_type_id, 0);
 	attr.btf_value_type_id = OPTS_GET(opts, btf_value_type_id, 0);
 	attr.btf_vmlinux_value_type_id = OPTS_GET(opts, btf_vmlinux_value_type_id, 0);
+	attr.mod_btf_fd = OPTS_GET(opts, mod_btf_fd, 0);
 
 	attr.inner_map_fd = OPTS_GET(opts, inner_map_fd, 0);
 	attr.map_flags = OPTS_GET(opts, map_flags, 0);
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 74c2887cfd24..d18f75b0ccc9 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -51,8 +51,10 @@ struct bpf_map_create_opts {
 
 	__u32 numa_node;
 	__u32 map_ifindex;
+
+	__u32 mod_btf_fd;
 };
-#define bpf_map_create_opts__last_field map_ifindex
+#define bpf_map_create_opts__last_field mod_btf_fd
 
 LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
 			      const char *map_name,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 3a6108e3238b..df6ba5494adb 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -519,6 +519,7 @@ struct bpf_map {
 	struct bpf_map_def def;
 	__u32 numa_node;
 	__u32 btf_var_idx;
+	struct module_btf *mod_btf;
 	__u32 btf_key_type_id;
 	__u32 btf_value_type_id;
 	__u32 btf_vmlinux_value_type_id;
@@ -893,6 +894,42 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
 	return 0;
 }
 
+static int load_module_btfs(struct bpf_object *obj);
+
+static int find_kern_btf_id(struct bpf_object *obj, const char *kern_name,
+			    __u16 kind, struct btf **res_btf,
+			    struct module_btf **res_mod_btf)
+{
+	struct module_btf *mod_btf;
+	struct btf *btf;
+	int i, id, err;
+
+	btf = obj->btf_vmlinux;
+	mod_btf = NULL;
+	id = btf__find_by_name_kind(btf, kern_name, kind);
+
+	if (id == -ENOENT) {
+		err = load_module_btfs(obj);
+		if (err)
+			return err;
+
+		for (i = 0; i < obj->btf_module_cnt; i++) {
+			/* we assume module_btf's BTF FD is always >0 */
+			mod_btf = &obj->btf_modules[i];
+			btf = mod_btf->btf;
+			id = btf__find_by_name_kind_own(btf, kern_name, kind);
+			if (id != -ENOENT)
+				break;
+		}
+	}
+	if (id <= 0)
+		return -ESRCH;
+
+	*res_btf = btf;
+	*res_mod_btf = mod_btf;
+	return id;
+}
+
 static const struct btf_member *
 find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
 {
@@ -927,17 +964,23 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
 				   const char *name, __u32 kind);
 
 static int
-find_struct_ops_kern_types(const struct btf *btf, const char *tname,
+find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
+			   struct module_btf **mod_btf,
 			   const struct btf_type **type, __u32 *type_id,
 			   const struct btf_type **vtype, __u32 *vtype_id,
 			   const struct btf_member **data_member)
 {
 	const struct btf_type *kern_type, *kern_vtype;
 	const struct btf_member *kern_data_member;
+	struct btf *btf;
 	__s32 kern_vtype_id, kern_type_id;
 	__u32 i;
 
-	kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);
+	/* XXX: should search module BTFs as well. We need module name here
+	 * to locate a correct BTF type.
+	 */
+	kern_type_id = find_kern_btf_id(obj, tname, BTF_KIND_STRUCT,
+					&btf, mod_btf);
 	if (kern_type_id < 0) {
 		pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
 			tname);
@@ -992,13 +1035,15 @@ static bool bpf_map__is_struct_ops(const struct bpf_map *map)
 
 /* Init the map's fields that depend on kern_btf */
 static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
-					 const struct btf *btf,
-					 const struct btf *kern_btf)
+					 struct bpf_object *obj)
 {
 	const struct btf_member *member, *kern_member, *kern_data_member;
 	const struct btf_type *type, *kern_type, *kern_vtype;
 	__u32 i, kern_type_id, kern_vtype_id, kern_data_off;
 	struct bpf_struct_ops *st_ops;
+	const struct btf *kern_btf;
+	struct module_btf *mod_btf;
+	const struct btf *btf = obj->btf;
 	void *data, *kern_data;
 	const char *tname;
 	int err;
@@ -1006,16 +1051,19 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
 	st_ops = map->st_ops;
 	type = st_ops->type;
 	tname = st_ops->tname;
-	err = find_struct_ops_kern_types(kern_btf, tname,
+	err = find_struct_ops_kern_types(obj, tname, &mod_btf,
 					 &kern_type, &kern_type_id,
 					 &kern_vtype, &kern_vtype_id,
 					 &kern_data_member);
 	if (err)
 		return err;
 
+	kern_btf = mod_btf ? mod_btf->btf : obj->btf_vmlinux;
+
 	pr_debug("struct_ops init_kern %s: type_id:%u kern_type_id:%u kern_vtype_id:%u\n",
 		 map->name, st_ops->type_id, kern_type_id, kern_vtype_id);
 
+	map->mod_btf = mod_btf;
 	map->def.value_size = kern_vtype->size;
 	map->btf_vmlinux_value_type_id = kern_vtype_id;
 
@@ -1091,6 +1139,9 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
 				return -ENOTSUP;
 			}
 
+			/* XXX: attach_btf_obj_fd is needed as well */
+			if (mod_btf)
+				prog->attach_btf_obj_fd = mod_btf->fd;
 			prog->attach_btf_id = kern_type_id;
 			prog->expected_attach_type = kern_member_idx;
 
@@ -1133,8 +1184,8 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
 		if (!bpf_map__is_struct_ops(map))
 			continue;
 
-		err = bpf_map__init_kern_struct_ops(map, obj->btf,
-						    obj->btf_vmlinux);
+		/* XXX: should be a module btf if not vmlinux */
+		err = bpf_map__init_kern_struct_ops(map, obj);
 		if (err)
 			return err;
 	}
@@ -5193,8 +5244,10 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
 	create_attr.numa_node = map->numa_node;
 	create_attr.map_extra = map->map_extra;
 
-	if (bpf_map__is_struct_ops(map))
+	if (bpf_map__is_struct_ops(map)) {
 		create_attr.btf_vmlinux_value_type_id = map->btf_vmlinux_value_type_id;
+		create_attr.mod_btf_fd = map->mod_btf ? map->mod_btf->fd : 0;
+	}
 
 	if (obj->btf && btf__fd(obj->btf) >= 0) {
 		create_attr.btf_fd = btf__fd(obj->btf);
@@ -7700,40 +7753,6 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
 	return libbpf_kallsyms_parse(kallsyms_cb, obj);
 }
 
-static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
-			    __u16 kind, struct btf **res_btf,
-			    struct module_btf **res_mod_btf)
-{
-	struct module_btf *mod_btf;
-	struct btf *btf;
-	int i, id, err;
-
-	btf = obj->btf_vmlinux;
-	mod_btf = NULL;
-	id = btf__find_by_name_kind(btf, ksym_name, kind);
-
-	if (id == -ENOENT) {
-		err = load_module_btfs(obj);
-		if (err)
-			return err;
-
-		for (i = 0; i < obj->btf_module_cnt; i++) {
-			/* we assume module_btf's BTF FD is always >0 */
-			mod_btf = &obj->btf_modules[i];
-			btf = mod_btf->btf;
-			id = btf__find_by_name_kind_own(btf, ksym_name, kind);
-			if (id != -ENOENT)
-				break;
-		}
-	}
-	if (id <= 0)
-		return -ESRCH;
-
-	*res_btf = btf;
-	*res_mod_btf = mod_btf;
-	return id;
-}
-
 static int bpf_object__resolve_ksym_var_btf_id(struct bpf_object *obj,
 					       struct extern_desc *ext)
 {
@@ -7744,7 +7763,7 @@ static int bpf_object__resolve_ksym_var_btf_id(struct bpf_object *obj,
 	struct btf *btf = NULL;
 	int id, err;
 
-	id = find_ksym_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
+	id = find_kern_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
 	if (id < 0) {
 		if (id == -ESRCH && ext->is_weak)
 			return 0;
@@ -7798,7 +7817,7 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
 
 	local_func_proto_id = ext->ksym.type_id;
 
-	kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
+	kfunc_id = find_kern_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
 				    &mod_btf);
 	if (kfunc_id < 0) {
 		if (kfunc_id == -ESRCH && ext->is_weak)
@@ -9464,9 +9483,9 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
 	return err;
 }
 
-static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
-			      enum bpf_attach_type attach_type,
-			      int *btf_obj_fd, int *btf_type_id)
+static int find_kernel_attach_btf_id(struct bpf_object *obj, const char *attach_name,
+				     enum bpf_attach_type attach_type,
+				     int *btf_obj_fd, int *btf_type_id)
 {
 	int ret, i;
 
@@ -9531,7 +9550,7 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, const char *attac
 		*btf_obj_fd = 0;
 		*btf_type_id = 1;
 	} else {
-		err = find_kernel_btf_id(prog->obj, attach_name, attach_type, btf_obj_fd, btf_type_id);
+		err = find_kernel_attach_btf_id(prog->obj, attach_name, attach_type, btf_obj_fd, btf_type_id);
 	}
 	if (err) {
 		pr_warn("prog '%s': failed to find kernel BTF type ID of '%s': %d\n",
@@ -12945,9 +12964,9 @@ int bpf_program__set_attach_target(struct bpf_program *prog,
 		err = bpf_object__load_vmlinux_btf(prog->obj, true);
 		if (err)
 			return libbpf_err(err);
-		err = find_kernel_btf_id(prog->obj, attach_func_name,
-					 prog->expected_attach_type,
-					 &btf_obj_fd, &btf_id);
+		err = find_kernel_attach_btf_id(prog->obj, attach_func_name,
+						prog->expected_attach_type,
+						&btf_obj_fd, &btf_id);
 		if (err)
 			return libbpf_err(err);
 	}
-- 
2.34.1

[RFC bpf-next v3 10/11] bpf: export btf_ctx_access to modules.

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

btf_ctx_access() is needed by module to call bpf_tracing_btf_ctx_access().
btf_ctx_access()) is helpful to implement validation functions that
validates ctx accesses.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/btf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 73d19ef99306..c970a04b9142 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6141,6 +6141,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		__btf_name_by_offset(btf, t->name_off));
 	return true;
 }
+EXPORT_SYMBOL_GPL(btf_ctx_access);
 
 enum bpf_struct_walk_result {
 	/* < 0 error */
-- 
2.34.1

[RFC bpf-next v3 11/11] selftests/bpf: test case for register_bpf_struct_ops().

[thinker.li]

From: Kui-Feng Lee <thinker.li@gmail.com>

Register a new struct_ops type bpf_testmod_ops from the bpf_testmod module.
test_2 of bpf_testmod_ops will be called by the bpf_testmod module.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 63 +++++++++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  5 ++
 .../bpf/prog_tests/test_struct_ops_module.c   | 43 +++++++++++++
 .../selftests/bpf/progs/struct_ops_module.c   | 30 +++++++++
 4 files changed, 141 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_module.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index cefc5dd72573..3d208dbd23e4 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2020 Facebook */
+#include <linux/bpf.h>
 #include <linux/btf.h>
 #include <linux/btf_ids.h>
 #include <linux/error-injection.h>
@@ -517,11 +518,70 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_offset)
 BTF_SET8_END(bpf_testmod_check_kfunc_ids)
 
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+
+DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_testmod_ops);
+
+static int bpf_testmod_ops_init(struct btf *btf)
+{
+	return 0;
+}
+
+static bool bpf_testmod_ops_is_valid_access(int off, int size,
+					    enum bpf_access_type type,
+					    const struct bpf_prog *prog,
+					    struct bpf_insn_access_aux *info)
+{
+	return bpf_tracing_btf_ctx_access(off, size, type, prog, info);
+}
+
+static int bpf_testmod_ops_init_member(const struct btf_type *t,
+				       const struct btf_member *member,
+				       void *kdata, const void *udata)
+{
+	return 0;
+}
+
 static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
 	.owner = THIS_MODULE,
 	.set   = &bpf_testmod_check_kfunc_ids,
 };
 
+static const struct bpf_verifier_ops bpf_testmod_verifier_ops = {
+	.is_valid_access = bpf_testmod_ops_is_valid_access,
+};
+
+static int bpf_dummy_reg(void *kdata)
+{
+	struct bpf_testmod_ops *ops = kdata;
+	int r;
+
+	BTF_STRUCT_OPS_TYPE_EMIT(bpf_testmod_ops);
+	r = ops->test_2(4, 3);
+	printk(KERN_CRIT "bpf_dummy_reg: test_2 %d\n", r);
+	return 0;
+}
+
+static void bpf_dummy_unreg(void *kdata)
+{
+}
+
+struct bpf_struct_ops bpf_bpf_testmod_ops = {
+	.verifier_ops = &bpf_testmod_verifier_ops,
+	.init = bpf_testmod_ops_init,
+	.init_member = bpf_testmod_ops_init_member,
+	.reg = bpf_dummy_reg,
+	.unreg = bpf_dummy_unreg,
+	.name = "bpf_testmod_ops",
+};
+
+static struct bpf_struct_ops_mod bpf_testmod_struct_ops = {
+	.owner = THIS_MODULE,
+	.st_ops = &bpf_bpf_testmod_ops,
+};
+
+#endif /* CONFIG_DEBUG_INFO_BTF_MODULES */
+
 extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
@@ -532,6 +592,9 @@ static int bpf_testmod_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+	ret = ret ?: register_bpf_struct_ops(&bpf_testmod_struct_ops);
+#endif
 	if (ret < 0)
 		return ret;
 	if (bpf_fentry_test1(0) < 0)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index f32793efe095..ca5435751c79 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -28,4 +28,9 @@ struct bpf_iter_testmod_seq {
 	int cnt;
 };
 
+struct bpf_testmod_ops {
+	int (*test_1)(void);
+	int (*test_2)(int a, int b);
+};
+
 #endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
new file mode 100644
index 000000000000..8219a477b6d6
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+
+#include "struct_ops_module.skel.h"
+#include "testing_helpers.h"
+
+static void test_regular_load(void)
+{
+	struct struct_ops_module *skel;
+	struct bpf_link *link;
+	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+	int err;
+
+	printf("test_regular_load\n");
+	skel = struct_ops_module__open_opts(&opts);
+	if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+		return;
+	err = struct_ops_module__load(skel);
+	if (!ASSERT_OK(err, "struct_ops_module_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+	ASSERT_OK_PTR(link, "attach_test_mod_1");
+
+	ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
+
+	bpf_link__destroy(link);
+
+	struct_ops_module__destroy(skel);
+
+	/* Wait for the map to be freed, or we may fail to unload
+	 * bpf_testmod.
+	 */
+	sleep(1);
+}
+
+void serial_test_struct_ops_module(void)
+{
+	if (test__start_subtest("regular_load"))
+		test_regular_load();
+}
+
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
new file mode 100644
index 000000000000..cb305d04342f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+int test_2_result = 0;
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1)
+{
+	return 0xdeadbeef;
+}
+
+SEC("struct_ops/test_2")
+int BPF_PROG(test_2, int a, int b)
+{
+	test_2_result = a + b;
+	return a + b;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+	.test_1 = (void *)test_1,
+	.test_2 = (void *)test_2,
+};
+
-- 
2.34.1

Re: [RFC bpf-next v3 02/11] bpf: add struct_ops_tab to btf.

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> struct_ops_tab will be used to restore registered struct_ops.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/btf.h |  9 +++++
>   kernel/bpf/btf.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 93 insertions(+)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 928113a80a95..5fabe23aedd2 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -571,4 +571,13 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type
>   	return btf_type_is_struct(t);
>   }
>   
> +struct bpf_struct_ops;
> +
> +int btf_add_struct_ops_btf(struct bpf_struct_ops *st_ops,
> +			   struct btf *btf);
> +int btf_add_struct_ops(struct bpf_struct_ops *st_ops,
> +		       struct module *owner);
> +const struct bpf_struct_ops **
> +btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
> +
>   #endif
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index f93e835d90af..3fb9964f8672 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -241,6 +241,12 @@ struct btf_id_dtor_kfunc_tab {
>   	struct btf_id_dtor_kfunc dtors[];
>   };
>   
> +struct btf_struct_ops_tab {
> +	u32 cnt;
> +	u32 capacity;
> +	struct bpf_struct_ops *ops[];
> +};
> +
>   struct btf {
>   	void *data;
>   	struct btf_type **types;
> @@ -258,6 +264,7 @@ struct btf {
>   	struct btf_kfunc_set_tab *kfunc_set_tab;
>   	struct btf_id_dtor_kfunc_tab *dtor_kfunc_tab;
>   	struct btf_struct_metas *struct_meta_tab;
> +	struct btf_struct_ops_tab *struct_ops_tab;
>   
>   	/* split BTF support */
>   	struct btf *base_btf;
> @@ -1688,11 +1695,20 @@ static void btf_free_struct_meta_tab(struct btf *btf)
>   	btf->struct_meta_tab = NULL;
>   }
>   
> +static void btf_free_struct_ops_tab(struct btf *btf)
> +{
> +	struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
> +
> +	kfree(tab);
> +	btf->struct_ops_tab = NULL;
> +}
> +
>   static void btf_free(struct btf *btf)
>   {
>   	btf_free_struct_meta_tab(btf);
>   	btf_free_dtor_kfunc_tab(btf);
>   	btf_free_kfunc_set_tab(btf);
> +	btf_free_struct_ops_tab(btf);
>   	kvfree(btf->types);
>   	kvfree(btf->resolved_sizes);
>   	kvfree(btf->resolved_ids);
> @@ -8601,3 +8617,71 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
>   
>   	return !strncmp(reg_name, arg_name, cmp_len);
>   }
> +
> +int btf_add_struct_ops_btf(struct bpf_struct_ops *st_ops, struct btf *btf)

A few nits.

'struct btf *btf' as the first argument, to be consistent with other similar btf 
functions.

This new function is not used outside of this file, so at least static. I would 
just fold this into btf_add_struct_ops() below which currently is mostly empty 
other than a btf_get/put.

> +{
> +	struct btf_struct_ops_tab *tab;
> +	int i;
> +
> +	/* Assume this function is called for a module when the module is
> +	 * loading.
> +	 */
> +
> +	tab = btf->struct_ops_tab;
> +	if (!tab) {
> +		tab = kzalloc(sizeof(*tab) +
> +			      sizeof(struct bpf_struct_ops *) * 4,
> +			      GFP_KERNEL);

nit. offsetof(struct bpf_struct_ops_tab, ops[4]).

> +		if (!tab)
> +			return -ENOMEM;
> +		tab->capacity = 4;
> +		btf->struct_ops_tab = tab;
> +	}
> +
> +	for (i = 0; i < tab->cnt; i++)
> +		if (tab->ops[i] == st_ops)
> +			return -EEXIST;
> +
> +	if (tab->cnt == tab->capacity) {
> +		struct btf_struct_ops_tab *new_tab;
> +
> +		new_tab = krealloc(tab, sizeof(*tab) +
> +				   sizeof(struct bpf_struct_ops *) *
> +				   tab->capacity * 2, GFP_KERNEL);
> +		if (!new_tab)
> +			return -ENOMEM;
> +		tab = new_tab;
> +		tab->capacity *= 2;
> +		btf->struct_ops_tab = tab;
> +	}
> +
> +	btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++] = st_ops;
> +
> +	return 0;
> +}
> +
> +int btf_add_struct_ops(struct bpf_struct_ops *st_ops, struct module *owner)
> +{
> +	struct btf *btf = btf_get_module_btf(owner);
> +	int ret;
> +
> +	if (!btf)
> +		return -ENOENT;
> +
> +	ret = btf_add_struct_ops_btf(st_ops, btf);
> +
> +	btf_put(btf);
> +
> +	return ret;
> +}
> +
> +const struct bpf_struct_ops **btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
> +{
> +	if (!btf)
> +		return NULL;
> +	if (!btf->struct_ops_tab)
> +		return NULL;
> +
> +	*ret_cnt = btf->struct_ops_tab->cnt;
> +	return (const struct bpf_struct_ops **)btf->struct_ops_tab->ops;
> +}

Re: [RFC bpf-next v3 02/11] bpf: add struct_ops_tab to btf.

[Kui-Feng Lee]

On 9/25/23 14:10, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> struct_ops_tab will be used to restore registered struct_ops.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/linux/btf.h |  9 +++++
>>   kernel/bpf/btf.c    | 84 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 93 insertions(+)
>>
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index 928113a80a95..5fabe23aedd2 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -571,4 +571,13 @@ static inline bool btf_type_is_struct_ptr(struct 
>> btf *btf, const struct btf_type
>>       return btf_type_is_struct(t);
>>   }
>> +struct bpf_struct_ops;
>> +
>> +int btf_add_struct_ops_btf(struct bpf_struct_ops *st_ops,
>> +               struct btf *btf);
>> +int btf_add_struct_ops(struct bpf_struct_ops *st_ops,
>> +               struct module *owner);
>> +const struct bpf_struct_ops **
>> +btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
>> +
>>   #endif
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index f93e835d90af..3fb9964f8672 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -241,6 +241,12 @@ struct btf_id_dtor_kfunc_tab {
>>       struct btf_id_dtor_kfunc dtors[];
>>   };
>> +struct btf_struct_ops_tab {
>> +    u32 cnt;
>> +    u32 capacity;
>> +    struct bpf_struct_ops *ops[];
>> +};
>> +
>>   struct btf {
>>       void *data;
>>       struct btf_type **types;
>> @@ -258,6 +264,7 @@ struct btf {
>>       struct btf_kfunc_set_tab *kfunc_set_tab;
>>       struct btf_id_dtor_kfunc_tab *dtor_kfunc_tab;
>>       struct btf_struct_metas *struct_meta_tab;
>> +    struct btf_struct_ops_tab *struct_ops_tab;
>>       /* split BTF support */
>>       struct btf *base_btf;
>> @@ -1688,11 +1695,20 @@ static void btf_free_struct_meta_tab(struct 
>> btf *btf)
>>       btf->struct_meta_tab = NULL;
>>   }
>> +static void btf_free_struct_ops_tab(struct btf *btf)
>> +{
>> +    struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
>> +
>> +    kfree(tab);
>> +    btf->struct_ops_tab = NULL;
>> +}
>> +
>>   static void btf_free(struct btf *btf)
>>   {
>>       btf_free_struct_meta_tab(btf);
>>       btf_free_dtor_kfunc_tab(btf);
>>       btf_free_kfunc_set_tab(btf);
>> +    btf_free_struct_ops_tab(btf);
>>       kvfree(btf->types);
>>       kvfree(btf->resolved_sizes);
>>       kvfree(btf->resolved_ids);
>> @@ -8601,3 +8617,71 @@ bool btf_type_ids_nocast_alias(struct 
>> bpf_verifier_log *log,
>>       return !strncmp(reg_name, arg_name, cmp_len);
>>   }
>> +
>> +int btf_add_struct_ops_btf(struct bpf_struct_ops *st_ops, struct btf 
>> *btf)
> 
> A few nits.
> 
> 'struct btf *btf' as the first argument, to be consistent with other 
> similar btf functions.

Got it!

> 
> This new function is not used outside of this file, so at least static. 
> I would just fold this into btf_add_struct_ops() below which currently 
> is mostly empty other than a btf_get/put.


Sure

> 
>> +{
>> +    struct btf_struct_ops_tab *tab;
>> +    int i;
>> +
>> +    /* Assume this function is called for a module when the module is
>> +     * loading.
>> +     */
>> +
>> +    tab = btf->struct_ops_tab;
>> +    if (!tab) {
>> +        tab = kzalloc(sizeof(*tab) +
>> +                  sizeof(struct bpf_struct_ops *) * 4,
>> +                  GFP_KERNEL);
> 
> nit. offsetof(struct bpf_struct_ops_tab, ops[4]).


Got it!

> 
>> +        if (!tab)
>> +            return -ENOMEM;
>> +        tab->capacity = 4;
>> +        btf->struct_ops_tab = tab;
>> +    }
>> +
>> +    for (i = 0; i < tab->cnt; i++)
>> +        if (tab->ops[i] == st_ops)
>> +            return -EEXIST;
>> +
>> +    if (tab->cnt == tab->capacity) {
>> +        struct btf_struct_ops_tab *new_tab;
>> +
>> +        new_tab = krealloc(tab, sizeof(*tab) +
>> +                   sizeof(struct bpf_struct_ops *) *
>> +                   tab->capacity * 2, GFP_KERNEL);
>> +        if (!new_tab)
>> +            return -ENOMEM;
>> +        tab = new_tab;
>> +        tab->capacity *= 2;
>> +        btf->struct_ops_tab = tab;
>> +    }
>> +
>> +    btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++] = st_ops;
>> +
>> +    return 0;
>> +}
>> +
>> +int btf_add_struct_ops(struct bpf_struct_ops *st_ops, struct module 
>> *owner)
>> +{
>> +    struct btf *btf = btf_get_module_btf(owner);
>> +    int ret;
>> +
>> +    if (!btf)
>> +        return -ENOENT;
>> +
>> +    ret = btf_add_struct_ops_btf(st_ops, btf);
>> +
>> +    btf_put(btf);
>> +
>> +    return ret;
>> +}
>> +
>> +const struct bpf_struct_ops **btf_get_struct_ops(struct btf *btf, u32 
>> *ret_cnt)
>> +{
>> +    if (!btf)
>> +        return NULL;
>> +    if (!btf->struct_ops_tab)
>> +        return NULL;
>> +
>> +    *ret_cnt = btf->struct_ops_tab->cnt;
>> +    return (const struct bpf_struct_ops **)btf->struct_ops_tab->ops;
>> +}
> 

Re: [RFC bpf-next v3 04/11] bpf: attach a module BTF to a bpf_struct_ops

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Every struct_ops type should has an associated module BTF to provide type
> information since we are going to allow modules to define and register new
> struct_ops types. New types may exist only in module itself, and the kernel
> BTF (vmlinux) doesn't know it at all. The attached module BTF here is going
> to be used to get correct btf and resolve type IDs of a struct_ops map.
> 
> However, it doesn't use the attached module BTF until we are ready to
> switch to registration function in subsequent patches.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/bpf.h         |  5 +++--
>   kernel/bpf/bpf_struct_ops.c | 27 ++++++++++++++++++---------
>   kernel/bpf/verifier.c       |  2 +-
>   3 files changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 67554f2f81b7..0776cb584b3f 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1626,6 +1626,7 @@ struct bpf_struct_ops {
>   	void (*unreg)(void *kdata);
>   	int (*update)(void *kdata, void *old_kdata);
>   	int (*validate)(void *kdata);
> +	const struct btf *btf;
>   	const struct btf_type *type;
>   	const struct btf_type *value_type;
>   	const char *name;
> @@ -1641,7 +1642,7 @@ struct bpf_struct_ops_mod {
>   
>   #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
>   #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
> -const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
> +const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf);
>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
>   bool bpf_struct_ops_get(const void *kdata);
>   void bpf_struct_ops_put(const void *kdata);
> @@ -1684,7 +1685,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
>   			    union bpf_attr __user *uattr);
>   #endif
>   #else
> -static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
> +static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf)
>   {
>   	return NULL;
>   }
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index cd688e9033b5..7c2ef53687ef 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -174,6 +174,10 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>   			pr_warn("Error in init bpf_struct_ops %s\n",
>   				st_ops->name);
>   		} else {
> +			/* XXX: We need a owner (module) here to company
> +			 * with type_id and value_id.
> +			 */
> +			st_ops->btf = btf;

I looked ahead in patch 5 and 7, I suspect I sort of getting why it does not 
need a refcount for the btf here.

Instead of having st_ops->btf pointing back to its containing btf, is it enough 
to store the btf in st_"map"->btf?

>   			st_ops->type_id = type_id;
>   			st_ops->type = t;
>   			st_ops->value_id = value_id;
> @@ -210,7 +214,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>   extern struct btf *btf_vmlinux;
>   
>   static const struct bpf_struct_ops *
> -bpf_struct_ops_find_value(u32 value_id)
> +bpf_struct_ops_find_value(u32 value_id, struct btf *btf)

nit. 'struct btf *btf' as the first argument, consistent with other btf search 
functions.

>   {
>   	unsigned int i;
>   
> @@ -225,7 +229,7 @@ bpf_struct_ops_find_value(u32 value_id)
>   	return NULL;
>   }
>   
> -const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
> +const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf)

same here.

>   {
>   	unsigned int i;
>   
> @@ -305,7 +309,7 @@ static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map)
>   	}
>   }
>   
> -static int check_zero_holes(const struct btf_type *t, void *data)
> +static int check_zero_holes(const struct btf *btf, const struct btf_type *t, void *data)
>   {
>   	const struct btf_member *member;
>   	u32 i, moff, msize, prev_mend = 0;
> @@ -317,8 +321,8 @@ static int check_zero_holes(const struct btf_type *t, void *data)
>   		    memchr_inv(data + prev_mend, 0, moff - prev_mend))
>   			return -EINVAL;
>   
> -		mtype = btf_type_by_id(btf_vmlinux, member->type);
> -		mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
> +		mtype = btf_type_by_id(btf, member->type);
> +		mtype = btf_resolve_size(btf, mtype, &msize);
>   		if (IS_ERR(mtype))
>   			return PTR_ERR(mtype);
>   		prev_mend = moff + msize;
> @@ -371,7 +375,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>   	const struct bpf_struct_ops *st_ops = st_map->st_ops;
>   	struct bpf_struct_ops_value *uvalue, *kvalue;
>   	const struct btf_member *member;
> -	const struct btf_type *t = st_ops->type;
> +	const struct btf_type *t;
>   	struct bpf_tramp_links *tlinks;
>   	void *udata, *kdata;
>   	int prog_fd, err;
> @@ -381,15 +385,20 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>   	if (flags)
>   		return -EINVAL;
>   
> +	if (!st_ops)
> +		return -EINVAL;

Why this new NULL check is needed?

> +
> +	t = st_ops->type;
> +
>   	if (*(u32 *)key != 0)
>   		return -E2BIG;
>   
> -	err = check_zero_holes(st_ops->value_type, value);
> +	err = check_zero_holes(st_ops->btf, st_ops->value_type, value);
>   	if (err)
>   		return err;
>   
>   	uvalue = value;
> -	err = check_zero_holes(t, uvalue->data);
> +	err = check_zero_holes(st_ops->btf, t, uvalue->data);
>   	if (err)
>   		return err;
>   
> @@ -660,7 +669,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	struct bpf_map *map;
>   	int ret;
>   
> -	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
> +	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
>   	if (!st_ops)
>   		return ERR_PTR(-ENOTSUPP);
>   
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a7178ecf676d..99b45501951c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19631,7 +19631,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>   	}
>   
>   	btf_id = prog->aux->attach_btf_id;
> -	st_ops = bpf_struct_ops_find(btf_id);
> +	st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
>   	if (!st_ops) {
>   		verbose(env, "attach_btf_id %u is not a supported struct\n",
>   			btf_id);

Re: [RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs.

[Andrii Nakryiko]

On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@gmail.com> wrote:
>
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> The type info of a struct_ops type may be in a module.  So, we need to know
> which module BTF to look for type information.  The later patches will make
> libbpf to attach module BTFs to programs. This patch passes attached BTF
> from syscall to bpf_struct_ops subsystem to make sure attached BTF is
> available when the bpf_struct_ops subsystem is ready to use it.
>
> bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along
> with the bpf_attr loading the program. attach_btf is used to find the btf
> type of attach_btf_id. attach_btf_id is used to identify the traced
> function for a trace program.  For struct_ops programs, it is used to
> identify the struct_ops type of the struct_ops object a program attached
> to.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  include/uapi/linux/bpf.h       |  4 ++++
>  kernel/bpf/bpf_struct_ops.c    | 12 +++++++++++-
>  kernel/bpf/syscall.c           |  2 +-
>  kernel/bpf/verifier.c          |  4 +++-
>  tools/include/uapi/linux/bpf.h |  4 ++++
>  5 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 73b155e52204..178d6fa45fa0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1390,6 +1390,10 @@ union bpf_attr {
>                  * to using 5 hash functions).
>                  */
>                 __u64   map_extra;
> +
> +               __u32   mod_btf_fd;     /* fd pointing to a BTF type data
> +                                        * for btf_vmlinux_value_type_id.
> +                                        */

we have attach_btf_obj_fd for BPF_PROG_LOAD command, so I guess
consistent naming would be "<something>_btf_obj_fd" where <something>
would make it more-or-less clear that this is BTF for
btf_vmlinux_value_type_id?

>         };
>
>         struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 8b5c859377e9..d5600d9ad302 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -765,9 +765,19 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>         struct bpf_struct_ops_map *st_map;
>         const struct btf_type *t, *vt;
>         struct bpf_map *map;
> +       struct btf *btf;
>         int ret;
>
> -       st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
> +       /* XXX: We need a module name or ID to find a BTF type. */
> +       /* XXX: should use btf from attr->btf_fd */

Do we need these XXX: comments? I think you had some more in previous patches

> +       if (attr->mod_btf_fd) {
> +               btf = btf_get_by_fd(attr->mod_btf_fd);
> +               if (IS_ERR(btf))
> +                       return ERR_PTR(PTR_ERR(btf));
> +       } else {
> +               btf = btf_vmlinux;
> +       }
> +       st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf);
>         if (!st_ops)
>                 return ERR_PTR(-ENOTSUPP);

should we make sure that module's BTF is put properly on error?

>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 85c1d908f70f..fed3870fec7a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1097,7 +1097,7 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
>         return ret;
>  }
>
> -#define BPF_MAP_CREATE_LAST_FIELD map_extra
> +#define BPF_MAP_CREATE_LAST_FIELD mod_btf_fd
>  /* called via syscall */
>  static int map_create(union bpf_attr *attr)
>  {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 99b45501951c..11f85dbc911b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19623,6 +19623,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>         const struct btf_member *member;
>         struct bpf_prog *prog = env->prog;
>         u32 btf_id, member_idx;
> +       struct btf *btf;
>         const char *mname;
>
>         if (!prog->gpl_compatible) {
> @@ -19630,8 +19631,9 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>                 return -EINVAL;
>         }
>
> +       btf = prog->aux->attach_btf;
>         btf_id = prog->aux->attach_btf_id;
> -       st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
> +       st_ops = bpf_struct_ops_find(btf_id, btf);
>         if (!st_ops) {
>                 verbose(env, "attach_btf_id %u is not a supported struct\n",
>                         btf_id);
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 73b155e52204..178d6fa45fa0 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1390,6 +1390,10 @@ union bpf_attr {
>                  * to using 5 hash functions).
>                  */
>                 __u64   map_extra;
> +
> +               __u32   mod_btf_fd;     /* fd pointing to a BTF type data
> +                                        * for btf_vmlinux_value_type_id.
> +                                        */
>         };
>
>         struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
> --
> 2.34.1
>

Re: [RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops.

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Provide registration functions to add/remove struct_ops types.
> 
> Currently, it does linear search to find a struct_ops type. It should be
> fine for now since we don't have many struct_ops types.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/bpf.h         |  9 +++++++++
>   include/linux/btf.h         | 27 +++++++++++++++++++++++++++
>   kernel/bpf/bpf_struct_ops.c | 11 -----------
>   kernel/bpf/btf.c            |  2 +-
>   4 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 30063a760b5a..67554f2f81b7 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1634,6 +1634,11 @@ struct bpf_struct_ops {
>   	u32 value_id;
>   };
>   
> +struct bpf_struct_ops_mod {
> +	struct module *owner;

After reading patch 5, I don't think this new 'struct bpf_struct_ops_mod' is needed.

> +	struct bpf_struct_ops *st_ops;

In patch 5, 'struct module *owner' has been added to 'bpf_struct_ops'. st_ops 
itself should already have the 'owner'.

> +};
> +
>   #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
>   #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
>   const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
> @@ -3205,4 +3210,8 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
>   	return prog->aux->func_idx != 0;
>   }
>   
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod);

This should be register_bpf_struct_ops(struct bpf_struct_ops *st_ops) instead.

> +#endif
> +
>   #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 5fabe23aedd2..8d50e46b21bc 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -12,6 +12,8 @@
>   #include <uapi/linux/bpf.h>
>   
>   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
> +#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0);	\
> +		((void)(struct bpf_struct_ops_##type *)0); }
>   #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>   
>   /* These need to be macros, as the expressions are used in assembler input */
> @@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
>   bool btf_is_kernel(const struct btf *btf);
>   bool btf_is_module(const struct btf *btf);
>   struct module *btf_try_get_module(const struct btf *btf);
> +struct btf *btf_get_module_btf(const struct module *module);
>   u32 btf_nr_types(const struct btf *btf);
>   bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
>   			   const struct btf_member *m,
> @@ -580,4 +583,28 @@ int btf_add_struct_ops(struct bpf_struct_ops *st_ops,
>   const struct bpf_struct_ops **
>   btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
>   
> +enum bpf_struct_ops_state {
> +	BPF_STRUCT_OPS_STATE_INIT,
> +	BPF_STRUCT_OPS_STATE_INUSE,
> +	BPF_STRUCT_OPS_STATE_TOBEFREE,
> +	BPF_STRUCT_OPS_STATE_READY,
> +};
> +
> +#define BPF_STRUCT_OPS_COMMON_VALUE			\
> +	refcount_t refcnt;				\
> +	enum bpf_struct_ops_state state
> +
> +/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
> + * the map's value exposed to the userspace and its btf-type-id is
> + * stored at the map->btf_vmlinux_value_type_id.
> + *
> + */
> +#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name)			\
> +extern struct bpf_struct_ops bpf_##_name;			\
> +								\
> +struct bpf_struct_ops_##_name {					\
> +	BPF_STRUCT_OPS_COMMON_VALUE;				\
> +	struct _name data ____cacheline_aligned_in_smp;		\
> +};
> +
>   #endif
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 627cf1ea840a..cd688e9033b5 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -13,17 +13,6 @@
>   #include <linux/btf_ids.h>
>   #include <linux/rcupdate_wait.h>
>   
> -enum bpf_struct_ops_state {
> -	BPF_STRUCT_OPS_STATE_INIT,
> -	BPF_STRUCT_OPS_STATE_INUSE,
> -	BPF_STRUCT_OPS_STATE_TOBEFREE,
> -	BPF_STRUCT_OPS_STATE_READY,
> -};
> -
> -#define BPF_STRUCT_OPS_COMMON_VALUE			\
> -	refcount_t refcnt;				\
> -	enum bpf_struct_ops_state state
> -
>   struct bpf_struct_ops_value {
>   	BPF_STRUCT_OPS_COMMON_VALUE;
>   	char data[] ____cacheline_aligned_in_smp;
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 3fb9964f8672..73d19ef99306 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct btf *btf)
>   /* Returns struct btf corresponding to the struct module.
>    * This function can return NULL or ERR_PTR.
>    */
> -static struct btf *btf_get_module_btf(const struct module *module)
> +struct btf *btf_get_module_btf(const struct module *module)
>   {
>   #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>   	struct btf_module *btf_mod, *tmp;

Re: [RFC bpf-next v3 09/11] libbpf: Find correct module BTFs for struct_ops maps and progs.

[Andrii Nakryiko]

On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@gmail.com> wrote:
>
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Find module BTFs for struct_ops maps and progs and pass them to the kernel.
> It ensures the kernel resolve type IDs from correct module BTFs.
>
> For the map of a struct_ops object, mod_btf is added to bpf_map to keep a
> reference to the module BTF. It's FD is passed to the kernel as mod_btf_fd
> when it is created.
>
> For a prog attaching to a struct_ops object, attach_btf_obj_fd of bpf_prog
> is the FD pointing to a module BTF in the kernel.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/lib/bpf/bpf.c    |   3 +-
>  tools/lib/bpf/bpf.h    |   4 +-
>  tools/lib/bpf/libbpf.c | 121 ++++++++++++++++++++++++-----------------
>  3 files changed, 75 insertions(+), 53 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index b0f1913763a3..df4b7570ad92 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -169,7 +169,7 @@ int bpf_map_create(enum bpf_map_type map_type,
>                    __u32 max_entries,
>                    const struct bpf_map_create_opts *opts)
>  {
> -       const size_t attr_sz = offsetofend(union bpf_attr, map_extra);
> +       const size_t attr_sz = offsetofend(union bpf_attr, mod_btf_fd);
>         union bpf_attr attr;
>         int fd;
>
> @@ -191,6 +191,7 @@ int bpf_map_create(enum bpf_map_type map_type,
>         attr.btf_key_type_id = OPTS_GET(opts, btf_key_type_id, 0);
>         attr.btf_value_type_id = OPTS_GET(opts, btf_value_type_id, 0);
>         attr.btf_vmlinux_value_type_id = OPTS_GET(opts, btf_vmlinux_value_type_id, 0);
> +       attr.mod_btf_fd = OPTS_GET(opts, mod_btf_fd, 0);
>
>         attr.inner_map_fd = OPTS_GET(opts, inner_map_fd, 0);
>         attr.map_flags = OPTS_GET(opts, map_flags, 0);
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index 74c2887cfd24..d18f75b0ccc9 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -51,8 +51,10 @@ struct bpf_map_create_opts {
>
>         __u32 numa_node;
>         __u32 map_ifindex;
> +
> +       __u32 mod_btf_fd;

please add `size_t :0;` at the end to avoid compiler leaving garbage
in added padding at the end of opts struct

>  };
> -#define bpf_map_create_opts__last_field map_ifindex
> +#define bpf_map_create_opts__last_field mod_btf_fd
>
>  LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
>                               const char *map_name,
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3a6108e3238b..df6ba5494adb 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -519,6 +519,7 @@ struct bpf_map {
>         struct bpf_map_def def;
>         __u32 numa_node;
>         __u32 btf_var_idx;
> +       struct module_btf *mod_btf;

It would be simpler to just store btf_fd instead of entire struct
module_btf pointer. You only need this to set btf_obj_fd on map
creation and program loading, right?

>         __u32 btf_key_type_id;
>         __u32 btf_value_type_id;
>         __u32 btf_vmlinux_value_type_id;
> @@ -893,6 +894,42 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
>         return 0;
>  }
>
> +static int load_module_btfs(struct bpf_object *obj);
> +
> +static int find_kern_btf_id(struct bpf_object *obj, const char *kern_name,
> +                           __u16 kind, struct btf **res_btf,
> +                           struct module_btf **res_mod_btf)
> +{
> +       struct module_btf *mod_btf;
> +       struct btf *btf;
> +       int i, id, err;
> +
> +       btf = obj->btf_vmlinux;
> +       mod_btf = NULL;
> +       id = btf__find_by_name_kind(btf, kern_name, kind);
> +
> +       if (id == -ENOENT) {
> +               err = load_module_btfs(obj);
> +               if (err)
> +                       return err;
> +
> +               for (i = 0; i < obj->btf_module_cnt; i++) {
> +                       /* we assume module_btf's BTF FD is always >0 */
> +                       mod_btf = &obj->btf_modules[i];
> +                       btf = mod_btf->btf;
> +                       id = btf__find_by_name_kind_own(btf, kern_name, kind);
> +                       if (id != -ENOENT)
> +                               break;
> +               }
> +       }
> +       if (id <= 0)
> +               return -ESRCH;
> +
> +       *res_btf = btf;
> +       *res_mod_btf = mod_btf;
> +       return id;
> +}
> +

there is no need to move the entire function body here, just add a
forward declaration. It will also make it easier to see what actually
changed about the function (if at all)

>  static const struct btf_member *
>  find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
>  {
> @@ -927,17 +964,23 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>                                    const char *name, __u32 kind);
>
>  static int
> -find_struct_ops_kern_types(const struct btf *btf, const char *tname,
> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
> +                          struct module_btf **mod_btf,
>                            const struct btf_type **type, __u32 *type_id,
>                            const struct btf_type **vtype, __u32 *vtype_id,
>                            const struct btf_member **data_member)
>  {
>         const struct btf_type *kern_type, *kern_vtype;
>         const struct btf_member *kern_data_member;
> +       struct btf *btf;
>         __s32 kern_vtype_id, kern_type_id;
>         __u32 i;
>
> -       kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);
> +       /* XXX: should search module BTFs as well. We need module name here
> +        * to locate a correct BTF type.
> +        */

aren't we searching module BTFs? Is this comment still relevant?

> +       kern_type_id = find_kern_btf_id(obj, tname, BTF_KIND_STRUCT,
> +                                       &btf, mod_btf);
>         if (kern_type_id < 0) {
>                 pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
>                         tname);
> @@ -992,13 +1035,15 @@ static bool bpf_map__is_struct_ops(const struct bpf_map *map)
>
>  /* Init the map's fields that depend on kern_btf */
>  static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
> -                                        const struct btf *btf,
> -                                        const struct btf *kern_btf)
> +                                        struct bpf_object *obj)

no need to pass obj separately, you can get it through `map->obj`

>  {
>         const struct btf_member *member, *kern_member, *kern_data_member;
>         const struct btf_type *type, *kern_type, *kern_vtype;
>         __u32 i, kern_type_id, kern_vtype_id, kern_data_off;
>         struct bpf_struct_ops *st_ops;
> +       const struct btf *kern_btf;
> +       struct module_btf *mod_btf;
> +       const struct btf *btf = obj->btf;
>         void *data, *kern_data;
>         const char *tname;
>         int err;
> @@ -1006,16 +1051,19 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>         st_ops = map->st_ops;
>         type = st_ops->type;
>         tname = st_ops->tname;
> -       err = find_struct_ops_kern_types(kern_btf, tname,
> +       err = find_struct_ops_kern_types(obj, tname, &mod_btf,
>                                          &kern_type, &kern_type_id,
>                                          &kern_vtype, &kern_vtype_id,
>                                          &kern_data_member);
>         if (err)
>                 return err;
>
> +       kern_btf = mod_btf ? mod_btf->btf : obj->btf_vmlinux;
> +
>         pr_debug("struct_ops init_kern %s: type_id:%u kern_type_id:%u kern_vtype_id:%u\n",
>                  map->name, st_ops->type_id, kern_type_id, kern_vtype_id);
>
> +       map->mod_btf = mod_btf;
>         map->def.value_size = kern_vtype->size;
>         map->btf_vmlinux_value_type_id = kern_vtype_id;
>
> @@ -1091,6 +1139,9 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>                                 return -ENOTSUP;
>                         }
>
> +                       /* XXX: attach_btf_obj_fd is needed as well */

seems like all these XXX comments are outdated and the code is already
doing all that, is that right? If so, please remove them, they are
confusing

> +                       if (mod_btf)
> +                               prog->attach_btf_obj_fd = mod_btf->fd;
>                         prog->attach_btf_id = kern_type_id;
>                         prog->expected_attach_type = kern_member_idx;
>
> @@ -1133,8 +1184,8 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
>                 if (!bpf_map__is_struct_ops(map))
>                         continue;
>
> -               err = bpf_map__init_kern_struct_ops(map, obj->btf,
> -                                                   obj->btf_vmlinux);
> +               /* XXX: should be a module btf if not vmlinux */
> +               err = bpf_map__init_kern_struct_ops(map, obj);
>                 if (err)
>                         return err;
>         }

[...]

Re: [RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops.

[Kui-Feng Lee]

On 9/25/23 16:07, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Provide registration functions to add/remove struct_ops types.
>>
>> Currently, it does linear search to find a struct_ops type. It should be
>> fine for now since we don't have many struct_ops types.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/linux/bpf.h         |  9 +++++++++
>>   include/linux/btf.h         | 27 +++++++++++++++++++++++++++
>>   kernel/bpf/bpf_struct_ops.c | 11 -----------
>>   kernel/bpf/btf.c            |  2 +-
>>   4 files changed, 37 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 30063a760b5a..67554f2f81b7 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1634,6 +1634,11 @@ struct bpf_struct_ops {
>>       u32 value_id;
>>   };
>> +struct bpf_struct_ops_mod {
>> +    struct module *owner;
> 
> After reading patch 5, I don't think this new 'struct 
> bpf_struct_ops_mod' is needed.
> 
>> +    struct bpf_struct_ops *st_ops;
> 
> In patch 5, 'struct module *owner' has been added to 'bpf_struct_ops'. 
> st_ops itself should already have the 'owner'.
> 
>> +};
>> +
>>   #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
>>   #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + 
>> POISON_POINTER_DELTA))
>>   const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
>> @@ -3205,4 +3210,8 @@ static inline bool bpf_is_subprog(const struct 
>> bpf_prog *prog)
>>       return prog->aux->func_idx != 0;
>>   }
>> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod);
> 
> This should be register_bpf_struct_ops(struct bpf_struct_ops *st_ops) 
> instead.

It is still required since the caller doesn't assign a module
to st_ops->owner.  I will force developers to fill st_ops->owner
before calling this function.

> 
>> +#endif
>> +
>>   #endif /* _LINUX_BPF_H */
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index 5fabe23aedd2..8d50e46b21bc 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -12,6 +12,8 @@
>>   #include <uapi/linux/bpf.h>
>>   #define BTF_TYPE_EMIT(type) ((void)(type *)0)
>> +#define BTF_STRUCT_OPS_TYPE_EMIT(type) {((void)(struct type *)0);    \
>> +        ((void)(struct bpf_struct_ops_##type *)0); }
>>   #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>>   /* These need to be macros, as the expressions are used in assembler 
>> input */
>> @@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
>>   bool btf_is_kernel(const struct btf *btf);
>>   bool btf_is_module(const struct btf *btf);
>>   struct module *btf_try_get_module(const struct btf *btf);
>> +struct btf *btf_get_module_btf(const struct module *module);
>>   u32 btf_nr_types(const struct btf *btf);
>>   bool btf_member_is_reg_int(const struct btf *btf, const struct 
>> btf_type *s,
>>                  const struct btf_member *m,
>> @@ -580,4 +583,28 @@ int btf_add_struct_ops(struct bpf_struct_ops 
>> *st_ops,
>>   const struct bpf_struct_ops **
>>   btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
>> +enum bpf_struct_ops_state {
>> +    BPF_STRUCT_OPS_STATE_INIT,
>> +    BPF_STRUCT_OPS_STATE_INUSE,
>> +    BPF_STRUCT_OPS_STATE_TOBEFREE,
>> +    BPF_STRUCT_OPS_STATE_READY,
>> +};
>> +
>> +#define BPF_STRUCT_OPS_COMMON_VALUE            \
>> +    refcount_t refcnt;                \
>> +    enum bpf_struct_ops_state state
>> +
>> +/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
>> + * the map's value exposed to the userspace and its btf-type-id is
>> + * stored at the map->btf_vmlinux_value_type_id.
>> + *
>> + */
>> +#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name)            \
>> +extern struct bpf_struct_ops bpf_##_name;            \
>> +                                \
>> +struct bpf_struct_ops_##_name {                    \
>> +    BPF_STRUCT_OPS_COMMON_VALUE;                \
>> +    struct _name data ____cacheline_aligned_in_smp;        \
>> +};
>> +
>>   #endif
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 627cf1ea840a..cd688e9033b5 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -13,17 +13,6 @@
>>   #include <linux/btf_ids.h>
>>   #include <linux/rcupdate_wait.h>
>> -enum bpf_struct_ops_state {
>> -    BPF_STRUCT_OPS_STATE_INIT,
>> -    BPF_STRUCT_OPS_STATE_INUSE,
>> -    BPF_STRUCT_OPS_STATE_TOBEFREE,
>> -    BPF_STRUCT_OPS_STATE_READY,
>> -};
>> -
>> -#define BPF_STRUCT_OPS_COMMON_VALUE            \
>> -    refcount_t refcnt;                \
>> -    enum bpf_struct_ops_state state
>> -
>>   struct bpf_struct_ops_value {
>>       BPF_STRUCT_OPS_COMMON_VALUE;
>>       char data[] ____cacheline_aligned_in_smp;
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 3fb9964f8672..73d19ef99306 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct 
>> btf *btf)
>>   /* Returns struct btf corresponding to the struct module.
>>    * This function can return NULL or ERR_PTR.
>>    */
>> -static struct btf *btf_get_module_btf(const struct module *module)
>> +struct btf *btf_get_module_btf(const struct module *module)
>>   {
>>   #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>>       struct btf_module *btf_mod, *tmp;
> 

Re: [RFC bpf-next v3 05/11] bpf: hold module for bpf_struct_ops_map.

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Ensure a module doesn't go away when a struct_ops object is still alive,
> being a struct_ops type that is registered by the module.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/bpf.h         | 1 +
>   kernel/bpf/bpf_struct_ops.c | 6 ++++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 0776cb584b3f..faaec20156f1 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
>   	int (*update)(void *kdata, void *old_kdata);
>   	int (*validate)(void *kdata);
>   	const struct btf *btf;
> +	struct module *owner;
>   	const struct btf_type *type;
>   	const struct btf_type *value_type;
>   	const char *name;
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 7c2ef53687ef..ef8a1edec891 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -632,6 +632,8 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
>   
>   static void bpf_struct_ops_map_free(struct bpf_map *map)
>   {
> +	struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
> +
>   	/* The struct_ops's function may switch to another struct_ops.
>   	 *
>   	 * For example, bpf_tcp_cc_x->init() may switch to
> @@ -649,6 +651,7 @@ static void bpf_struct_ops_map_free(struct bpf_map *map)
>   	 */
>   	synchronize_rcu_mult(call_rcu, call_rcu_tasks);
>   
> +	module_put(st_map->st_ops->owner);
>   	__bpf_struct_ops_map_free(map);
>   }
>   
> @@ -673,6 +676,9 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	if (!st_ops)
>   		return ERR_PTR(-ENOTSUPP);
>   
> +	if (!try_module_get(st_ops->owner))
> +		return ERR_PTR(-EINVAL);

The module can be gone at this point?
I don't think try_module_get is safe. btf_try_get_module should be used instead.

> +
>   	vt = st_ops->value_type;
>   	if (attr->value_size != vt->size)
>   		return ERR_PTR(-EINVAL);

Re: [RFC bpf-next v3 04/11] bpf: attach a module BTF to a bpf_struct_ops

[Kui-Feng Lee]

On 9/25/23 15:57, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Every struct_ops type should has an associated module BTF to provide type
>> information since we are going to allow modules to define and register 
>> new
>> struct_ops types. New types may exist only in module itself, and the 
>> kernel
>> BTF (vmlinux) doesn't know it at all. The attached module BTF here is 
>> going
>> to be used to get correct btf and resolve type IDs of a struct_ops map.
>>
>> However, it doesn't use the attached module BTF until we are ready to
>> switch to registration function in subsequent patches.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/linux/bpf.h         |  5 +++--
>>   kernel/bpf/bpf_struct_ops.c | 27 ++++++++++++++++++---------
>>   kernel/bpf/verifier.c       |  2 +-
>>   3 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 67554f2f81b7..0776cb584b3f 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1626,6 +1626,7 @@ struct bpf_struct_ops {
>>       void (*unreg)(void *kdata);
>>       int (*update)(void *kdata, void *old_kdata);
>>       int (*validate)(void *kdata);
>> +    const struct btf *btf;
>>       const struct btf_type *type;
>>       const struct btf_type *value_type;
>>       const char *name;
>> @@ -1641,7 +1642,7 @@ struct bpf_struct_ops_mod {
>>   #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
>>   #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + 
>> POISON_POINTER_DELTA))
>> -const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
>> +const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct 
>> btf *btf);
>>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log 
>> *log);
>>   bool bpf_struct_ops_get(const void *kdata);
>>   void bpf_struct_ops_put(const void *kdata);
>> @@ -1684,7 +1685,7 @@ int bpf_struct_ops_test_run(struct bpf_prog 
>> *prog, const union bpf_attr *kattr,
>>                   union bpf_attr __user *uattr);
>>   #endif
>>   #else
>> -static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32 
>> type_id)
>> +static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32 
>> type_id, struct btf *btf)
>>   {
>>       return NULL;
>>   }
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index cd688e9033b5..7c2ef53687ef 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -174,6 +174,10 @@ static void bpf_struct_ops_init_one(struct 
>> bpf_struct_ops *st_ops,
>>               pr_warn("Error in init bpf_struct_ops %s\n",
>>                   st_ops->name);
>>           } else {
>> +            /* XXX: We need a owner (module) here to company
>> +             * with type_id and value_id.
>> +             */
>> +            st_ops->btf = btf;
> 
> I looked ahead in patch 5 and 7, I suspect I sort of getting why it does 
> not need a refcount for the btf here.
> 
> Instead of having st_ops->btf pointing back to its containing btf, is it 
> enough to store the btf in st_"map"->btf?

Basically, we can put the pointer to btf at either st_ops or st_maps.
Since a st_ops is always associated with a btf, and its st_maps need
a pointer to st_ops, keep the btf pointer at st_ops is reasonable for me
and efficient.

What is your concern about st_ops->btf?

> 
>>               st_ops->type_id = type_id;
>>               st_ops->type = t;
>>               st_ops->value_id = value_id;
>> @@ -210,7 +214,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
>> bpf_verifier_log *log)
>>   extern struct btf *btf_vmlinux;
>>   static const struct bpf_struct_ops *
>> -bpf_struct_ops_find_value(u32 value_id)
>> +bpf_struct_ops_find_value(u32 value_id, struct btf *btf)
> 
> nit. 'struct btf *btf' as the first argument, consistent with other btf 
> search functions.

Got it!

> 
>>   {
>>       unsigned int i;
>> @@ -225,7 +229,7 @@ bpf_struct_ops_find_value(u32 value_id)
>>       return NULL;
>>   }
>> -const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
>> +const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct 
>> btf *btf)
> 
> same here.
> 
>>   {
>>       unsigned int i;
>> @@ -305,7 +309,7 @@ static void bpf_struct_ops_map_put_progs(struct 
>> bpf_struct_ops_map *st_map)
>>       }
>>   }
>> -static int check_zero_holes(const struct btf_type *t, void *data)
>> +static int check_zero_holes(const struct btf *btf, const struct 
>> btf_type *t, void *data)
>>   {
>>       const struct btf_member *member;
>>       u32 i, moff, msize, prev_mend = 0;
>> @@ -317,8 +321,8 @@ static int check_zero_holes(const struct btf_type 
>> *t, void *data)
>>               memchr_inv(data + prev_mend, 0, moff - prev_mend))
>>               return -EINVAL;
>> -        mtype = btf_type_by_id(btf_vmlinux, member->type);
>> -        mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
>> +        mtype = btf_type_by_id(btf, member->type);
>> +        mtype = btf_resolve_size(btf, mtype, &msize);
>>           if (IS_ERR(mtype))
>>               return PTR_ERR(mtype);
>>           prev_mend = moff + msize;
>> @@ -371,7 +375,7 @@ static long bpf_struct_ops_map_update_elem(struct 
>> bpf_map *map, void *key,
>>       const struct bpf_struct_ops *st_ops = st_map->st_ops;
>>       struct bpf_struct_ops_value *uvalue, *kvalue;
>>       const struct btf_member *member;
>> -    const struct btf_type *t = st_ops->type;
>> +    const struct btf_type *t;
>>       struct bpf_tramp_links *tlinks;
>>       void *udata, *kdata;
>>       int prog_fd, err;
>> @@ -381,15 +385,20 @@ static long 
>> bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
>>       if (flags)
>>           return -EINVAL;
>> +    if (!st_ops)
>> +        return -EINVAL;
> 
> Why this new NULL check is needed?
> 
>> +
>> +    t = st_ops->type;
>> +
>>       if (*(u32 *)key != 0)
>>           return -E2BIG;
>> -    err = check_zero_holes(st_ops->value_type, value);
>> +    err = check_zero_holes(st_ops->btf, st_ops->value_type, value);
>>       if (err)
>>           return err;
>>       uvalue = value;
>> -    err = check_zero_holes(t, uvalue->data);
>> +    err = check_zero_holes(st_ops->btf, t, uvalue->data);
>>       if (err)
>>           return err;
>> @@ -660,7 +669,7 @@ static struct bpf_map 
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>       struct bpf_map *map;
>>       int ret;
>> -    st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
>> +    st_ops = 
>> bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
>>       if (!st_ops)
>>           return ERR_PTR(-ENOTSUPP);
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index a7178ecf676d..99b45501951c 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19631,7 +19631,7 @@ static int check_struct_ops_btf_id(struct 
>> bpf_verifier_env *env)
>>       }
>>       btf_id = prog->aux->attach_btf_id;
>> -    st_ops = bpf_struct_ops_find(btf_id);
>> +    st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
>>       if (!st_ops) {
>>           verbose(env, "attach_btf_id %u is not a supported struct\n",
>>               btf_id);
> 

Re: [RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops.

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES

CONFIG_DEBUG_INFO_BTF_MODULES is probably too restrictive. bpf_tcp_ca does not 
necessary need CONFIG_DEBUG_INFO_BTF_MODULES

> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod);
> +#endif

Re: [RFC bpf-next v3 05/11] bpf: hold module for bpf_struct_ops_map.

[Kui-Feng Lee]

On 9/25/23 16:23, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Ensure a module doesn't go away when a struct_ops object is still alive,
>> being a struct_ops type that is registered by the module.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/linux/bpf.h         | 1 +
>>   kernel/bpf/bpf_struct_ops.c | 6 ++++++
>>   2 files changed, 7 insertions(+)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 0776cb584b3f..faaec20156f1 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
>>       int (*update)(void *kdata, void *old_kdata);
>>       int (*validate)(void *kdata);
>>       const struct btf *btf;
>> +    struct module *owner;
>>       const struct btf_type *type;
>>       const struct btf_type *value_type;
>>       const char *name;
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 7c2ef53687ef..ef8a1edec891 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -632,6 +632,8 @@ static void __bpf_struct_ops_map_free(struct 
>> bpf_map *map)
>>   static void bpf_struct_ops_map_free(struct bpf_map *map)
>>   {
>> +    struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map 
>> *)map;
>> +
>>       /* The struct_ops's function may switch to another struct_ops.
>>        *
>>        * For example, bpf_tcp_cc_x->init() may switch to
>> @@ -649,6 +651,7 @@ static void bpf_struct_ops_map_free(struct bpf_map 
>> *map)
>>        */
>>       synchronize_rcu_mult(call_rcu, call_rcu_tasks);
>> +    module_put(st_map->st_ops->owner);
>>       __bpf_struct_ops_map_free(map);
>>   }
>> @@ -673,6 +676,9 @@ static struct bpf_map 
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>       if (!st_ops)
>>           return ERR_PTR(-ENOTSUPP);
>> +    if (!try_module_get(st_ops->owner))
>> +        return ERR_PTR(-EINVAL);
> 
> The module can be gone at this point?
> I don't think try_module_get is safe. btf_try_get_module should be used 
> instead.

At this point, it holds btf, but not module. Module can go away while
some one still holding a refcount to the btf.

And, you are right, I should use btf_try_get_module().

> 
>> +
>>       vt = st_ops->value_type;
>>       if (attr->value_size != vt->size)
>>           return ERR_PTR(-EINVAL);
> 

Re: [RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs.

[Kui-Feng Lee]

On 9/25/23 15:58, Andrii Nakryiko wrote:
> On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@gmail.com> wrote:
>>
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> The type info of a struct_ops type may be in a module.  So, we need to know
>> which module BTF to look for type information.  The later patches will make
>> libbpf to attach module BTFs to programs. This patch passes attached BTF
>> from syscall to bpf_struct_ops subsystem to make sure attached BTF is
>> available when the bpf_struct_ops subsystem is ready to use it.
>>
>> bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along
>> with the bpf_attr loading the program. attach_btf is used to find the btf
>> type of attach_btf_id. attach_btf_id is used to identify the traced
>> function for a trace program.  For struct_ops programs, it is used to
>> identify the struct_ops type of the struct_ops object a program attached
>> to.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/uapi/linux/bpf.h       |  4 ++++
>>   kernel/bpf/bpf_struct_ops.c    | 12 +++++++++++-
>>   kernel/bpf/syscall.c           |  2 +-
>>   kernel/bpf/verifier.c          |  4 +++-
>>   tools/include/uapi/linux/bpf.h |  4 ++++
>>   5 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 73b155e52204..178d6fa45fa0 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1390,6 +1390,10 @@ union bpf_attr {
>>                   * to using 5 hash functions).
>>                   */
>>                  __u64   map_extra;
>> +
>> +               __u32   mod_btf_fd;     /* fd pointing to a BTF type data
>> +                                        * for btf_vmlinux_value_type_id.
>> +                                        */
> 
> we have attach_btf_obj_fd for BPF_PROG_LOAD command, so I guess
> consistent naming would be "<something>_btf_obj_fd" where <something>
> would make it more-or-less clear that this is BTF for
> btf_vmlinux_value_type_id?

Got it! I will rename it to value_type_btf_obj_fd.

> 
>>          };
>>
>>          struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 8b5c859377e9..d5600d9ad302 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -765,9 +765,19 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>          struct bpf_struct_ops_map *st_map;
>>          const struct btf_type *t, *vt;
>>          struct bpf_map *map;
>> +       struct btf *btf;
>>          int ret;
>>
>> -       st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
>> +       /* XXX: We need a module name or ID to find a BTF type. */
>> +       /* XXX: should use btf from attr->btf_fd */
> 
> Do we need these XXX: comments? I think you had some more in previous patches

Will be removed.

> 
>> +       if (attr->mod_btf_fd) {
>> +               btf = btf_get_by_fd(attr->mod_btf_fd);
>> +               if (IS_ERR(btf))
>> +                       return ERR_PTR(PTR_ERR(btf));
>> +       } else {
>> +               btf = btf_vmlinux;
>> +       }
>> +       st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf);
>>          if (!st_ops)
>>                  return ERR_PTR(-ENOTSUPP);
> 
> should we make sure that module's BTF is put properly on error?

Yes, this issue has been addressed locally.

> 
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 85c1d908f70f..fed3870fec7a 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -1097,7 +1097,7 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
>>          return ret;
>>   }
>>
>> -#define BPF_MAP_CREATE_LAST_FIELD map_extra
>> +#define BPF_MAP_CREATE_LAST_FIELD mod_btf_fd
>>   /* called via syscall */
>>   static int map_create(union bpf_attr *attr)
>>   {
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 99b45501951c..11f85dbc911b 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -19623,6 +19623,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>>          const struct btf_member *member;
>>          struct bpf_prog *prog = env->prog;
>>          u32 btf_id, member_idx;
>> +       struct btf *btf;
>>          const char *mname;
>>
>>          if (!prog->gpl_compatible) {
>> @@ -19630,8 +19631,9 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
>>                  return -EINVAL;
>>          }
>>
>> +       btf = prog->aux->attach_btf;
>>          btf_id = prog->aux->attach_btf_id;
>> -       st_ops = bpf_struct_ops_find(btf_id, btf_vmlinux);
>> +       st_ops = bpf_struct_ops_find(btf_id, btf);
>>          if (!st_ops) {
>>                  verbose(env, "attach_btf_id %u is not a supported struct\n",
>>                          btf_id);
>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>> index 73b155e52204..178d6fa45fa0 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -1390,6 +1390,10 @@ union bpf_attr {
>>                   * to using 5 hash functions).
>>                   */
>>                  __u64   map_extra;
>> +
>> +               __u32   mod_btf_fd;     /* fd pointing to a BTF type data
>> +                                        * for btf_vmlinux_value_type_id.
>> +                                        */
>>          };
>>
>>          struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
>> --
>> 2.34.1
>>

Re: [RFC bpf-next v3 07/11] bpf, net: switch to storing struct_ops in btf

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Use struct_ops registered and stored in module btf instead of static ones.
> 
> Both bpf_dummy_ops and bpf_tcp_ca switches to calling the registration
> function instead of listed in bpf_struct_ops_types.h.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/bpf_struct_ops.c       | 114 ++++++++++++++++++------------
>   kernel/bpf/bpf_struct_ops_types.h |  12 ----
>   net/bpf/bpf_dummy_struct_ops.c    |  12 +++-
>   net/ipv4/bpf_tcp_ca.c             |  20 +++++-
>   4 files changed, 94 insertions(+), 64 deletions(-)
>   delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index fb684d2ee99d..8b5c859377e9 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -59,35 +59,6 @@ static DEFINE_MUTEX(update_mutex);
>   #define VALUE_PREFIX "bpf_struct_ops_"
>   #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
>   
> -/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
> - * the map's value exposed to the userspace and its btf-type-id is
> - * stored at the map->btf_vmlinux_value_type_id.
> - *
> - */
> -#define BPF_STRUCT_OPS_TYPE(_name)				\
> -extern struct bpf_struct_ops bpf_##_name;			\
> -								\
> -struct bpf_struct_ops_##_name {						\
> -	BPF_STRUCT_OPS_COMMON_VALUE;				\
> -	struct _name data ____cacheline_aligned_in_smp;		\
> -};
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> -
> -enum {
> -#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> -	__NR_BPF_STRUCT_OPS_TYPE,
> -};
> -
> -static struct bpf_struct_ops * const bpf_struct_ops[] = {
> -#define BPF_STRUCT_OPS_TYPE(_name)				\
> -	[BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> -};
> -
>   const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
>   };
>   
> @@ -264,14 +235,11 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>   
>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>   {
> -	struct bpf_struct_ops *st_ops;
> +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
> +	extern struct bpf_struct_ops_mod bpf_testmod_struct_ops;
> +	int ret;
> +#endif
>   	s32 module_id;
> -	u32 i;
> -
> -	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
> -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
>   
>   	module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
>   	if (module_id < 0) {
> @@ -280,43 +248,95 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>   	}
>   	module_type = btf_type_by_id(btf, module_id);
>   
> -	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
> -		st_ops = bpf_struct_ops[i];
> -		bpf_struct_ops_init_one(st_ops, btf, log);
> +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
> +	ret = register_bpf_struct_ops(&bpf_testmod_struct_ops);

What is stopping the 'register_bpf_struct_ops(&bpf_testmod_struct_ops)' to be 
done in bpf_dummy_struct_ops.c instead of here?

I am hoping bpf_dummy_struct_ops.c can eventually be moved out to 
bpf_testmod_struct_ops.c but it is better to leave it as a followup later.

> +	if (ret)
> +		pr_warn("Cannot register bpf_testmod_struct_ops\n");
> +#endif
> +}
> +
> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
> +{
> +	struct bpf_struct_ops *st_ops = mod->st_ops;
> +	struct bpf_verifier_log *log;
> +	struct btf *btf;
> +	int err;
> +
> +	if (mod->st_ops == NULL ||
> +	    mod->owner == NULL)
> +		return -EINVAL;
> +
> +	log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
> +	if (!log) {
> +		err = -ENOMEM;
> +		goto errout;
> +	}
> +
> +	log->level = BPF_LOG_KERNEL;
> +
> +	btf = btf_get_module_btf(mod->owner);
> +	if (!btf) {
> +		err = -EINVAL;
> +		goto errout;
>   	}
> +
> +	bpf_struct_ops_init_one(st_ops, btf, log);
> +
> +	btf_put(btf);
> +
> +	st_ops->owner = mod->owner;
> +	err = btf_add_struct_ops(st_ops, st_ops->owner);
> +
> +errout:
> +	kfree(log);
> +
> +	return err;
>   }
> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
>   
>   extern struct btf *btf_vmlinux;
>   
>   static const struct bpf_struct_ops *
>   bpf_struct_ops_find_value(u32 value_id, struct btf *btf)
>   {
> +	const struct bpf_struct_ops *st_ops = NULL;
> +	const struct bpf_struct_ops **st_ops_list;
>   	unsigned int i;
> +	u32 cnt = 0;
>   
>   	if (!value_id || !btf_vmlinux)
>   		return NULL;
>   
> -	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
> -		if (bpf_struct_ops[i]->value_id == value_id)
> -			return bpf_struct_ops[i];
> +	st_ops_list = btf_get_struct_ops(btf, &cnt);
> +	for (i = 0; i < cnt; i++) {
> +		if (st_ops_list[i]->value_id == value_id) {
> +			st_ops = st_ops_list[i];
> +			break;
> +		}
>   	}
>   
> -	return NULL;
> +	return st_ops;
>   }
>   
>   const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct btf *btf)
>   {
> +	const struct bpf_struct_ops *st_ops = NULL;
> +	const struct bpf_struct_ops **st_ops_list;
>   	unsigned int i;
> +	u32 cnt;
>   
>   	if (!type_id || !btf_vmlinux)
>   		return NULL;
>   
> -	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
> -		if (bpf_struct_ops[i]->type_id == type_id)
> -			return bpf_struct_ops[i];
> +	st_ops_list = btf_get_struct_ops(btf, &cnt);
> +	for (i = 0; i < cnt; i++) {
> +		if (st_ops_list[i]->type_id == type_id) {
> +			st_ops = st_ops_list[i];
> +			break;
> +		}
>   	}
>   
> -	return NULL;
> +	return st_ops;
>   }
>   
>   static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,
> diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
> deleted file mode 100644
> index 5678a9ddf817..000000000000
> --- a/kernel/bpf/bpf_struct_ops_types.h
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/* internal file - do not include directly */
> -
> -#ifdef CONFIG_BPF_JIT
> -#ifdef CONFIG_NET
> -BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
> -#endif
> -#ifdef CONFIG_INET
> -#include <net/tcp.h>
> -BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
> -#endif
> -#endif
> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index 5918d1b32e19..9cb982c67c4c 100644
> --- a/net/bpf/bpf_dummy_struct_ops.c
> +++ b/net/bpf/bpf_dummy_struct_ops.c
> @@ -7,7 +7,7 @@
>   #include <linux/bpf.h>
>   #include <linux/btf.h>
>   
> -extern struct bpf_struct_ops bpf_bpf_dummy_ops;
> +static struct bpf_struct_ops bpf_bpf_dummy_ops;
>   
>   /* A common type for test_N with return value in bpf_dummy_ops */
>   typedef int (*dummy_ops_test_ret_fn)(struct bpf_dummy_ops_state *state, ...);
> @@ -218,9 +218,12 @@ static int bpf_dummy_reg(void *kdata)
>   
>   static void bpf_dummy_unreg(void *kdata)
>   {
> +	BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops);
>   }
>   
> -struct bpf_struct_ops bpf_bpf_dummy_ops = {
> +DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_dummy_ops);
> +
> +static struct bpf_struct_ops bpf_bpf_dummy_ops = {
>   	.verifier_ops = &bpf_dummy_verifier_ops,
>   	.init = bpf_dummy_init,
>   	.check_member = bpf_dummy_ops_check_member,
> @@ -229,3 +232,8 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = {
>   	.unreg = bpf_dummy_unreg,
>   	.name = "bpf_dummy_ops",
>   };
> +
> +struct bpf_struct_ops_mod bpf_testmod_struct_ops = {
> +	.st_ops = &bpf_bpf_dummy_ops,
> +	.owner = THIS_MODULE,
> +};
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index 39dcccf0f174..9947323f3e22 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -12,7 +12,7 @@
>   #include <net/bpf_sk_storage.h>
>   
>   /* "extern" is to avoid sparse warning.  It is only used in bpf_struct_ops.c. */
> -extern struct bpf_struct_ops bpf_tcp_congestion_ops;
> +static struct bpf_struct_ops bpf_tcp_congestion_ops;
>   
>   static u32 unsupported_ops[] = {
>   	offsetof(struct tcp_congestion_ops, get_info),
> @@ -271,7 +271,9 @@ static int bpf_tcp_ca_validate(void *kdata)
>   	return tcp_validate_congestion_control(kdata);
>   }
>   
> -struct bpf_struct_ops bpf_tcp_congestion_ops = {
> +DEFINE_STRUCT_OPS_VALUE_TYPE(tcp_congestion_ops);
> +
> +static struct bpf_struct_ops bpf_tcp_congestion_ops = {
>   	.verifier_ops = &bpf_tcp_ca_verifier_ops,
>   	.reg = bpf_tcp_ca_reg,
>   	.unreg = bpf_tcp_ca_unreg,
> @@ -283,8 +285,20 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = {
>   	.name = "tcp_congestion_ops",
>   };
>   
> +static struct bpf_struct_ops_mod bpf_tcp_ca_ops_mod = {
> +	.st_ops = &bpf_tcp_congestion_ops,
> +	.owner = THIS_MODULE,
> +};
> +
>   static int __init bpf_tcp_ca_kfunc_init(void)
>   {
> -	return register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set);
> +	int ret;
> +
> +	BTF_STRUCT_OPS_TYPE_EMIT(tcp_congestion_ops);
> +
> +	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set);
> +	ret = ret ?: register_bpf_struct_ops(&bpf_tcp_ca_ops_mod);
> +
> +	return ret;
>   }
>   late_initcall(bpf_tcp_ca_kfunc_init);

Re: [RFC bpf-next v3 09/11] libbpf: Find correct module BTFs for struct_ops maps and progs.

[Kui-Feng Lee]

On 9/25/23 16:09, Andrii Nakryiko wrote:
> On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@gmail.com> wrote:
>>
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Find module BTFs for struct_ops maps and progs and pass them to the kernel.
>> It ensures the kernel resolve type IDs from correct module BTFs.
>>
>> For the map of a struct_ops object, mod_btf is added to bpf_map to keep a
>> reference to the module BTF. It's FD is passed to the kernel as mod_btf_fd
>> when it is created.
>>
>> For a prog attaching to a struct_ops object, attach_btf_obj_fd of bpf_prog
>> is the FD pointing to a module BTF in the kernel.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/lib/bpf/bpf.c    |   3 +-
>>   tools/lib/bpf/bpf.h    |   4 +-
>>   tools/lib/bpf/libbpf.c | 121 ++++++++++++++++++++++++-----------------
>>   3 files changed, 75 insertions(+), 53 deletions(-)
>>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index b0f1913763a3..df4b7570ad92 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -169,7 +169,7 @@ int bpf_map_create(enum bpf_map_type map_type,
>>                     __u32 max_entries,
>>                     const struct bpf_map_create_opts *opts)
>>   {
>> -       const size_t attr_sz = offsetofend(union bpf_attr, map_extra);
>> +       const size_t attr_sz = offsetofend(union bpf_attr, mod_btf_fd);
>>          union bpf_attr attr;
>>          int fd;
>>
>> @@ -191,6 +191,7 @@ int bpf_map_create(enum bpf_map_type map_type,
>>          attr.btf_key_type_id = OPTS_GET(opts, btf_key_type_id, 0);
>>          attr.btf_value_type_id = OPTS_GET(opts, btf_value_type_id, 0);
>>          attr.btf_vmlinux_value_type_id = OPTS_GET(opts, btf_vmlinux_value_type_id, 0);
>> +       attr.mod_btf_fd = OPTS_GET(opts, mod_btf_fd, 0);
>>
>>          attr.inner_map_fd = OPTS_GET(opts, inner_map_fd, 0);
>>          attr.map_flags = OPTS_GET(opts, map_flags, 0);
>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> index 74c2887cfd24..d18f75b0ccc9 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
>> @@ -51,8 +51,10 @@ struct bpf_map_create_opts {
>>
>>          __u32 numa_node;
>>          __u32 map_ifindex;
>> +
>> +       __u32 mod_btf_fd;
> 
> please add `size_t :0;` at the end to avoid compiler leaving garbage
> in added padding at the end of opts struct

Ok!

> 
>>   };
>> -#define bpf_map_create_opts__last_field map_ifindex
>> +#define bpf_map_create_opts__last_field mod_btf_fd
>>
>>   LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
>>                                const char *map_name,
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 3a6108e3238b..df6ba5494adb 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -519,6 +519,7 @@ struct bpf_map {
>>          struct bpf_map_def def;
>>          __u32 numa_node;
>>          __u32 btf_var_idx;
>> +       struct module_btf *mod_btf;
> 
> It would be simpler to just store btf_fd instead of entire struct
> module_btf pointer. You only need this to set btf_obj_fd on map
> creation and program loading, right?

Yes!

> 
>>          __u32 btf_key_type_id;
>>          __u32 btf_value_type_id;
>>          __u32 btf_vmlinux_value_type_id;
>> @@ -893,6 +894,42 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
>>          return 0;
>>   }
>>
>> +static int load_module_btfs(struct bpf_object *obj);
>> +
>> +static int find_kern_btf_id(struct bpf_object *obj, const char *kern_name,
>> +                           __u16 kind, struct btf **res_btf,
>> +                           struct module_btf **res_mod_btf)
>> +{
>> +       struct module_btf *mod_btf;
>> +       struct btf *btf;
>> +       int i, id, err;
>> +
>> +       btf = obj->btf_vmlinux;
>> +       mod_btf = NULL;
>> +       id = btf__find_by_name_kind(btf, kern_name, kind);
>> +
>> +       if (id == -ENOENT) {
>> +               err = load_module_btfs(obj);
>> +               if (err)
>> +                       return err;
>> +
>> +               for (i = 0; i < obj->btf_module_cnt; i++) {
>> +                       /* we assume module_btf's BTF FD is always >0 */
>> +                       mod_btf = &obj->btf_modules[i];
>> +                       btf = mod_btf->btf;
>> +                       id = btf__find_by_name_kind_own(btf, kern_name, kind);
>> +                       if (id != -ENOENT)
>> +                               break;
>> +               }
>> +       }
>> +       if (id <= 0)
>> +               return -ESRCH;
>> +
>> +       *res_btf = btf;
>> +       *res_mod_btf = mod_btf;
>> +       return id;
>> +}
>> +
> 
> there is no need to move the entire function body here, just add a
> forward declaration. It will also make it easier to see what actually
> changed about the function (if at all)

Got it

> 
>>   static const struct btf_member *
>>   find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
>>   {
>> @@ -927,17 +964,23 @@ static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
>>                                     const char *name, __u32 kind);
>>
>>   static int
>> -find_struct_ops_kern_types(const struct btf *btf, const char *tname,
>> +find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
>> +                          struct module_btf **mod_btf,
>>                             const struct btf_type **type, __u32 *type_id,
>>                             const struct btf_type **vtype, __u32 *vtype_id,
>>                             const struct btf_member **data_member)
>>   {
>>          const struct btf_type *kern_type, *kern_vtype;
>>          const struct btf_member *kern_data_member;
>> +       struct btf *btf;
>>          __s32 kern_vtype_id, kern_type_id;
>>          __u32 i;
>>
>> -       kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);
>> +       /* XXX: should search module BTFs as well. We need module name here
>> +        * to locate a correct BTF type.
>> +        */
> 
> aren't we searching module BTFs? Is this comment still relevant?
> 
>> +       kern_type_id = find_kern_btf_id(obj, tname, BTF_KIND_STRUCT,
>> +                                       &btf, mod_btf);
>>          if (kern_type_id < 0) {
>>                  pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
>>                          tname);
>> @@ -992,13 +1035,15 @@ static bool bpf_map__is_struct_ops(const struct bpf_map *map)
>>
>>   /* Init the map's fields that depend on kern_btf */
>>   static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>> -                                        const struct btf *btf,
>> -                                        const struct btf *kern_btf)
>> +                                        struct bpf_object *obj)
> 
> no need to pass obj separately, you can get it through `map->obj`

Good to know!

> 
>>   {
>>          const struct btf_member *member, *kern_member, *kern_data_member;
>>          const struct btf_type *type, *kern_type, *kern_vtype;
>>          __u32 i, kern_type_id, kern_vtype_id, kern_data_off;
>>          struct bpf_struct_ops *st_ops;
>> +       const struct btf *kern_btf;
>> +       struct module_btf *mod_btf;
>> +       const struct btf *btf = obj->btf;
>>          void *data, *kern_data;
>>          const char *tname;
>>          int err;
>> @@ -1006,16 +1051,19 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>>          st_ops = map->st_ops;
>>          type = st_ops->type;
>>          tname = st_ops->tname;
>> -       err = find_struct_ops_kern_types(kern_btf, tname,
>> +       err = find_struct_ops_kern_types(obj, tname, &mod_btf,
>>                                           &kern_type, &kern_type_id,
>>                                           &kern_vtype, &kern_vtype_id,
>>                                           &kern_data_member);
>>          if (err)
>>                  return err;
>>
>> +       kern_btf = mod_btf ? mod_btf->btf : obj->btf_vmlinux;
>> +
>>          pr_debug("struct_ops init_kern %s: type_id:%u kern_type_id:%u kern_vtype_id:%u\n",
>>                   map->name, st_ops->type_id, kern_type_id, kern_vtype_id);
>>
>> +       map->mod_btf = mod_btf;
>>          map->def.value_size = kern_vtype->size;
>>          map->btf_vmlinux_value_type_id = kern_vtype_id;
>>
>> @@ -1091,6 +1139,9 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
>>                                  return -ENOTSUP;
>>                          }
>>
>> +                       /* XXX: attach_btf_obj_fd is needed as well */
> 
> seems like all these XXX comments are outdated and the code is already
> doing all that, is that right? If so, please remove them, they are
> confusing

Sure!

> 
>> +                       if (mod_btf)
>> +                               prog->attach_btf_obj_fd = mod_btf->fd;
>>                          prog->attach_btf_id = kern_type_id;
>>                          prog->expected_attach_type = kern_member_idx;
>>
>> @@ -1133,8 +1184,8 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
>>                  if (!bpf_map__is_struct_ops(map))
>>                          continue;
>>
>> -               err = bpf_map__init_kern_struct_ops(map, obj->btf,
>> -                                                   obj->btf_vmlinux);
>> +               /* XXX: should be a module btf if not vmlinux */
>> +               err = bpf_map__init_kern_struct_ops(map, obj);
>>                  if (err)
>>                          return err;
>>          }
> 
> [...]

Re: [RFC bpf-next v3 07/11] bpf, net: switch to storing struct_ops in btf

[Kui-Feng Lee]

On 9/25/23 17:02, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Use struct_ops registered and stored in module btf instead of static 
>> ones.
>>
>> Both bpf_dummy_ops and bpf_tcp_ca switches to calling the registration
>> function instead of listed in bpf_struct_ops_types.h.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c       | 114 ++++++++++++++++++------------
>>   kernel/bpf/bpf_struct_ops_types.h |  12 ----
>>   net/bpf/bpf_dummy_struct_ops.c    |  12 +++-
>>   net/ipv4/bpf_tcp_ca.c             |  20 +++++-
>>   4 files changed, 94 insertions(+), 64 deletions(-)
>>   delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index fb684d2ee99d..8b5c859377e9 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -59,35 +59,6 @@ static DEFINE_MUTEX(update_mutex);
>>   #define VALUE_PREFIX "bpf_struct_ops_"
>>   #define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
>> -/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
>> - * the map's value exposed to the userspace and its btf-type-id is
>> - * stored at the map->btf_vmlinux_value_type_id.
>> - *
>> - */
>> -#define BPF_STRUCT_OPS_TYPE(_name)                \
>> -extern struct bpf_struct_ops bpf_##_name;            \
>> -                                \
>> -struct bpf_struct_ops_##_name {                        \
>> -    BPF_STRUCT_OPS_COMMON_VALUE;                \
>> -    struct _name data ____cacheline_aligned_in_smp;        \
>> -};
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> -
>> -enum {
>> -#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> -    __NR_BPF_STRUCT_OPS_TYPE,
>> -};
>> -
>> -static struct bpf_struct_ops * const bpf_struct_ops[] = {
>> -#define BPF_STRUCT_OPS_TYPE(_name)                \
>> -    [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> -};
>> -
>>   const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
>>   };
>> @@ -264,14 +235,11 @@ static void bpf_struct_ops_init_one(struct 
>> bpf_struct_ops *st_ops,
>>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>>   {
>> -    struct bpf_struct_ops *st_ops;
>> +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
>> +    extern struct bpf_struct_ops_mod bpf_testmod_struct_ops;
>> +    int ret;
>> +#endif
>>       s32 module_id;
>> -    u32 i;
>> -
>> -    /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
>> -#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct 
>> bpf_struct_ops_##_name);
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>>       module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
>>       if (module_id < 0) {
>> @@ -280,43 +248,95 @@ void bpf_struct_ops_init(struct btf *btf, struct 
>> bpf_verifier_log *log)
>>       }
>>       module_type = btf_type_by_id(btf, module_id);
>> -    for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
>> -        st_ops = bpf_struct_ops[i];
>> -        bpf_struct_ops_init_one(st_ops, btf, log);
>> +#if defined(CONFIG_BPF_JIT) && defined(CONFIG_NET)
>> +    ret = register_bpf_struct_ops(&bpf_testmod_struct_ops);
> 
> What is stopping the 'register_bpf_struct_ops(&bpf_testmod_struct_ops)' 
> to be done in bpf_dummy_struct_ops.c instead of here?
> 

I will remove it from here.

> I am hoping bpf_dummy_struct_ops.c can eventually be moved out to 
> bpf_testmod_struct_ops.c but it is better to leave it as a followup later.
> 
>> +    if (ret)
>> +        pr_warn("Cannot register bpf_testmod_struct_ops\n");
>> +#endif
>> +}
>> +
>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
>> +{
>> +    struct bpf_struct_ops *st_ops = mod->st_ops;
>> +    struct bpf_verifier_log *log;
>> +    struct btf *btf;
>> +    int err;
>> +
>> +    if (mod->st_ops == NULL ||
>> +        mod->owner == NULL)
>> +        return -EINVAL;
>> +
>> +    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
>> +    if (!log) {
>> +        err = -ENOMEM;
>> +        goto errout;
>> +    }
>> +
>> +    log->level = BPF_LOG_KERNEL;
>> +
>> +    btf = btf_get_module_btf(mod->owner);
>> +    if (!btf) {
>> +        err = -EINVAL;
>> +        goto errout;
>>       }
>> +
>> +    bpf_struct_ops_init_one(st_ops, btf, log);
>> +
>> +    btf_put(btf);
>> +
>> +    st_ops->owner = mod->owner;
>> +    err = btf_add_struct_ops(st_ops, st_ops->owner);
>> +
>> +errout:
>> +    kfree(log);
>> +
>> +    return err;
>>   }
>> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
>>   extern struct btf *btf_vmlinux;
>>   static const struct bpf_struct_ops *
>>   bpf_struct_ops_find_value(u32 value_id, struct btf *btf)
>>   {
>> +    const struct bpf_struct_ops *st_ops = NULL;
>> +    const struct bpf_struct_ops **st_ops_list;
>>       unsigned int i;
>> +    u32 cnt = 0;
>>       if (!value_id || !btf_vmlinux)
>>           return NULL;
>> -    for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
>> -        if (bpf_struct_ops[i]->value_id == value_id)
>> -            return bpf_struct_ops[i];
>> +    st_ops_list = btf_get_struct_ops(btf, &cnt);
>> +    for (i = 0; i < cnt; i++) {
>> +        if (st_ops_list[i]->value_id == value_id) {
>> +            st_ops = st_ops_list[i];
>> +            break;
>> +        }
>>       }
>> -    return NULL;
>> +    return st_ops;
>>   }
>>   const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id, struct 
>> btf *btf)
>>   {
>> +    const struct bpf_struct_ops *st_ops = NULL;
>> +    const struct bpf_struct_ops **st_ops_list;
>>       unsigned int i;
>> +    u32 cnt;
>>       if (!type_id || !btf_vmlinux)
>>           return NULL;
>> -    for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
>> -        if (bpf_struct_ops[i]->type_id == type_id)
>> -            return bpf_struct_ops[i];
>> +    st_ops_list = btf_get_struct_ops(btf, &cnt);
>> +    for (i = 0; i < cnt; i++) {
>> +        if (st_ops_list[i]->type_id == type_id) {
>> +            st_ops = st_ops_list[i];
>> +            break;
>> +        }
>>       }
>> -    return NULL;
>> +    return st_ops;
>>   }
>>   static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void 
>> *key,
>> diff --git a/kernel/bpf/bpf_struct_ops_types.h 
>> b/kernel/bpf/bpf_struct_ops_types.h
>> deleted file mode 100644
>> index 5678a9ddf817..000000000000
>> --- a/kernel/bpf/bpf_struct_ops_types.h
>> +++ /dev/null
>> @@ -1,12 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0 */
>> -/* internal file - do not include directly */
>> -
>> -#ifdef CONFIG_BPF_JIT
>> -#ifdef CONFIG_NET
>> -BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
>> -#endif
>> -#ifdef CONFIG_INET
>> -#include <net/tcp.h>
>> -BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
>> -#endif
>> -#endif
>> diff --git a/net/bpf/bpf_dummy_struct_ops.c 
>> b/net/bpf/bpf_dummy_struct_ops.c
>> index 5918d1b32e19..9cb982c67c4c 100644
>> --- a/net/bpf/bpf_dummy_struct_ops.c
>> +++ b/net/bpf/bpf_dummy_struct_ops.c
>> @@ -7,7 +7,7 @@
>>   #include <linux/bpf.h>
>>   #include <linux/btf.h>
>> -extern struct bpf_struct_ops bpf_bpf_dummy_ops;
>> +static struct bpf_struct_ops bpf_bpf_dummy_ops;
>>   /* A common type for test_N with return value in bpf_dummy_ops */
>>   typedef int (*dummy_ops_test_ret_fn)(struct bpf_dummy_ops_state 
>> *state, ...);
>> @@ -218,9 +218,12 @@ static int bpf_dummy_reg(void *kdata)
>>   static void bpf_dummy_unreg(void *kdata)
>>   {
>> +    BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops);
>>   }
>> -struct bpf_struct_ops bpf_bpf_dummy_ops = {
>> +DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_dummy_ops);
>> +
>> +static struct bpf_struct_ops bpf_bpf_dummy_ops = {
>>       .verifier_ops = &bpf_dummy_verifier_ops,
>>       .init = bpf_dummy_init,
>>       .check_member = bpf_dummy_ops_check_member,
>> @@ -229,3 +232,8 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = {
>>       .unreg = bpf_dummy_unreg,
>>       .name = "bpf_dummy_ops",
>>   };
>> +
>> +struct bpf_struct_ops_mod bpf_testmod_struct_ops = {
>> +    .st_ops = &bpf_bpf_dummy_ops,
>> +    .owner = THIS_MODULE,
>> +};
>> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
>> index 39dcccf0f174..9947323f3e22 100644
>> --- a/net/ipv4/bpf_tcp_ca.c
>> +++ b/net/ipv4/bpf_tcp_ca.c
>> @@ -12,7 +12,7 @@
>>   #include <net/bpf_sk_storage.h>
>>   /* "extern" is to avoid sparse warning.  It is only used in 
>> bpf_struct_ops.c. */
>> -extern struct bpf_struct_ops bpf_tcp_congestion_ops;
>> +static struct bpf_struct_ops bpf_tcp_congestion_ops;
>>   static u32 unsupported_ops[] = {
>>       offsetof(struct tcp_congestion_ops, get_info),
>> @@ -271,7 +271,9 @@ static int bpf_tcp_ca_validate(void *kdata)
>>       return tcp_validate_congestion_control(kdata);
>>   }
>> -struct bpf_struct_ops bpf_tcp_congestion_ops = {
>> +DEFINE_STRUCT_OPS_VALUE_TYPE(tcp_congestion_ops);
>> +
>> +static struct bpf_struct_ops bpf_tcp_congestion_ops = {
>>       .verifier_ops = &bpf_tcp_ca_verifier_ops,
>>       .reg = bpf_tcp_ca_reg,
>>       .unreg = bpf_tcp_ca_unreg,
>> @@ -283,8 +285,20 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = {
>>       .name = "tcp_congestion_ops",
>>   };
>> +static struct bpf_struct_ops_mod bpf_tcp_ca_ops_mod = {
>> +    .st_ops = &bpf_tcp_congestion_ops,
>> +    .owner = THIS_MODULE,
>> +};
>> +
>>   static int __init bpf_tcp_ca_kfunc_init(void)
>>   {
>> -    return register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, 
>> &bpf_tcp_ca_kfunc_set);
>> +    int ret;
>> +
>> +    BTF_STRUCT_OPS_TYPE_EMIT(tcp_congestion_ops);
>> +
>> +    ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, 
>> &bpf_tcp_ca_kfunc_set);
>> +    ret = ret ?: register_bpf_struct_ops(&bpf_tcp_ca_ops_mod);
>> +
>> +    return ret;
>>   }
>>   late_initcall(bpf_tcp_ca_kfunc_init);
> 

Re: [RFC bpf-next v3 03/11] bpf: add register and unregister functions for struct_ops.

[Kui-Feng Lee]

On 9/25/23 16:31, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> 
> CONFIG_DEBUG_INFO_BTF_MODULES is probably too restrictive. bpf_tcp_ca 
> does not necessary need CONFIG_DEBUG_INFO_BTF_MODULES
> 
Make sense to me!

>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod);
>> +#endif
> 

Re: [RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs.

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> The type info of a struct_ops type may be in a module.  So, we need to know
> which module BTF to look for type information.  The later patches will make
> libbpf to attach module BTFs to programs. This patch passes attached BTF
> from syscall to bpf_struct_ops subsystem to make sure attached BTF is
> available when the bpf_struct_ops subsystem is ready to use it.
> 
> bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along
> with the bpf_attr loading the program. attach_btf is used to find the btf
> type of attach_btf_id. attach_btf_id is used to identify the traced
> function for a trace program.  For struct_ops programs, it is used to
> identify the struct_ops type of the struct_ops object a program attached
> to.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/uapi/linux/bpf.h       |  4 ++++
>   kernel/bpf/bpf_struct_ops.c    | 12 +++++++++++-
>   kernel/bpf/syscall.c           |  2 +-
>   kernel/bpf/verifier.c          |  4 +++-
>   tools/include/uapi/linux/bpf.h |  4 ++++
>   5 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 73b155e52204..178d6fa45fa0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1390,6 +1390,10 @@ union bpf_attr {
>   		 * to using 5 hash functions).
>   		 */
>   		__u64	map_extra;
> +
> +		__u32   mod_btf_fd;	/* fd pointing to a BTF type data
> +					 * for btf_vmlinux_value_type_id.
> +					 */
>   	};
>   
>   	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 8b5c859377e9..d5600d9ad302 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -765,9 +765,19 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	struct bpf_struct_ops_map *st_map;
>   	const struct btf_type *t, *vt;
>   	struct bpf_map *map;
> +	struct btf *btf;
>   	int ret;
>   
> -	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
> +	/* XXX: We need a module name or ID to find a BTF type. */
> +	/* XXX: should use btf from attr->btf_fd */
> +	if (attr->mod_btf_fd) {
> +		btf = btf_get_by_fd(attr->mod_btf_fd);

The btf is leaked in all cases because it is not stored (and owned) in st_map 
during map_alloc. This circle back to the earlier comment in patch 4 about where 
btf is stored.

> +		if (IS_ERR(btf))
> +			return ERR_PTR(PTR_ERR(btf));
> +	} else {
> +		btf = btf_vmlinux;
> +	}
> +	st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf);
>   	if (!st_ops)
>   		return ERR_PTR(-ENOTSUPP);
>   

Re: [RFC bpf-next v3 08/11] bpf: pass attached BTF to find correct type info of struct_ops progs.

[Kui-Feng Lee]

On 9/25/23 17:24, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> The type info of a struct_ops type may be in a module.  So, we need to 
>> know
>> which module BTF to look for type information.  The later patches will 
>> make
>> libbpf to attach module BTFs to programs. This patch passes attached BTF
>> from syscall to bpf_struct_ops subsystem to make sure attached BTF is
>> available when the bpf_struct_ops subsystem is ready to use it.
>>
>> bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along
>> with the bpf_attr loading the program. attach_btf is used to find the btf
>> type of attach_btf_id. attach_btf_id is used to identify the traced
>> function for a trace program.  For struct_ops programs, it is used to
>> identify the struct_ops type of the struct_ops object a program attached
>> to.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/uapi/linux/bpf.h       |  4 ++++
>>   kernel/bpf/bpf_struct_ops.c    | 12 +++++++++++-
>>   kernel/bpf/syscall.c           |  2 +-
>>   kernel/bpf/verifier.c          |  4 +++-
>>   tools/include/uapi/linux/bpf.h |  4 ++++
>>   5 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 73b155e52204..178d6fa45fa0 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1390,6 +1390,10 @@ union bpf_attr {
>>            * to using 5 hash functions).
>>            */
>>           __u64    map_extra;
>> +
>> +        __u32   mod_btf_fd;    /* fd pointing to a BTF type data
>> +                     * for btf_vmlinux_value_type_id.
>> +                     */
>>       };
>>       struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 8b5c859377e9..d5600d9ad302 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -765,9 +765,19 @@ static struct bpf_map 
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>       struct bpf_struct_ops_map *st_map;
>>       const struct btf_type *t, *vt;
>>       struct bpf_map *map;
>> +    struct btf *btf;
>>       int ret;
>> -    st_ops = 
>> bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
>> +    /* XXX: We need a module name or ID to find a BTF type. */
>> +    /* XXX: should use btf from attr->btf_fd */
>> +    if (attr->mod_btf_fd) {
>> +        btf = btf_get_by_fd(attr->mod_btf_fd);
> 
> The btf is leaked in all cases because it is not stored (and owned) in 
> st_map during map_alloc. This circle back to the earlier comment in 
> patch 4 about where btf is stored.

This has been fixed locally. Basically, map will hold the module instead
of btf.


> 
>> +        if (IS_ERR(btf))
>> +            return ERR_PTR(PTR_ERR(btf));
>> +    } else {
>> +        btf = btf_vmlinux;
>> +    }
>> +    st_ops = 
>> bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf);
>>       if (!st_ops)
>>           return ERR_PTR(-ENOTSUPP);
> 

Re: [RFC bpf-next v3 06/11] bpf: validate value_type

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> A value_type should has three members; refcnt, state, and data.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/bpf_struct_ops.c | 75 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index ef8a1edec891..fb684d2ee99d 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -99,6 +99,79 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
>   
>   static const struct btf_type *module_type;
>   
> +static bool check_value_member(struct btf *btf,
> +			       const struct btf_member *member,
> +			       int index,
> +			       const char *value_name,
> +			       const char *name, const char *type_name,
> +			       u16 kind)
> +{
> +	const char *mname, *mtname;
> +	const struct btf_type *mt;
> +	s32 mtype_id;
> +
> +	mname = btf_name_by_offset(btf, member->name_off);
> +	if (!*mname) {
> +		pr_warn("The member %d of %s should have a name\n",
> +			index, value_name);
> +		return false;
> +	}
> +	if (strcmp(mname, name)) {
> +		pr_warn("The member %d of %s should be refcnt\n",
> +			index, value_name);
> +		return false;
> +	}
> +	mtype_id = member->type;
> +	mt = btf_type_by_id(btf, mtype_id);
> +	mtname = btf_name_by_offset(btf, mt->name_off);
> +	if (!*mtname) {
> +		pr_warn("The type of the member %d of %s should have a name\n",
> +			index, value_name);
> +		return false;
> +	}
> +	if (strcmp(mtname, type_name)) {
> +		pr_warn("The type of the member %d of %s should be refcount_t\n",
> +			index, value_name);
> +		return false;
> +	}
> +	if (btf_kind(mt) != kind) {
> +		pr_warn("The type of the member %d of %s should be %d\n",
> +			index, value_name, btf_kind(mt));
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static bool is_valid_value_type(struct btf *btf, s32 value_id,
> +				const char *type_name, const char *value_name)
> +{
> +	const struct btf_member *member;
> +	const struct btf_type *vt;
> +
> +	vt = btf_type_by_id(btf, value_id);
> +	if (btf_vlen(vt) != 3) {
> +		pr_warn("The number of %s's members should be 3, but we get %d\n",
> +			value_name, btf_vlen(vt));
> +		return false;
> +	}
> +	member = btf_type_member(vt);
> +	if (!check_value_member(btf, member, 0, value_name,
> +				"refcnt", "refcount_t", BTF_KIND_TYPEDEF))
> +		return false;
> +	member++;
> +	if (!check_value_member(btf, member, 1, value_name,
> +				"state", "bpf_struct_ops_state",
> +				BTF_KIND_ENUM))
> +		return false;
> +	member++;

I wonder if giving BPF_STRUCT_OPS_COMMON_VALUE a proper struct will make the 
validation cleaner. Like,

struct bpf_struct_ops_common {
	refcount_t refcnt;
	enum bpf_struct_ops_state state;
};

wdyt?

> +	if (!check_value_member(btf, member, 2, value_name,
> +				"data", type_name, BTF_KIND_STRUCT))

Instead of checking name, I think this can directly check with the st_ops->type.

> +		return false;
> +
> +	return true;
> +}

Re: [RFC bpf-next v3 11/11] selftests/bpf: test case for register_bpf_struct_ops().

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> new file mode 100644
> index 000000000000..8219a477b6d6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> +#include <test_progs.h>
> +
> +#include "struct_ops_module.skel.h"
> +#include "testing_helpers.h"
> +
> +static void test_regular_load(void)
> +{
> +	struct struct_ops_module *skel;
> +	struct bpf_link *link;
> +	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> +	int err;
> +
> +	printf("test_regular_load\n");
> +	skel = struct_ops_module__open_opts(&opts);
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
> +		return;
> +	err = struct_ops_module__load(skel);
> +	if (!ASSERT_OK(err, "struct_ops_module_load"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
> +	ASSERT_OK_PTR(link, "attach_test_mod_1");
> +
> +	ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
> +
> +	bpf_link__destroy(link);
> +
> +	struct_ops_module__destroy(skel);
> +
> +	/* Wait for the map to be freed, or we may fail to unload
> +	 * bpf_testmod.
> +	 */
> +	sleep(1);

Instead of sleep(1), please check if something can be reused from 
kern_sync_rcu_tasks_trace() in map_kptr.c such that the wait can be done 
deterministically. If not, waiting for a fexit trace on bpf_struct_ops_map_free 
probably should work also.

Re: [RFC bpf-next v3 00/11] Registrating struct_ops types from modules

[Martin KaFai Lau]

On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Given the current constraints of the current implementation,
> struct_ops cannot be registered dynamically. This presents a
> significant limitation for modules like coming fuse-bpf, which seeks
> to implement a new struct_ops type. To address this issue, a new API
> is introduced that allows the registration of new struct_ops types
> from modules.
> 
> Previously, struct_ops types were defined in bpf_struct_ops_types.h
> and collected as a static array. The new API lets callers add new
> struct_ops types dynamically. The static array has been removed and
> replaced by the per-btf struct_ops_tab.
> 
> The struct_ops subsystem relies on BTF to determine the layout of
> values in a struct_ops map and identify the subsystem that the
> struct_ops map registers to. However, the kernel BTF does not include
> the type information of struct_ops types defined by a module. The
> struct_ops subsystem requires knowledge of the corresponding module
> for a given struct_ops map and the utilization of BTF information from
> that module. We empower libbpf to determine the correct module for
> accessing the BTF information and pass an identity (FD) of the module
> btf to the kernel. The kernel looks up type information and registered
> struct_ops types directly from the given btf.
> 
> If a module exits while one or more struct_ops maps still refer to a
> struct_ops type defined by the module, it can lead to unforeseen
> complications. Therefore, it is crucial to ensure that a module
> remains intact as long as any struct_ops map is still linked to a
> struct_ops type defined by the module. To achieve this, every
> struct_ops map holds a reference to the module for each struct_ops
> map.

Overall makes sense. The next revision should be in a non-RFC state.

The 'bpftool struct_ops dump' support is also needed.

Please cc the FUSE-bpf members in the next revision.

Please also cc netdev for the patch changing the net/ipv4/bpf_tcp_ca.c.

Thanks.

Re: [RFC bpf-next v3 06/11] bpf: validate value_type

[Kui-Feng Lee]

On 9/25/23 18:03, Martin KaFai Lau wrote:
> On 9/20/23 8:59 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> A value_type should has three members; refcnt, state, and data.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 75 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index ef8a1edec891..fb684d2ee99d 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -99,6 +99,79 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
>>   static const struct btf_type *module_type;
>> +static bool check_value_member(struct btf *btf,
>> +                   const struct btf_member *member,
>> +                   int index,
>> +                   const char *value_name,
>> +                   const char *name, const char *type_name,
>> +                   u16 kind)
>> +{
>> +    const char *mname, *mtname;
>> +    const struct btf_type *mt;
>> +    s32 mtype_id;
>> +
>> +    mname = btf_name_by_offset(btf, member->name_off);
>> +    if (!*mname) {
>> +        pr_warn("The member %d of %s should have a name\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (strcmp(mname, name)) {
>> +        pr_warn("The member %d of %s should be refcnt\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    mtype_id = member->type;
>> +    mt = btf_type_by_id(btf, mtype_id);
>> +    mtname = btf_name_by_offset(btf, mt->name_off);
>> +    if (!*mtname) {
>> +        pr_warn("The type of the member %d of %s should have a name\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (strcmp(mtname, type_name)) {
>> +        pr_warn("The type of the member %d of %s should be 
>> refcount_t\n",
>> +            index, value_name);
>> +        return false;
>> +    }
>> +    if (btf_kind(mt) != kind) {
>> +        pr_warn("The type of the member %d of %s should be %d\n",
>> +            index, value_name, btf_kind(mt));
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static bool is_valid_value_type(struct btf *btf, s32 value_id,
>> +                const char *type_name, const char *value_name)
>> +{
>> +    const struct btf_member *member;
>> +    const struct btf_type *vt;
>> +
>> +    vt = btf_type_by_id(btf, value_id);
>> +    if (btf_vlen(vt) != 3) {
>> +        pr_warn("The number of %s's members should be 3, but we get 
>> %d\n",
>> +            value_name, btf_vlen(vt));
>> +        return false;
>> +    }
>> +    member = btf_type_member(vt);
>> +    if (!check_value_member(btf, member, 0, value_name,
>> +                "refcnt", "refcount_t", BTF_KIND_TYPEDEF))
>> +        return false;
>> +    member++;
>> +    if (!check_value_member(btf, member, 1, value_name,
>> +                "state", "bpf_struct_ops_state",
>> +                BTF_KIND_ENUM))
>> +        return false;
>> +    member++;
> 
> I wonder if giving BPF_STRUCT_OPS_COMMON_VALUE a proper struct will make 
> the validation cleaner. Like,
> 
> struct bpf_struct_ops_common {
>      refcount_t refcnt;
>      enum bpf_struct_ops_state state;
> };
> 
> wdyt?

It should work.

> 
>> +    if (!check_value_member(btf, member, 2, value_name,
>> +                "data", type_name, BTF_KIND_STRUCT))
> 
> Instead of checking name, I think this can directly check with the 
> st_ops->type.

Make sense

> 
>> +        return false;
>> +
>> +    return true;
>> +}
> 
>