bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules
@ 2023-10-17 16:22 thinker.li
  2023-10-17 16:22 ` [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function thinker.li
                   ` (8 more replies)
  0 siblings, 9 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:22 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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 v4:

 - Fix the dependency between testing_helpers.o and
   rcu_tasks_trace_gp.skel.h.

Changes from v3:

 - Fix according to the feedback for v3.

   - Change of the order of arguments to make btf as the first
     argument.

   - Use btf_try_get_module() instead of try_get_module() since the
     module pointed by st_ops->owner can gone while some one is still
     holding its btf.

   - Move variables defined by BPF_STRUCT_OPS_COMMON_VALUE to struct
     bpf_struct_ops_common_value to validation easier.

   - Register the struct_ops type defined by bpf_testmod in its init
     function.

   - Rename field name to 'value_type_btf_obj_fd' to make it explicit.

   - Fix leaking of btf objects on error.

   - st_maps hold their modules to keep modules alive and prevent they
     from unloading.

   - bpf_map of libbpf keeps mod_btf_fd instead of a pointer to module_btf.

   - Do call_rcu_tasks_trace() in kern_sync_rcu() to ensure the
     bpf_testmod is unloaded properly. It uses rcu_tasks_trace_gp to
     trigger call_rcu_tasks_trace() in the kernel.

 - Merge and reorder patches in a reasonable order.

 - 

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.

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

Kui-Feng Lee (9):
  bpf: refactory struct_ops type initialization to a function.
  bpf: add struct_ops_tab to btf.
  bpf: hold module for bpf_struct_ops_map.
  bpf: validate value_type
  bpf: pass attached BTF to the bpf_struct_ops subsystem
  bpf, net: switch to dynamic registration
  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                           |   8 +-
 include/linux/btf.h                           |  35 ++
 include/uapi/linux/bpf.h                      |   5 +
 kernel/bpf/bpf_struct_ops.c                   | 375 +++++++++++-------
 kernel/bpf/bpf_struct_ops_types.h             |  12 -
 kernel/bpf/btf.c                              |  73 +++-
 kernel/bpf/syscall.c                          |   2 +-
 kernel/bpf/verifier.c                         |   4 +-
 net/bpf/bpf_dummy_struct_ops.c                |  14 +-
 net/ipv4/bpf_tcp_ca.c                         |  16 +-
 tools/include/uapi/linux/bpf.h                |   5 +
 tools/lib/bpf/bpf.c                           |   4 +-
 tools/lib/bpf/bpf.h                           |   5 +-
 tools/lib/bpf/libbpf.c                        |  68 ++--
 tools/testing/selftests/bpf/Makefile          |   2 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  59 +++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |   5 +
 .../bpf/prog_tests/test_struct_ops_module.c   |  38 ++
 .../selftests/bpf/progs/struct_ops_module.c   |  30 ++
 tools/testing/selftests/bpf/testing_helpers.c |  35 ++
 20 files changed, 604 insertions(+), 191 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function.
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
@ 2023-10-17 16:22 ` thinker.li
  2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:22 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

Move the majority of the code to bpf_struct_ops_init_one(), which can then
be utilized for the initialization of newly registered dynamically
allocated struct_ops types in the following patches.

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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
  2023-10-17 16:22 ` [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function thinker.li
@ 2023-10-17 16:22 ` thinker.li
  2023-10-19  0:00   ` Martin KaFai Lau
  2023-10-19  2:28   ` Martin KaFai Lau
  2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
                   ` (6 subsequent siblings)
  8 siblings, 2 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:22 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

Maintain a registry of registered struct_ops types in the per-btf (module)
struct_ops_tab. This registry allows for easy lookup of struct_ops types
that are registered by a specific module.

Every struct_ops type should have an associated module BTF to provide type
information since we are going to allow modules to define and register new
struct_ops types. Once this change is made, the bpf_struct_ops subsystem
knows where to look up type info with just a bpf_struct_ops.

The subsystem looks up struct_ops types from a given module BTF although it
is always btf_vmlinux now. Once start using struct_ops_tab, btfs other than
btf_vmlinux can be used as well.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/bpf.h         |  5 +--
 include/linux/btf.h         |  6 ++++
 kernel/bpf/bpf_struct_ops.c | 17 ++++-----
 kernel/bpf/btf.c            | 70 +++++++++++++++++++++++++++++++++++++
 kernel/bpf/verifier.c       |  2 +-
 5 files changed, 89 insertions(+), 11 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 30063a760b5a..e6a648af2daa 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);
+	struct btf *btf;
 	const struct btf_type *type;
 	const struct btf_type *value_type;
 	const char *name;
@@ -1636,7 +1637,7 @@ struct bpf_struct_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);
+const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id);
 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);
@@ -1679,7 +1680,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(struct btf *btf, u32 type_id)
 {
 	return NULL;
 }
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 928113a80a95..aa2ba77648be 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -571,4 +571,10 @@ 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(struct bpf_struct_ops *st_ops);
+const struct bpf_struct_ops **
+btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
+
 #endif
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 627cf1ea840a..7758f66ad734 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -185,6 +185,7 @@ 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 {
+			st_ops->btf = btf;
 			st_ops->type_id = type_id;
 			st_ops->type = t;
 			st_ops->value_id = value_id;
@@ -221,7 +222,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(struct btf *btf, u32 value_id)
 {
 	unsigned int i;
 
@@ -236,7 +237,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(struct btf *btf, u32 type_id)
 {
 	unsigned int i;
 
@@ -316,7 +317,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;
@@ -328,8 +329,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;
@@ -395,12 +396,12 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	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;
 
@@ -671,7 +672,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/btf.c b/kernel/bpf/btf.c
index f93e835d90af..be5144dbb53d 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,57 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
 
 	return !strncmp(reg_name, arg_name, cmp_len);
 }
+
+int btf_add_struct_ops(struct bpf_struct_ops *st_ops)
+{
+	struct btf_struct_ops_tab *tab, *new_tab;
+	struct btf *btf = st_ops->btf;
+	int i;
+
+	if (!btf)
+		return -ENOENT;
+
+	/* Assume this function is called for a module when the module is
+	 * loading.
+	 */
+
+	tab = btf->struct_ops_tab;
+	if (!tab) {
+		tab = kzalloc(offsetof(struct btf_struct_ops_tab, 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) {
+		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;
+}
+
+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;
+}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a7178ecf676d..6564a03c425d 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_vmlinux, btf_id);
 	if (!st_ops) {
 		verbose(env, "attach_btf_id %u is not a supported struct\n",
 			btf_id);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
  2023-10-17 16:22 ` [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function thinker.li
  2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
@ 2023-10-17 16:23 ` thinker.li
  2023-10-19  0:36   ` Martin KaFai Lau
  2023-10-17 16:23 ` [PATCH bpf-next v5 4/9] bpf: validate value_type thinker.li
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

To ensure that a module remains accessible whenever a struct_ops object of
a struct_ops type provided by the module is still in use.

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

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e6a648af2daa..1e1647c8b0ce 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);
 	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 7758f66ad734..b561245fe235 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -112,6 +112,7 @@ static const struct btf_type *module_type;
 
 static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 				    struct btf *btf,
+				    struct module *owner,
 				    struct bpf_verifier_log *log)
 {
 	const struct btf_member *member;
@@ -186,6 +187,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 				st_ops->name);
 		} else {
 			st_ops->btf = btf;
+			st_ops->owner = owner;
 			st_ops->type_id = type_id;
 			st_ops->type = t;
 			st_ops->value_id = value_id;
@@ -193,6 +195,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 							    value_id);
 		}
 	}
+
 }
 
 void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
@@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 
 	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);
+		bpf_struct_ops_init_one(st_ops, btf, NULL, log);
 	}
 }
 
@@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
 		bpf_jit_uncharge_modmem(PAGE_SIZE);
 	}
 	bpf_map_area_free(st_map->uvalue);
+	module_put(st_map->st_ops->owner);
 	bpf_map_area_free(st_map);
 }
 
@@ -676,9 +680,18 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 	if (!st_ops)
 		return ERR_PTR(-ENOTSUPP);
 
+	/* If st_ops->owner is NULL, it means the struct_ops is
+	 * statically defined in the kernel.  We don't need to
+	 * take a refcount on it.
+	 */
+	if (st_ops->owner && !btf_try_get_module(st_ops->btf))
+		return ERR_PTR(-EINVAL);
+
 	vt = st_ops->value_type;
-	if (attr->value_size != vt->size)
+	if (attr->value_size != vt->size) {
+		module_put(st_ops->owner);
 		return ERR_PTR(-EINVAL);
+	}
 
 	t = st_ops->type;
 
@@ -689,8 +702,10 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 		(vt->size - sizeof(struct bpf_struct_ops_value));
 
 	st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
-	if (!st_map)
+	if (!st_map) {
+		module_put(st_ops->owner);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	st_map->st_ops = st_ops;
 	map = &st_map->map;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 4/9] bpf: validate value_type
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
                   ` (2 preceding siblings ...)
  2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
@ 2023-10-17 16:23 ` thinker.li
  2023-10-17 16:23 ` [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

A value_type should consist of three components: refcnt, state, and data.
refcnt and state has been move to struct bpf_struct_ops_common_value to
make it easier to check the value type.

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

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index b561245fe235..69703584fa4a 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -20,9 +20,11 @@ enum bpf_struct_ops_state {
 	BPF_STRUCT_OPS_STATE_READY,
 };
 
-#define BPF_STRUCT_OPS_COMMON_VALUE			\
-	refcount_t refcnt;				\
-	enum bpf_struct_ops_state state
+struct bpf_struct_ops_common_value {
+	refcount_t refcnt;
+	enum bpf_struct_ops_state state;
+};
+#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
 
 struct bpf_struct_ops_value {
 	BPF_STRUCT_OPS_COMMON_VALUE;
@@ -109,6 +111,38 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
 };
 
 static const struct btf_type *module_type;
+static const struct btf_type *common_value_type;
+
+static bool is_valid_value_type(struct btf *btf, s32 value_id,
+				const struct btf_type *type,
+				const char *value_name)
+{
+	const struct btf_member *member;
+	const struct btf_type *vt, *mt;
+
+	vt = btf_type_by_id(btf, value_id);
+	if (btf_vlen(vt) != 2) {
+		pr_warn("The number of %s's members should be 2, but we get %d\n",
+			value_name, btf_vlen(vt));
+		return false;
+	}
+	member = btf_type_member(vt);
+	mt = btf_type_by_id(btf, member->type);
+	if (mt != common_value_type) {
+		pr_warn("The first member of %s should be bpf_struct_ops_common_value\n",
+			value_name);
+		return false;
+	}
+	member++;
+	mt = btf_type_by_id(btf, member->type);
+	if (mt != type) {
+		pr_warn("The second member of %s should be %s\n",
+			value_name, btf_name_by_offset(btf, type->name_off));
+		return false;
+	}
+
+	return true;
+}
 
 static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 				    struct btf *btf,
@@ -130,14 +164,6 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 	}
 	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);
-		return;
-	}
-
 	type_id = btf_find_by_name_kind(btf, st_ops->name,
 					BTF_KIND_STRUCT);
 	if (type_id < 0) {
@@ -152,6 +178,16 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 		return;
 	}
 
+	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;
+	}
+	if (!is_valid_value_type(btf, value_id, t, value_name))
+		return;
+
 	for_each_member(i, t, member) {
 		const struct btf_type *func_proto;
 
@@ -201,7 +237,7 @@ 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;
-	s32 module_id;
+	s32 module_id, common_value_id;
 	u32 i;
 
 	/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
@@ -215,6 +251,14 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 		return;
 	}
 	module_type = btf_type_by_id(btf, module_id);
+	common_value_id = btf_find_by_name_kind(btf,
+						"bpf_struct_ops_common_value",
+						BTF_KIND_STRUCT);
+	if (common_value_id < 0) {
+		pr_warn("Cannot find struct common_value in btf_vmlinux\n");
+		return;
+	}
+	common_value_type = btf_type_by_id(btf, common_value_id);
 
 	for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
 		st_ops = bpf_struct_ops[i];
@@ -278,7 +322,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
 
 	kvalue = &st_map->kvalue;
 	/* Pair with smp_store_release() during map_update */
-	state = smp_load_acquire(&kvalue->state);
+	state = smp_load_acquire(&kvalue->common.state);
 	if (state == BPF_STRUCT_OPS_STATE_INIT) {
 		memset(value, 0, map->value_size);
 		return 0;
@@ -289,7 +333,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
 	 */
 	uvalue = value;
 	memcpy(uvalue, st_map->uvalue, map->value_size);
-	uvalue->state = state;
+	uvalue->common.state = state;
 
 	/* This value offers the user space a general estimate of how
 	 * many sockets are still utilizing this struct_ops for TCP
@@ -297,7 +341,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
 	 * should sufficiently meet our present goals.
 	 */
 	refcnt = atomic64_read(&map->refcnt) - atomic64_read(&map->usercnt);
-	refcount_set(&uvalue->refcnt, max_t(s64, refcnt, 0));
+	refcount_set(&uvalue->common.refcnt, max_t(s64, refcnt, 0));
 
 	return 0;
 }
@@ -408,7 +452,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 	if (err)
 		return err;
 
-	if (uvalue->state || refcount_read(&uvalue->refcnt))
+	if (uvalue->common.state || refcount_read(&uvalue->common.refcnt))
 		return -EINVAL;
 
 	tlinks = kcalloc(BPF_TRAMP_MAX, sizeof(*tlinks), GFP_KERNEL);
@@ -420,7 +464,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 
 	mutex_lock(&st_map->lock);
 
-	if (kvalue->state != BPF_STRUCT_OPS_STATE_INIT) {
+	if (kvalue->common.state != BPF_STRUCT_OPS_STATE_INIT) {
 		err = -EBUSY;
 		goto unlock;
 	}
@@ -533,7 +577,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		 *
 		 * Pair with smp_load_acquire() during lookup_elem().
 		 */
-		smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_READY);
+		smp_store_release(&kvalue->common.state, BPF_STRUCT_OPS_STATE_READY);
 		goto unlock;
 	}
 
@@ -551,7 +595,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		 * It ensures the above udata updates (e.g. prog->aux->id)
 		 * can be seen once BPF_STRUCT_OPS_STATE_INUSE is set.
 		 */
-		smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_INUSE);
+		smp_store_release(&kvalue->common.state, BPF_STRUCT_OPS_STATE_INUSE);
 		goto unlock;
 	}
 
@@ -582,7 +626,7 @@ static long bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
 	if (st_map->map.map_flags & BPF_F_LINK)
 		return -EOPNOTSUPP;
 
-	prev_state = cmpxchg(&st_map->kvalue.state,
+	prev_state = cmpxchg(&st_map->kvalue.common.state,
 			     BPF_STRUCT_OPS_STATE_INUSE,
 			     BPF_STRUCT_OPS_STATE_TOBEFREE);
 	switch (prev_state) {
@@ -676,7 +720,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, btf_vmlinux);
+	st_ops = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
 	if (!st_ops)
 		return ERR_PTR(-ENOTSUPP);
 
@@ -805,7 +849,7 @@ static bool bpf_struct_ops_valid_to_reg(struct bpf_map *map)
 	return map->map_type == BPF_MAP_TYPE_STRUCT_OPS &&
 		map->map_flags & BPF_F_LINK &&
 		/* Pair with smp_store_release() during map_update */
-		smp_load_acquire(&st_map->kvalue.state) == BPF_STRUCT_OPS_STATE_READY;
+		smp_load_acquire(&st_map->kvalue.common.state) == BPF_STRUCT_OPS_STATE_READY;
 }
 
 static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
                   ` (3 preceding siblings ...)
  2023-10-17 16:23 ` [PATCH bpf-next v5 4/9] bpf: validate value_type thinker.li
@ 2023-10-17 16:23 ` thinker.li
  2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

Giving a BTF, the bpf_struct_ops knows the right place to look up type info
associated with a type ID. This enables a user space program to load a
struct_ops object linked to a struct_ops type defined by a module, by
providing the module BTF (fd).

The bpf_prog includes attach_btf in aux which is passed along with the
bpf_attr when loading the program. The purpose of attach_btf is to
determine the btf type of attach_btf_id. The attach_btf_id is then used to
identify the traced function for a trace program. In the case of struct_ops
programs, it is used to identify the struct_ops type of the struct_ops
object that a program is attached to.

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

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 73b155e52204..b5ef22f65f35 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1390,6 +1390,11 @@ union bpf_attr {
 		 * to using 5 hash functions).
 		 */
 		__u64	map_extra;
+
+		__u32   value_type_btf_obj_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 69703584fa4a..60445ff32275 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -677,6 +677,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
 		bpf_jit_uncharge_modmem(PAGE_SIZE);
 	}
 	bpf_map_area_free(st_map->uvalue);
+	btf_put(st_map->st_ops->btf);
 	module_put(st_map->st_ops->owner);
 	bpf_map_area_free(st_map);
 }
@@ -718,23 +719,36 @@ 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(btf_vmlinux, attr->btf_vmlinux_value_type_id);
-	if (!st_ops)
+	if (attr->value_type_btf_obj_fd) {
+		btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+		if (IS_ERR(btf))
+			return ERR_PTR(PTR_ERR(btf));
+	} else {
+		btf = btf_vmlinux;
+		btf_get(btf);
+	}
+	st_ops = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
+	if (!st_ops) {
+		btf_put(btf);
 		return ERR_PTR(-ENOTSUPP);
+	}
 
 	/* If st_ops->owner is NULL, it means the struct_ops is
 	 * statically defined in the kernel.  We don't need to
 	 * take a refcount on it.
 	 */
-	if (st_ops->owner && !btf_try_get_module(st_ops->btf))
+	if (st_ops->owner && !btf_try_get_module(st_ops->btf)) {
+		btf_put(btf);
 		return ERR_PTR(-EINVAL);
+	}
 
 	vt = st_ops->value_type;
 	if (attr->value_size != vt->size) {
-		module_put(st_ops->owner);
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto errout;
 	}
 
 	t = st_ops->type;
@@ -747,8 +761,8 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 
 	st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
 	if (!st_map) {
-		module_put(st_ops->owner);
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto errout;
 	}
 
 	st_map->st_ops = st_ops;
@@ -784,6 +798,12 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 	bpf_map_init_from_attr(map, attr);
 
 	return map;
+
+errout:
+	btf_put(btf);
+	module_put(st_ops->owner);
+
+	return ERR_PTR(ret);
 }
 
 static u64 bpf_struct_ops_map_mem_usage(const struct bpf_map *map)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 85c1d908f70f..5daf8a2c2bba 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 value_type_btf_obj_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 6564a03c425d..ce4df24eb03b 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_vmlinux, btf_id);
+	st_ops = bpf_struct_ops_find(btf, btf_id);
 	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..b5ef22f65f35 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1390,6 +1390,11 @@ union bpf_attr {
 		 * to using 5 hash functions).
 		 */
 		__u64	map_extra;
+
+		__u32   value_type_btf_obj_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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
                   ` (4 preceding siblings ...)
  2023-10-17 16:23 ` [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
@ 2023-10-17 16:23 ` thinker.li
  2023-10-19  1:49   ` Martin KaFai Lau
  2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee, netdev

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

Replace the static list of struct_ops types with pre-btf struct_ops_tab to
enable dynamic registration.

Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function
instead of being listed in bpf_struct_ops_types.h.

Cc: netdev@vger.kernel.org
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/bpf.h               |   2 +
 include/linux/btf.h               |  29 +++++++
 kernel/bpf/bpf_struct_ops.c       | 124 +++++++++++++++---------------
 kernel/bpf/bpf_struct_ops_types.h |  12 ---
 kernel/bpf/btf.c                  |   2 +-
 net/bpf/bpf_dummy_struct_ops.c    |  14 +++-
 net/ipv4/bpf_tcp_ca.c             |  16 +++-
 7 files changed, 119 insertions(+), 80 deletions(-)
 delete mode 100644 kernel/bpf/bpf_struct_ops_types.h

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1e1647c8b0ce..b0f33147aa93 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
 	return prog->aux->func_idx != 0;
 }
 
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
+
 #endif /* _LINUX_BPF_H */
diff --git a/include/linux/btf.h b/include/linux/btf.h
index aa2ba77648be..fdc83aa10462 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,
@@ -577,4 +580,30 @@ 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,
+};
+
+struct bpf_struct_ops_common_value {
+	refcount_t refcnt;
+	enum bpf_struct_ops_state state;
+};
+#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
+
+/* 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 60445ff32275..175068b083cb 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -13,19 +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,
-};
-
-struct bpf_struct_ops_common_value {
-	refcount_t refcnt;
-	enum bpf_struct_ops_state state;
-};
-#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
-
 struct bpf_struct_ops_value {
 	BPF_STRUCT_OPS_COMMON_VALUE;
 	char data[] ____cacheline_aligned_in_smp;
@@ -72,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 = {
 };
 
@@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
 
 }
 
+static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
+				       struct btf *btf)
+{
+	struct bpf_verifier_log *log;
+	int err;
+
+	if (st_ops == NULL)
+		return -EINVAL;
+
+	log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+	if (!log) {
+		err = -ENOMEM;
+		goto errout;
+	}
+
+	log->level = BPF_LOG_KERNEL;
+
+	bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
+
+	err = btf_add_struct_ops(st_ops);
+
+errout:
+	kfree(log);
+
+	return err;
+}
+
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
+{
+	struct btf *btf;
+	int err;
+
+	btf = btf_get_module_btf(st_ops->owner);
+	if (!btf)
+		return -EINVAL;
+	err = register_bpf_struct_ops_btf(st_ops, btf);
+	btf_put(btf);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
+
 void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 {
-	struct bpf_struct_ops *st_ops;
 	s32 module_id, common_value_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) {
@@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 		return;
 	}
 	common_value_type = btf_type_by_id(btf, common_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, NULL, log);
-	}
 }
 
 extern struct btf *btf_vmlinux;
@@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;
 static const struct bpf_struct_ops *
 bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
 {
+	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(struct btf *btf, u32 type_id)
 {
+	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/kernel/bpf/btf.c b/kernel/bpf/btf.c
index be5144dbb53d..990973d6057d 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;
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 5918d1b32e19..724bb7224079 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, ...);
@@ -216,11 +216,13 @@ static int bpf_dummy_reg(void *kdata)
 	return -EOPNOTSUPP;
 }
 
+DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_dummy_ops);
+
 static void bpf_dummy_unreg(void *kdata)
 {
 }
 
-struct bpf_struct_ops bpf_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,
@@ -228,4 +230,12 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = {
 	.reg = bpf_dummy_reg,
 	.unreg = bpf_dummy_unreg,
 	.name = "bpf_dummy_ops",
+	.owner = THIS_MODULE,
 };
+
+static int __init bpf_dummy_struct_ops_init(void)
+{
+	BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops);
+	return register_bpf_struct_ops(&bpf_bpf_dummy_ops);
+}
+late_initcall(bpf_dummy_struct_ops_init);
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 39dcccf0f174..20c401c73dfb 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,
@@ -281,10 +283,18 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = {
 	.init = bpf_tcp_ca_init,
 	.validate = bpf_tcp_ca_validate,
 	.name = "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_congestion_ops);
+
+	return ret;
 }
 late_initcall(bpf_tcp_ca_kfunc_init);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
                   ` (5 preceding siblings ...)
  2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
@ 2023-10-17 16:23 ` thinker.li
  2023-10-17 21:49   ` Andrii Nakryiko
  2023-10-19  2:43   ` Martin KaFai Lau
  2023-10-17 16:23 ` [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules thinker.li
  2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
  8 siblings, 2 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

Locate the module BTFs for struct_ops maps and progs and pass them to the
kernel. This ensures that the kernel correctly resolves type IDs from the
appropriate 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. The FD of the module BTF is passed to the
kernel as mod_btf_fd when the struct_ops object is loaded.

For a bpf_struct_ops prog, attach_btf_obj_fd of bpf_prog is the FD of a
module BTF in the kernel.

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

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index b0f1913763a3..af46488e4ea9 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -169,7 +169,8 @@ 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,
+					   value_type_btf_obj_fd);
 	union bpf_attr attr;
 	int fd;
 
@@ -191,6 +192,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.value_type_btf_obj_fd = OPTS_GET(opts, value_type_btf_obj_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..1733cdc21241 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -51,8 +51,11 @@ struct bpf_map_create_opts {
 
 	__u32 numa_node;
 	__u32 map_ifindex;
+
+	__u32 value_type_btf_obj_fd;
+	size_t:0;
 };
-#define bpf_map_create_opts__last_field map_ifindex
+#define bpf_map_create_opts__last_field value_type_btf_obj_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..d8a60fb52f5c 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;
+	int mod_btf_fd;
 	__u32 btf_key_type_id;
 	__u32 btf_value_type_id;
 	__u32 btf_vmlinux_value_type_id;
@@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
 	return 0;
 }
 
+static int load_module_btfs(struct bpf_object *obj);
+
 static const struct btf_member *
 find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
 {
@@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
 	return NULL;
 }
 
+static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
+			      __u16 kind, struct btf **res_btf,
+			      struct module_btf **res_mod_btf);
+
 #define STRUCT_OPS_VALUE_PREFIX "bpf_struct_ops_"
 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);
+	kern_type_id = find_module_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);
@@ -991,14 +1001,16 @@ 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)
+static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 {
 	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_object *obj = map->obj;
+	const struct btf *btf = obj->btf;
 	struct bpf_struct_ops *st_ops;
+	const struct btf *kern_btf;
+	struct module_btf *mod_btf;
 	void *data, *kern_data;
 	const char *tname;
 	int err;
@@ -1006,16 +1018,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_fd = mod_btf ? mod_btf->fd : 0;
 	map->def.value_size = kern_vtype->size;
 	map->btf_vmlinux_value_type_id = kern_vtype_id;
 
@@ -1091,6 +1106,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
 				return -ENOTSUP;
 			}
 
+			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 +1150,7 @@ 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);
+		err = bpf_map__init_kern_struct_ops(map);
 		if (err)
 			return err;
 	}
@@ -5193,8 +5209,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.value_type_btf_obj_fd = map->mod_btf_fd;
+	}
 
 	if (obj->btf && btf__fd(obj->btf) >= 0) {
 		create_attr.btf_fd = btf__fd(obj->btf);
@@ -7700,9 +7718,9 @@ 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)
+static int find_module_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;
@@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
 
 	btf = obj->btf_vmlinux;
 	mod_btf = NULL;
-	id = btf__find_by_name_kind(btf, ksym_name, kind);
+	id = btf__find_by_name_kind(btf, kern_name, kind);
 
 	if (id == -ENOENT) {
 		err = load_module_btfs(obj);
@@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
 			/* 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);
+			id = btf__find_by_name_kind_own(btf, kern_name, kind);
 			if (id != -ENOENT)
 				break;
 		}
@@ -7744,7 +7762,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_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
 	if (id < 0) {
 		if (id == -ESRCH && ext->is_weak)
 			return 0;
@@ -7798,8 +7816,8 @@ 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,
-				    &mod_btf);
+	kfunc_id = find_module_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)
 			return 0;
@@ -9464,9 +9482,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 +9549,9 @@ 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 +12965,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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules.
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
                   ` (6 preceding siblings ...)
  2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
@ 2023-10-17 16:23 ` thinker.li
  2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
  8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

The module requires the use of btf_ctx_access() to invoke
bpf_tracing_btf_ctx_access() from a module. This function is valuable for
implementing validation functions that ensure proper access to ctx.

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 990973d6057d..dc6d8fe9b63d 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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops().
  2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
                   ` (7 preceding siblings ...)
  2023-10-17 16:23 ` [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules thinker.li
@ 2023-10-17 16:23 ` thinker.li
  2023-10-17 18:03   ` kernel test robot
  8 siblings, 1 reply; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: sinquersw, kuifeng, Kui-Feng Lee

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

Create a new struct_ops type called bpf_testmod_ops within the bpf_testmod
module. When a struct_ops object is registered, the bpf_testmod module will
invoke test_2 from the module.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/testing/selftests/bpf/Makefile          |  2 +
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 59 +++++++++++++++++++
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |  5 ++
 .../bpf/prog_tests/test_struct_ops_module.c   | 38 ++++++++++++
 .../selftests/bpf/progs/struct_ops_module.c   | 30 ++++++++++
 tools/testing/selftests/bpf/testing_helpers.c | 35 +++++++++++
 6 files changed, 169 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/Makefile b/tools/testing/selftests/bpf/Makefile
index caede9b574cb..dd7ff14e1fdf 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -706,6 +706,8 @@ $(OUTPUT)/uprobe_multi: uprobe_multi.c
 	$(call msg,BINARY,,$@)
 	$(Q)$(CC) $(CFLAGS) $(LDFLAGS) $^ $(LDLIBS) -o $@
 
+$(OUTPUT)/testing_helpers.o: $(OUTPUT)/rcu_tasks_trace_gp.skel.h
+
 EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)	\
 	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
 	feature bpftool							\
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index cefc5dd72573..f1a20669d884 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,66 @@ 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);
+
+	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",
+	.owner = THIS_MODULE,
+};
+
+#endif /* CONFIG_DEBUG_INFO_BTF_MODULES */
+
 extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
@@ -532,6 +588,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_bpf_testmod_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..7261fc6c377a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "rcu_tasks_trace_gp.skel.h"
+#include "struct_ops_module.skel.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;
+
+	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);
+}
+
+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,
+};
+
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 8d994884c7b4..05870cd62458 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -10,6 +10,7 @@
 #include "test_progs.h"
 #include "testing_helpers.h"
 #include <linux/membarrier.h>
+#include "rcu_tasks_trace_gp.skel.h"
 
 int parse_num_list(const char *s, bool **num_set, int *num_set_len)
 {
@@ -380,10 +381,44 @@ int load_bpf_testmod(bool verbose)
 	return 0;
 }
 
+/* This function will trigger call_rcu_tasks_trace() in the kernel */
+static int kern_sync_rcu_tasks_trace(void)
+{
+	struct rcu_tasks_trace_gp *rcu;
+	time_t start;
+	long gp_seq;
+	LIBBPF_OPTS(bpf_test_run_opts, opts);
+
+	rcu = rcu_tasks_trace_gp__open_and_load();
+	if (IS_ERR(rcu))
+		return -EFAULT;
+	if (rcu_tasks_trace_gp__attach(rcu))
+		return -EFAULT;
+
+	gp_seq = READ_ONCE(rcu->bss->gp_seq);
+
+	if (bpf_prog_test_run_opts(bpf_program__fd(rcu->progs.do_call_rcu_tasks_trace),
+				   &opts))
+		return -EFAULT;
+	if (opts.retval != 0)
+		return -EFAULT;
+
+	start = time(NULL);
+	while ((start + 2) > time(NULL) &&
+	       gp_seq == READ_ONCE(rcu->bss->gp_seq))
+		sched_yield();
+
+	rcu_tasks_trace_gp__destroy(rcu);
+
+	return 0;
+}
+
 /*
  * Trigger synchronize_rcu() in kernel.
  */
 int kern_sync_rcu(void)
 {
+	if (kern_sync_rcu_tasks_trace())
+		return -EFAULT;
 	return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0);
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops().
  2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
@ 2023-10-17 18:03   ` kernel test robot
  0 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2023-10-17 18:03 UTC (permalink / raw)
  To: thinker.li, bpf, ast, martin.lau, song, kernel-team, andrii, drosen
  Cc: oe-kbuild-all

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/thinker-li-gmail-com/bpf-refactory-struct_ops-type-initialization-to-a-function/20231018-002613
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20231017162306.176586-10-thinker.li%40gmail.com
patch subject: [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops().
reproduce: (https://download.01.org/0day-ci/archive/20231018/202310180128.xAuYnY3h-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310180128.xAuYnY3h-lkp@intel.com/

# many are suggestions rather than must-fix

WARNING:LINE_SPACING: Missing a blank line after declarations
#239: FILE: tools/testing/selftests/bpf/testing_helpers.c:390:
+	long gp_seq;
+	LIBBPF_OPTS(bpf_test_run_opts, opts);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
  2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
@ 2023-10-17 21:49   ` Andrii Nakryiko
  2023-10-18  2:25     ` Kui-Feng Lee
  2023-10-19  2:43   ` Martin KaFai Lau
  1 sibling, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2023-10-17 21:49 UTC (permalink / raw)
  To: thinker.li
  Cc: bpf, ast, martin.lau, song, kernel-team, andrii, drosen,
	sinquersw, kuifeng

On Tue, Oct 17, 2023 at 9:23 AM <thinker.li@gmail.com> wrote:
>
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Locate the module BTFs for struct_ops maps and progs and pass them to the
> kernel. This ensures that the kernel correctly resolves type IDs from the
> appropriate 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. The FD of the module BTF is passed to the
> kernel as mod_btf_fd when the struct_ops object is loaded.
>
> For a bpf_struct_ops prog, attach_btf_obj_fd of bpf_prog is the FD of a
> module BTF in the kernel.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/lib/bpf/bpf.c    |  4 ++-
>  tools/lib/bpf/bpf.h    |  5 +++-
>  tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
>  3 files changed, 51 insertions(+), 26 deletions(-)
>

I have a few nits, please accommodate them, and with that please add
my ack on libbpf side of things

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index b0f1913763a3..af46488e4ea9 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -169,7 +169,8 @@ 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,
> +                                          value_type_btf_obj_fd);
>         union bpf_attr attr;
>         int fd;
>
> @@ -191,6 +192,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.value_type_btf_obj_fd = OPTS_GET(opts, value_type_btf_obj_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..1733cdc21241 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -51,8 +51,11 @@ struct bpf_map_create_opts {
>
>         __u32 numa_node;
>         __u32 map_ifindex;
> +
> +       __u32 value_type_btf_obj_fd;
> +       size_t:0;
>  };
> -#define bpf_map_create_opts__last_field map_ifindex
> +#define bpf_map_create_opts__last_field value_type_btf_obj_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..d8a60fb52f5c 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;
> +       int mod_btf_fd;
>         __u32 btf_key_type_id;
>         __u32 btf_value_type_id;
>         __u32 btf_vmlinux_value_type_id;
> @@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
>         return 0;
>  }
>
> +static int load_module_btfs(struct bpf_object *obj);
> +

you don't need this forward declaration, do you?

>  static const struct btf_member *
>  find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
>  {
> @@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
>         return NULL;
>  }
>

[...]

>         if (obj->btf && btf__fd(obj->btf) >= 0) {
>                 create_attr.btf_fd = btf__fd(obj->btf);
> @@ -7700,9 +7718,9 @@ 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)
> +static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
> +                             __u16 kind, struct btf **res_btf,
> +                             struct module_btf **res_mod_btf)

I actually find "find_module" terminology confusing, because it might
not be in the module after all, right? I think "find_ksym_btf_id" is a
totally appropriate name, and it's just that in some cases that kernel
symbol (ksym) will be found in the kernel module instead of in vmlinux
image itself. Still, it's a kernel. Let's keep the name?

>  {
>         struct module_btf *mod_btf;
>         struct btf *btf;

[...]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
  2023-10-17 21:49   ` Andrii Nakryiko
@ 2023-10-18  2:25     ` Kui-Feng Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-18  2:25 UTC (permalink / raw)
  To: Andrii Nakryiko, thinker.li
  Cc: bpf, ast, martin.lau, song, kernel-team, andrii, drosen, kuifeng



On 10/17/23 14:49, Andrii Nakryiko wrote:
> On Tue, Oct 17, 2023 at 9:23 AM <thinker.li@gmail.com> wrote:
>>
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Locate the module BTFs for struct_ops maps and progs and pass them to the
>> kernel. This ensures that the kernel correctly resolves type IDs from the
>> appropriate 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. The FD of the module BTF is passed to the
>> kernel as mod_btf_fd when the struct_ops object is loaded.
>>
>> For a bpf_struct_ops prog, attach_btf_obj_fd of bpf_prog is the FD of a
>> module BTF in the kernel.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/lib/bpf/bpf.c    |  4 ++-
>>   tools/lib/bpf/bpf.h    |  5 +++-
>>   tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
>>   3 files changed, 51 insertions(+), 26 deletions(-)
>>
> 
> I have a few nits, please accommodate them, and with that please add
> my ack on libbpf side of things
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>


Thanks for reviewing!

> 
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index b0f1913763a3..af46488e4ea9 100644
>> --- a/tools/lib/bpf/bpf.c
>> +++ b/tools/lib/bpf/bpf.c
>> @@ -169,7 +169,8 @@ 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,
>> +                                          value_type_btf_obj_fd);
>>          union bpf_attr attr;
>>          int fd;
>>
>> @@ -191,6 +192,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.value_type_btf_obj_fd = OPTS_GET(opts, value_type_btf_obj_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..1733cdc21241 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
>> @@ -51,8 +51,11 @@ struct bpf_map_create_opts {
>>
>>          __u32 numa_node;
>>          __u32 map_ifindex;
>> +
>> +       __u32 value_type_btf_obj_fd;
>> +       size_t:0;
>>   };
>> -#define bpf_map_create_opts__last_field map_ifindex
>> +#define bpf_map_create_opts__last_field value_type_btf_obj_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..d8a60fb52f5c 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;
>> +       int mod_btf_fd;
>>          __u32 btf_key_type_id;
>>          __u32 btf_value_type_id;
>>          __u32 btf_vmlinux_value_type_id;
>> @@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
>>          return 0;
>>   }
>>
>> +static int load_module_btfs(struct bpf_object *obj);
>> +
> 
> you don't need this forward declaration, do you?


I will remove it.

> 
>>   static const struct btf_member *
>>   find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
>>   {
>> @@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
>>          return NULL;
>>   }
>>
> 
> [...]
> 
>>          if (obj->btf && btf__fd(obj->btf) >= 0) {
>>                  create_attr.btf_fd = btf__fd(obj->btf);
>> @@ -7700,9 +7718,9 @@ 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)
>> +static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
>> +                             __u16 kind, struct btf **res_btf,
>> +                             struct module_btf **res_mod_btf)
> 
> I actually find "find_module" terminology confusing, because it might
> not be in the module after all, right? I think "find_ksym_btf_id" is a
> totally appropriate name, and it's just that in some cases that kernel
> symbol (ksym) will be found in the kernel module instead of in vmlinux
> image itself. Still, it's a kernel. Let's keep the name?

Agree!

> 
>>   {
>>          struct module_btf *mod_btf;
>>          struct btf *btf;
> 
> [...]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
  2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
@ 2023-10-19  0:00   ` Martin KaFai Lau
  2023-10-19  0:33     ` Kui-Feng Lee
  2023-10-19  2:28   ` Martin KaFai Lau
  1 sibling, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19  0:00 UTC (permalink / raw)
  To: thinker.li
  Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen

On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
>   static const struct bpf_struct_ops *
> -bpf_struct_ops_find_value(u32 value_id)
> +bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>   {
>   	unsigned int i;
>   

[ ... ]

> @@ -671,7 +672,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);

This patch does not compile because of the argument ordering.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
  2023-10-19  0:00   ` Martin KaFai Lau
@ 2023-10-19  0:33     ` Kui-Feng Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19  0:33 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen



On 10/18/23 17:00, Martin KaFai Lau wrote:
> On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
>>   static const struct bpf_struct_ops *
>> -bpf_struct_ops_find_value(u32 value_id)
>> +bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>>   {
>>       unsigned int i;
> 
> [ ... ]
> 
>> @@ -671,7 +672,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);
> 
> This patch does not compile because of the argument ordering.
> 
> 
Looks like I split the patch wrongly! Will fix it!

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
  2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
@ 2023-10-19  0:36   ` Martin KaFai Lau
  2023-10-19 16:29     ` Kui-Feng Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19  0:36 UTC (permalink / raw)
  To: thinker.li
  Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen

On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> To ensure that a module remains accessible whenever a struct_ops object of
> a struct_ops type provided by the module is still in use.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/bpf.h         |  1 +
>   kernel/bpf/bpf_struct_ops.c | 21 ++++++++++++++++++---
>   2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index e6a648af2daa..1e1647c8b0ce 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);
>   	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 7758f66ad734..b561245fe235 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -112,6 +112,7 @@ static const struct btf_type *module_type;
>   
>   static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>   				    struct btf *btf,
> +				    struct module *owner,
>   				    struct bpf_verifier_log *log)
>   {
>   	const struct btf_member *member;
> @@ -186,6 +187,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>   				st_ops->name);
>   		} else {
>   			st_ops->btf = btf;
> +			st_ops->owner = owner;

I suspect it will turn out to be just "st_ops->owner = st_ops->owner;" in a 
latter patch. st_ops->owner should have already been initialized (with 
THIS_MODULE?).

>   			st_ops->type_id = type_id;
>   			st_ops->type = t;
>   			st_ops->value_id = value_id;
> @@ -193,6 +195,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>   							    value_id);
>   		}
>   	}
> +

nit. extra newline.

>   }
>   
>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>   
>   	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);
> +		bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>   	}
>   }
>   
> @@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
>   		bpf_jit_uncharge_modmem(PAGE_SIZE);
>   	}
>   	bpf_map_area_free(st_map->uvalue);
> +	module_put(st_map->st_ops->owner);
>   	bpf_map_area_free(st_map);
>   }
>   
> @@ -676,9 +680,18 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	if (!st_ops)
>   		return ERR_PTR(-ENOTSUPP);
>   
> +	/* If st_ops->owner is NULL, it means the struct_ops is
> +	 * statically defined in the kernel.  We don't need to
> +	 * take a refcount on it.
> +	 */
> +	if (st_ops->owner && !btf_try_get_module(st_ops->btf))

This just came to my mind. Is the module refcnt needed during map alloc/free or 
it could be done during the reg/unreg instead?


> +		return ERR_PTR(-EINVAL);
> +
>   	vt = st_ops->value_type;
> -	if (attr->value_size != vt->size)
> +	if (attr->value_size != vt->size) {
> +		module_put(st_ops->owner);
>   		return ERR_PTR(-EINVAL);
> +	}
>   
>   	t = st_ops->type;
>   
> @@ -689,8 +702,10 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   		(vt->size - sizeof(struct bpf_struct_ops_value));
>   
>   	st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
> -	if (!st_map)
> +	if (!st_map) {
> +		module_put(st_ops->owner);
>   		return ERR_PTR(-ENOMEM);
> +	}
>   
>   	st_map->st_ops = st_ops;
>   	map = &st_map->map;


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
  2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
@ 2023-10-19  1:49   ` Martin KaFai Lau
  2023-10-20 15:12     ` Kui-Feng Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19  1:49 UTC (permalink / raw)
  To: thinker.li
  Cc: sinquersw, kuifeng, netdev, bpf, ast, song, kernel-team, andrii, drosen

On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
> 
> Replace the static list of struct_ops types with pre-btf struct_ops_tab to
> enable dynamic registration.
> 
> Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function
> instead of being listed in bpf_struct_ops_types.h.
> 
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   include/linux/bpf.h               |   2 +
>   include/linux/btf.h               |  29 +++++++
>   kernel/bpf/bpf_struct_ops.c       | 124 +++++++++++++++---------------
>   kernel/bpf/bpf_struct_ops_types.h |  12 ---
>   kernel/bpf/btf.c                  |   2 +-
>   net/bpf/bpf_dummy_struct_ops.c    |  14 +++-
>   net/ipv4/bpf_tcp_ca.c             |  16 +++-
>   7 files changed, 119 insertions(+), 80 deletions(-)
>   delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 1e1647c8b0ce..b0f33147aa93 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
>   	return prog->aux->func_idx != 0;
>   }
>   
> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
> +
>   #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index aa2ba77648be..fdc83aa10462 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,
> @@ -577,4 +580,30 @@ 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,
> +};
> +
> +struct bpf_struct_ops_common_value {
> +	refcount_t refcnt;
> +	enum bpf_struct_ops_state state;
> +};
> +#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common

Since there is 'struct bpf_struct_ops_common_value' now, the 
BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets remove it.

> +
> +/* 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;		\
> +}

I think the bpp_struct_ops_* should not be in btf.h. Probably move them to bpf.h 
instead. or there is some other considerations I am missing?

> +
>   #endif
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 60445ff32275..175068b083cb 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -13,19 +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,
> -};
> -
> -struct bpf_struct_ops_common_value {
> -	refcount_t refcnt;
> -	enum bpf_struct_ops_state state;
> -};
> -#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
> -
>   struct bpf_struct_ops_value {
>   	BPF_STRUCT_OPS_COMMON_VALUE;
>   	char data[] ____cacheline_aligned_in_smp;
> @@ -72,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 = {
>   };
>   
> @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>   
>   }
>   
> +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
> +				       struct btf *btf)

Please combine this function into register_bpf_struct_ops(). They are both very 
short.

> +{
> +	struct bpf_verifier_log *log;
> +	int err;
> +
> +	if (st_ops == NULL)
> +		return -EINVAL;
> +
> +	log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
> +	if (!log) {
> +		err = -ENOMEM;
> +		goto errout;
> +	}
> +
> +	log->level = BPF_LOG_KERNEL;
> +
> +	bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
> +
> +	err = btf_add_struct_ops(st_ops);
> +
> +errout:
> +	kfree(log);
> +
> +	return err;
> +}
> +
> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)

Similar to the register kfunc counterpart, can this be moved to btf.c instead by 
extern-ing bpf_struct_ops_init_one()? or there are some other structs/functions 
need to extern?

> +{
> +	struct btf *btf;
> +	int err;
> +
> +	btf = btf_get_module_btf(st_ops->owner);
> +	if (!btf)
> +		return -EINVAL;
> +	err = register_bpf_struct_ops_btf(st_ops, btf);
> +	btf_put(btf);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
> +
>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)

The bpf_struct_ops_init() is pretty much only finding the btf "module_id" and 
"common_value_id". Lets use the BTF_ID_LIST to do it instead. Then the newly 
added bpf_struct_ops_init_one() could use a proper name bpf_struct_ops_init() 
instead of having the special "_one" suffix.

>   {
> -	struct bpf_struct_ops *st_ops;
>   	s32 module_id, common_value_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) {
> @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>   		return;
>   	}
>   	common_value_type = btf_type_by_id(btf, common_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, NULL, log);
> -	}
>   }
>   
>   extern struct btf *btf_vmlinux;
> @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;
>   static const struct bpf_struct_ops *
>   bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>   {
> +	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)

The "!btf_vmlinux" should have been changed to "!btf" in the earlier patch 
(patch 2?),

and is this null check still needed now?

>   		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];

nit. Like the change in the earlier patch that is being replaced here,
directly "return st_ops_list[i];".

> +			break;
> +		}
>   	}
>   
> -	return NULL;
> +	return st_ops;
>   }
>   
>   const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id)
>   {
> +	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];

Same.

> +			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

Seeing this gone is satisfying.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index be5144dbb53d..990973d6057d 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;
> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index 5918d1b32e19..724bb7224079 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;

Is it still needed ?




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
  2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
  2023-10-19  0:00   ` Martin KaFai Lau
@ 2023-10-19  2:28   ` Martin KaFai Lau
  2023-10-19 16:15     ` Kui-Feng Lee
  1 sibling, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19  2:28 UTC (permalink / raw)
  To: thinker.li
  Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen

On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
> +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;

Is it possible that the module is already gone here? If that is the case, the 
st_ops pointer probably cannot be used?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
  2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
  2023-10-17 21:49   ` Andrii Nakryiko
@ 2023-10-19  2:43   ` Martin KaFai Lau
  2023-10-19 16:31     ` Kui-Feng Lee
  1 sibling, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19  2:43 UTC (permalink / raw)
  To: thinker.li
  Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen

On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
> -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)
> +static int find_module_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;
> @@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
>   
>   	btf = obj->btf_vmlinux;
>   	mod_btf = NULL;
> -	id = btf__find_by_name_kind(btf, ksym_name, kind);
> +	id = btf__find_by_name_kind(btf, kern_name, kind);
>   
>   	if (id == -ENOENT) {
>   		err = load_module_btfs(obj);
> @@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
>   			/* 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);
> +			id = btf__find_by_name_kind_own(btf, kern_name, kind);
>   			if (id != -ENOENT)
>   				break;
>   		}
> @@ -7744,7 +7762,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_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
>   	if (id < 0) {
>   		if (id == -ESRCH && ext->is_weak)
>   			return 0;
> @@ -7798,8 +7816,8 @@ 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,
> -				    &mod_btf);
> +	kfunc_id = find_module_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)
>   			return 0;
> @@ -9464,9 +9482,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 +9549,9 @@ 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 +12965,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);

Please avoid mixing this level of name changes with the main changes. It is 
quite confusing for the reviewer and it is not mentioned in the commit message 
either.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
  2023-10-19  2:28   ` Martin KaFai Lau
@ 2023-10-19 16:15     ` Kui-Feng Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19 16:15 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen



On 10/18/23 19:28, Martin KaFai Lau wrote:
> On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
>> +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;
> 
> Is it possible that the module is already gone here? If that is the 
> case, the st_ops pointer probably cannot be used?
The callers should call bpf_try_get_module() before calling this 
function.  I will check the code to ensure all callers do that in
the next version.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
  2023-10-19  0:36   ` Martin KaFai Lau
@ 2023-10-19 16:29     ` Kui-Feng Lee
  2023-10-20  5:07       ` Kui-Feng Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19 16:29 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen



On 10/18/23 17:36, Martin KaFai Lau wrote:
> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> To ensure that a module remains accessible whenever a struct_ops 
>> object of
>> a struct_ops type provided by the module is still in use.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/linux/bpf.h         |  1 +
>>   kernel/bpf/bpf_struct_ops.c | 21 ++++++++++++++++++---
>>   2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index e6a648af2daa..1e1647c8b0ce 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);
>>       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 7758f66ad734..b561245fe235 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -112,6 +112,7 @@ static const struct btf_type *module_type;
>>   static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>>                       struct btf *btf,
>> +                    struct module *owner,
>>                       struct bpf_verifier_log *log)
>>   {
>>       const struct btf_member *member;
>> @@ -186,6 +187,7 @@ static void bpf_struct_ops_init_one(struct 
>> bpf_struct_ops *st_ops,
>>                   st_ops->name);
>>           } else {
>>               st_ops->btf = btf;
>> +            st_ops->owner = owner;
> 
> I suspect it will turn out to be just "st_ops->owner = st_ops->owner;" 
> in a latter patch. st_ops->owner should have already been initialized 
> (with THIS_MODULE?).


Yes, you are correct.  It ends up st_ops->owner passing from the caller.
I will remove this line and the argument.

> 
>>               st_ops->type_id = type_id;
>>               st_ops->type = t;
>>               st_ops->value_id = value_id;
>> @@ -193,6 +195,7 @@ static void bpf_struct_ops_init_one(struct 
>> bpf_struct_ops *st_ops,
>>                                   value_id);
>>           }
>>       }
>> +
> 
> nit. extra newline.

got it!

> 
>>   }
>>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
>> bpf_verifier_log *log)
>>       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);
>> +        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>>       }
>>   }
>> @@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct 
>> bpf_map *map)
>>           bpf_jit_uncharge_modmem(PAGE_SIZE);
>>       }
>>       bpf_map_area_free(st_map->uvalue);
>> +    module_put(st_map->st_ops->owner);
>>       bpf_map_area_free(st_map);
>>   }
>> @@ -676,9 +680,18 @@ static struct bpf_map 
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>       if (!st_ops)
>>           return ERR_PTR(-ENOTSUPP);
>> +    /* If st_ops->owner is NULL, it means the struct_ops is
>> +     * statically defined in the kernel.  We don't need to
>> +     * take a refcount on it.
>> +     */
>> +    if (st_ops->owner && !btf_try_get_module(st_ops->btf))
> 
> This just came to my mind. Is the module refcnt needed during map 
> alloc/free or it could be done during the reg/unreg instead?


Sure, I can move it to reg/unreg.

> 
> 
>> +        return ERR_PTR(-EINVAL);
>> +
>>       vt = st_ops->value_type;
>> -    if (attr->value_size != vt->size)
>> +    if (attr->value_size != vt->size) {
>> +        module_put(st_ops->owner);
>>           return ERR_PTR(-EINVAL);
>> +    }
>>       t = st_ops->type;
>> @@ -689,8 +702,10 @@ static struct bpf_map 
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>           (vt->size - sizeof(struct bpf_struct_ops_value));
>>       st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
>> -    if (!st_map)
>> +    if (!st_map) {
>> +        module_put(st_ops->owner);
>>           return ERR_PTR(-ENOMEM);
>> +    }
>>       st_map->st_ops = st_ops;
>>       map = &st_map->map;
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
  2023-10-19  2:43   ` Martin KaFai Lau
@ 2023-10-19 16:31     ` Kui-Feng Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19 16:31 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen



On 10/18/23 19:43, Martin KaFai Lau wrote:
> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>> -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)
>> +static int find_module_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;
>> @@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object 
>> *obj, const char *ksym_name,
>>       btf = obj->btf_vmlinux;
>>       mod_btf = NULL;
>> -    id = btf__find_by_name_kind(btf, ksym_name, kind);
>> +    id = btf__find_by_name_kind(btf, kern_name, kind);
>>       if (id == -ENOENT) {
>>           err = load_module_btfs(obj);
>> @@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object 
>> *obj, const char *ksym_name,
>>               /* 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);
>> +            id = btf__find_by_name_kind_own(btf, kern_name, kind);
>>               if (id != -ENOENT)
>>                   break;
>>           }
>> @@ -7744,7 +7762,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_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, 
>> &mod_btf);
>>       if (id < 0) {
>>           if (id == -ESRCH && ext->is_weak)
>>               return 0;
>> @@ -7798,8 +7816,8 @@ 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,
>> -                    &mod_btf);
>> +    kfunc_id = find_module_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)
>>               return 0;
>> @@ -9464,9 +9482,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 +9549,9 @@ 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 +12965,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);
> 
> Please avoid mixing this level of name changes with the main changes. It 
> is quite confusing for the reviewer and it is not mentioned in the 
> commit message either.

Got it! Sorry confusing.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
  2023-10-19 16:29     ` Kui-Feng Lee
@ 2023-10-20  5:07       ` Kui-Feng Lee
  2023-10-20 21:37         ` Martin KaFai Lau
  0 siblings, 1 reply; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20  5:07 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen



On 10/19/23 09:29, Kui-Feng Lee wrote:
> 
> 
> On 10/18/23 17:36, Martin KaFai Lau wrote:
>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
> 
>>
>>>   }
>>>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log 
>>> *log)
>>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
>>> bpf_verifier_log *log)
>>>       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);
>>> +        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>>>       }
>>>   }
>>> @@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct 
>>> bpf_map *map)
>>>           bpf_jit_uncharge_modmem(PAGE_SIZE);
>>>       }
>>>       bpf_map_area_free(st_map->uvalue);
>>> +    module_put(st_map->st_ops->owner);
>>>       bpf_map_area_free(st_map);
>>>   }
>>> @@ -676,9 +680,18 @@ static struct bpf_map 
>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>       if (!st_ops)
>>>           return ERR_PTR(-ENOTSUPP);
>>> +    /* If st_ops->owner is NULL, it means the struct_ops is
>>> +     * statically defined in the kernel.  We don't need to
>>> +     * take a refcount on it.
>>> +     */
>>> +    if (st_ops->owner && !btf_try_get_module(st_ops->btf))
>>
>> This just came to my mind. Is the module refcnt needed during map 
>> alloc/free or it could be done during the reg/unreg instead?
> 
> 
> Sure, I can move it to reg/unreg.

Just found that we relies type information in st_ops to update element 
and clean up maps.
We can not move get/put modules to reg/unreg except keeping a redundant 
copy in
st_map or somewhere. It make the code much more complicated by
introducing get/put module here and there.

I prefer to keep as it is now. WDYT?

>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
  2023-10-19  1:49   ` Martin KaFai Lau
@ 2023-10-20 15:12     ` Kui-Feng Lee
  2023-10-20 17:53       ` Kui-Feng Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20 15:12 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, netdev, bpf, ast, song, kernel-team, andrii, drosen



On 10/18/23 18:49, Martin KaFai Lau wrote:
> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Replace the static list of struct_ops types with pre-btf 
>> struct_ops_tab to
>> enable dynamic registration.
>>
>> Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function
>> instead of being listed in bpf_struct_ops_types.h.
>>
>> Cc: netdev@vger.kernel.org
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   include/linux/bpf.h               |   2 +
>>   include/linux/btf.h               |  29 +++++++
>>   kernel/bpf/bpf_struct_ops.c       | 124 +++++++++++++++---------------
>>   kernel/bpf/bpf_struct_ops_types.h |  12 ---
>>   kernel/bpf/btf.c                  |   2 +-
>>   net/bpf/bpf_dummy_struct_ops.c    |  14 +++-
>>   net/ipv4/bpf_tcp_ca.c             |  16 +++-
>>   7 files changed, 119 insertions(+), 80 deletions(-)
>>   delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 1e1647c8b0ce..b0f33147aa93 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3207,4 +3207,6 @@ static inline bool bpf_is_subprog(const struct 
>> bpf_prog *prog)
>>       return prog->aux->func_idx != 0;
>>   }
>> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
>> +
>>   #endif /* _LINUX_BPF_H */
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index aa2ba77648be..fdc83aa10462 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,
>> @@ -577,4 +580,30 @@ 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,
>> +};
>> +
>> +struct bpf_struct_ops_common_value {
>> +    refcount_t refcnt;
>> +    enum bpf_struct_ops_state state;
>> +};
>> +#define BPF_STRUCT_OPS_COMMON_VALUE struct 
>> bpf_struct_ops_common_value common
> 
> Since there is 'struct bpf_struct_ops_common_value' now, the 
> BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets 
> remove it.

Agree

> 
>> +
>> +/* 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;        \
>> +}
> 
> I think the bpp_struct_ops_* should not be in btf.h. Probably move them 
> to bpf.h instead. or there is some other considerations I am missing?

Yes, I think bpf.h is the right place.

> 
>> +
>>   #endif
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 60445ff32275..175068b083cb 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -13,19 +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,
>> -};
>> -
>> -struct bpf_struct_ops_common_value {
>> -    refcount_t refcnt;
>> -    enum bpf_struct_ops_state state;
>> -};
>> -#define BPF_STRUCT_OPS_COMMON_VALUE struct 
>> bpf_struct_ops_common_value common
>> -
>>   struct bpf_struct_ops_value {
>>       BPF_STRUCT_OPS_COMMON_VALUE;
>>       char data[] ____cacheline_aligned_in_smp;
>> @@ -72,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 = {
>>   };
>> @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct 
>> bpf_struct_ops *st_ops,
>>   }
>> +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
>> +                       struct btf *btf)
> 
> Please combine this function into register_bpf_struct_ops(). They are 
> both very short.
> 

Got it!

>> +{
>> +    struct bpf_verifier_log *log;
>> +    int err;
>> +
>> +    if (st_ops == NULL)
>> +        return -EINVAL;
>> +
>> +    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
>> +    if (!log) {
>> +        err = -ENOMEM;
>> +        goto errout;
>> +    }
>> +
>> +    log->level = BPF_LOG_KERNEL;
>> +
>> +    bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
>> +
>> +    err = btf_add_struct_ops(st_ops);
>> +
>> +errout:
>> +    kfree(log);
>> +
>> +    return err;
>> +}
>> +
>> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
> 
> Similar to the register kfunc counterpart, can this be moved to btf.c 
> instead by extern-ing bpf_struct_ops_init_one()? or there are some other 
> structs/functions need to extern?

It is wierd to move a function of bpf_struct_ops to btf.
But, kfunc already did that, I don't mind to follow it.

> 
>> +{
>> +    struct btf *btf;
>> +    int err;
>> +
>> +    btf = btf_get_module_btf(st_ops->owner);
>> +    if (!btf)
>> +        return -EINVAL;
>> +    err = register_bpf_struct_ops_btf(st_ops, btf);
>> +    btf_put(btf);
>> +
>> +    return err;
>> +}
>> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
>> +
>>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> 
> The bpf_struct_ops_init() is pretty much only finding the btf 
> "module_id" and "common_value_id". Lets use the BTF_ID_LIST to do it 
> instead. Then the newly added bpf_struct_ops_init_one() could use a 
> proper name bpf_struct_ops_init() instead of having the special "_one" 
> suffix.

Got it!

> 
>>   {
>> -    struct bpf_struct_ops *st_ops;
>>       s32 module_id, common_value_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) {
>> @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct 
>> bpf_verifier_log *log)
>>           return;
>>       }
>>       common_value_type = btf_type_by_id(btf, common_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, NULL, log);
>> -    }
>>   }
>>   extern struct btf *btf_vmlinux;
>> @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;
>>   static const struct bpf_struct_ops *
>>   bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>>   {
>> +    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)
> 
> The "!btf_vmlinux" should have been changed to "!btf" in the earlier 
> patch (patch 2?),

This is not btf. It mean to check if btf_vmlinux is initialized.
It is not necessary anymore.
For checking btf, the following btf_get_struct_ops() will keep cnt zero
if btf is NULL, so it is unnecessary as well.

> 
> and is this null check still needed now?
> 
>>           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];
> 
> nit. Like the change in the earlier patch that is being replaced here,
> directly "return st_ops_list[i];".

Got it!

> 
>> +            break;
>> +        }
>>       }
>> -    return NULL;
>> +    return st_ops;
>>   }
>>   const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, 
>> u32 type_id)
>>   {
>> +    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];
> 
> Same.

Ack!

> 
>> +            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
> 
> Seeing this gone is satisfying
> 
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index be5144dbb53d..990973d6057d 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;
>> diff --git a/net/bpf/bpf_dummy_struct_ops.c 
>> b/net/bpf/bpf_dummy_struct_ops.c
>> index 5918d1b32e19..724bb7224079 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;
> 
> Is it still needed ?

Yes, it will be used by bpf_struct_ops_test_run().


> 
> 
> 


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
  2023-10-20 15:12     ` Kui-Feng Lee
@ 2023-10-20 17:53       ` Kui-Feng Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20 17:53 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, netdev, bpf, ast, song, kernel-team, andrii, drosen



On 10/20/23 08:12, Kui-Feng Lee wrote:
> 
> 
> On 10/18/23 18:49, Martin KaFai Lau wrote:
>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>>   static const struct bpf_struct_ops *
>>>   bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>>>   {
>>> +    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)
>>
>> The "!btf_vmlinux" should have been changed to "!btf" in the earlier 
>> patch (patch 2?),
> 
> This is not btf. It mean to check if btf_vmlinux is initialized.
> It is not necessary anymore.
> For checking btf, the following btf_get_struct_ops() will keep cnt zero
> if btf is NULL, so it is unnecessary as well.

Forget my previous comment.  I think you are right!


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
  2023-10-20  5:07       ` Kui-Feng Lee
@ 2023-10-20 21:37         ` Martin KaFai Lau
  2023-10-20 22:28           ` Kui-Feng Lee
  0 siblings, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-20 21:37 UTC (permalink / raw)
  To: Kui-Feng Lee, thinker.li
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen

On 10/19/23 10:07 PM, Kui-Feng Lee wrote:
> 
> 
> On 10/19/23 09:29, Kui-Feng Lee wrote:
>>
>>
>> On 10/18/23 17:36, Martin KaFai Lau wrote:
>>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>>
>>>
>>>>   }
>>>>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>>>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct 
>>>> bpf_verifier_log *log)
>>>>       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);
>>>> +        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>>>>       }
>>>>   }
>>>> @@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
>>>>           bpf_jit_uncharge_modmem(PAGE_SIZE);
>>>>       }
>>>>       bpf_map_area_free(st_map->uvalue);
>>>> +    module_put(st_map->st_ops->owner);
>>>>       bpf_map_area_free(st_map);
>>>>   }
>>>> @@ -676,9 +680,18 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union 
>>>> bpf_attr *attr)
>>>>       if (!st_ops)
>>>>           return ERR_PTR(-ENOTSUPP);
>>>> +    /* If st_ops->owner is NULL, it means the struct_ops is
>>>> +     * statically defined in the kernel.  We don't need to
>>>> +     * take a refcount on it.
>>>> +     */
>>>> +    if (st_ops->owner && !btf_try_get_module(st_ops->btf))

While replying and looking at it again, I don't think the 
btf_try_get_module(st_ops->btf) is safe. The module's owned st_ops itself could 
have been gone with the module. The same goes with the "st_ops->owner" test, so 
btf_is_module(btf) should be used instead.

I am risking to act like a broken clock to repeat this question, does it really 
need to store btf back into the st_ops which may accidentally get into the above 
btf_try_get_module(st_ops->btf) usage?

>>>
>>> This just came to my mind. Is the module refcnt needed during map alloc/free 
>>> or it could be done during the reg/unreg instead?
>>
>>
>> Sure, I can move it to reg/unreg.
> 
> Just found that we relies type information in st_ops to update element and clean 
> up maps.
> We can not move get/put modules to reg/unreg except keeping a redundant copy in
> st_map or somewhere. It make the code much more complicated by
> introducing get/put module here and there.
> 
> I prefer to keep as it is now. WDYT?

Yeah, sure. I was asking after seeing a longer wait time for the module to go 
away in patch 11 selftest and requires an explicit waiting for the tasks_trace 
period. Releasing the module refcnt earlier will help.

Regardless of the module refcnt hold/free location, I think storing the type* 
and value* in the module's owned st_ops does not look correct now. It was fine 
and convenient to piggy back them into bpf_struct_ops when everything was 
built-in the kernel and no lifetime concern. It makes sense now to separate them 
out from the module's owned st_ops. Something like:

struct btf_struct_ops_desc {
	struct bpf_struct_ops *ops;
         const struct btf_type *type;
         const struct btf_type *value_type;
         u32 type_id;
         u32 value_id;
};

struct btf_struct_ops_tab {
         u32 cnt;
	u32 capacity;
	struct btf_struct_ops_desc *st_ops_desc[];
};

wdyt?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
  2023-10-20 21:37         ` Martin KaFai Lau
@ 2023-10-20 22:28           ` Kui-Feng Lee
  0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20 22:28 UTC (permalink / raw)
  To: Martin KaFai Lau, thinker.li
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen



On 10/20/23 14:37, Martin KaFai Lau wrote:
> On 10/19/23 10:07 PM, Kui-Feng Lee wrote:
>>
>>
>> On 10/19/23 09:29, Kui-Feng Lee wrote:
>>>
>>>
>>> On 10/18/23 17:36, Martin KaFai Lau wrote:
>>>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>>>
>>>>
>>>>>   }
>>>>>   void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log 
>>>>> *log)
>>>>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, 
>>>>> struct bpf_verifier_log *log)
>>>>>       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);
>>>>> +        bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>>>>>       }
>>>>>   }
>>>>> @@ -630,6 +633,7 @@ static void __bpf_struct_ops_map_free(struct 
>>>>> bpf_map *map)
>>>>>           bpf_jit_uncharge_modmem(PAGE_SIZE);
>>>>>       }
>>>>>       bpf_map_area_free(st_map->uvalue);
>>>>> +    module_put(st_map->st_ops->owner);
>>>>>       bpf_map_area_free(st_map);
>>>>>   }
>>>>> @@ -676,9 +680,18 @@ static struct bpf_map 
>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>>       if (!st_ops)
>>>>>           return ERR_PTR(-ENOTSUPP);
>>>>> +    /* If st_ops->owner is NULL, it means the struct_ops is
>>>>> +     * statically defined in the kernel.  We don't need to
>>>>> +     * take a refcount on it.
>>>>> +     */
>>>>> +    if (st_ops->owner && !btf_try_get_module(st_ops->btf))
> 
> While replying and looking at it again, I don't think the 
> btf_try_get_module(st_ops->btf) is safe. The module's owned st_ops 
> itself could have been gone with the module. The same goes with the 
> "st_ops->owner" test, so btf_is_module(btf) should be used instead.

I have change it locally. Here, it calls btf_try_get_module() after
calling btf_struct_ops_find_value(). The new code will call
btf_try_get_module() against the btf from attr->value_type_btf_obj_fd
before btf_struct_ops_find_value(). Just like I mentioned earlier to
ensure the callers of btf_struct_ops_find_value() and
btf_struct_ops_find() hold a refcount to the module.

> 
> I am risking to act like a broken clock to repeat this question, does it 
> really need to store btf back into the st_ops which may accidentally get 
> into the above btf_try_get_module(st_ops->btf) usage?



> 
>>>>
>>>> This just came to my mind. Is the module refcnt needed during map 
>>>> alloc/free or it could be done during the reg/unreg instead?
>>>
>>>
>>> Sure, I can move it to reg/unreg.
>>
>> Just found that we relies type information in st_ops to update element 
>> and clean up maps.
>> We can not move get/put modules to reg/unreg except keeping a 
>> redundant copy in
>> st_map or somewhere. It make the code much more complicated by
>> introducing get/put module here and there.
>>
>> I prefer to keep as it is now. WDYT?
> 
> Yeah, sure. I was asking after seeing a longer wait time for the module 
> to go away in patch 11 selftest and requires an explicit waiting for the 
> tasks_trace period. Releasing the module refcnt earlier will help.
> 
> Regardless of the module refcnt hold/free location, I think storing the 
> type* and value* in the module's owned st_ops does not look correct now. 
> It was fine and convenient to piggy back them into bpf_struct_ops when 
> everything was built-in the kernel and no lifetime concern. It makes 
> sense now to separate them out from the module's owned st_ops. Something 
> like:
> 
> struct btf_struct_ops_desc {
>      struct bpf_struct_ops *ops;
>          const struct btf_type *type;
>          const struct btf_type *value_type;
>          u32 type_id;
>          u32 value_id;
> };
> 
> struct btf_struct_ops_tab {
>          u32 cnt;
>      u32 capacity;
>      struct btf_struct_ops_desc *st_ops_desc[];
> };
> 
> wdyt?

So, st_map should hold a pointer to a bpf_struct_ops_desc instead of
st_ops, right? It would work!

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2023-10-20 22:28 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
2023-10-19  0:00   ` Martin KaFai Lau
2023-10-19  0:33     ` Kui-Feng Lee
2023-10-19  2:28   ` Martin KaFai Lau
2023-10-19 16:15     ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
2023-10-19  0:36   ` Martin KaFai Lau
2023-10-19 16:29     ` Kui-Feng Lee
2023-10-20  5:07       ` Kui-Feng Lee
2023-10-20 21:37         ` Martin KaFai Lau
2023-10-20 22:28           ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 4/9] bpf: validate value_type thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
2023-10-19  1:49   ` Martin KaFai Lau
2023-10-20 15:12     ` Kui-Feng Lee
2023-10-20 17:53       ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
2023-10-17 21:49   ` Andrii Nakryiko
2023-10-18  2:25     ` Kui-Feng Lee
2023-10-19  2:43   ` Martin KaFai Lau
2023-10-19 16:31     ` Kui-Feng Lee
2023-10-17 16:23 ` [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
2023-10-17 18:03   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).