* [PATCH bpf-next v6 02/10] bpf, net: introduce bpf_struct_ops_desc. [not found] <20231022050335.2579051-1-thinker.li@gmail.com> @ 2023-10-22 5:03 ` thinker.li 2023-10-22 5:03 ` [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration thinker.li 1 sibling, 0 replies; 6+ messages in thread From: thinker.li @ 2023-10-22 5:03 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> Move some of members of bpf_struct_ops to bpf_struct_ops_desc. When we introduce the APIs to register new bpf_struct_ops types from modules, bpf_struct_ops may destroyed when the module is unloaded. Moving these members to bpf_struct_ops_desc make these data available even the module is unloaded. Cc: netdev@vger.kernel.org Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> --- include/linux/bpf.h | 13 ++++-- kernel/bpf/bpf_struct_ops.c | 76 +++++++++++++++++----------------- kernel/bpf/verifier.c | 8 ++-- net/bpf/bpf_dummy_struct_ops.c | 9 +++- net/ipv4/bpf_tcp_ca.c | 8 +++- 5 files changed, 68 insertions(+), 46 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 30063a760b5a..705e1accfb98 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1626,17 +1626,22 @@ struct bpf_struct_ops { void (*unreg)(void *kdata); int (*update)(void *kdata, void *old_kdata); int (*validate)(void *kdata); - const struct btf_type *type; - const struct btf_type *value_type; const char *name; struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS]; +}; + +struct bpf_struct_ops_desc { + struct bpf_struct_ops *st_ops; + + const struct btf_type *type; + const struct btf_type *value_type; u32 type_id; u32 value_id; }; #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_desc *bpf_struct_ops_find(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 +1684,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_desc *bpf_struct_ops_find(u32 type_id) { return NULL; } diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 627cf1ea840a..e35d6321a2f8 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -32,7 +32,7 @@ struct bpf_struct_ops_value { struct bpf_struct_ops_map { struct bpf_map map; struct rcu_head rcu; - const struct bpf_struct_ops *st_ops; + const struct bpf_struct_ops_desc *st_ops_desc; /* protect map_update */ struct mutex lock; /* link has all the bpf_links that is populated @@ -92,9 +92,9 @@ enum { __NR_BPF_STRUCT_OPS_TYPE, }; -static struct bpf_struct_ops * const bpf_struct_ops[] = { +static struct bpf_struct_ops_desc bpf_struct_ops[] = { #define BPF_STRUCT_OPS_TYPE(_name) \ - [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name, + [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name }, #include "bpf_struct_ops_types.h" #undef BPF_STRUCT_OPS_TYPE }; @@ -110,10 +110,11 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = { static const struct btf_type *module_type; -static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops, +static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc, struct btf *btf, struct bpf_verifier_log *log) { + struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; const struct btf_member *member; const struct btf_type *t; s32 type_id, value_id; @@ -185,18 +186,18 @@ 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->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); + st_ops_desc->type_id = type_id; + st_ops_desc->type = t; + st_ops_desc->value_id = value_id; + st_ops_desc->value_type = btf_type_by_id(btf, + value_id); } } } void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) { - struct bpf_struct_ops *st_ops; + struct bpf_struct_ops_desc *st_ops_desc; s32 module_id; u32 i; @@ -213,14 +214,14 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) module_type = btf_type_by_id(btf, module_id); for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { - st_ops = bpf_struct_ops[i]; - bpf_struct_ops_init_one(st_ops, btf, log); + st_ops_desc = &bpf_struct_ops[i]; + bpf_struct_ops_init_one(st_ops_desc, btf, log); } } extern struct btf *btf_vmlinux; -static const struct bpf_struct_ops * +static const struct bpf_struct_ops_desc * bpf_struct_ops_find_value(u32 value_id) { unsigned int i; @@ -229,14 +230,14 @@ bpf_struct_ops_find_value(u32 value_id) 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]; + if (bpf_struct_ops[i].value_id == value_id) + return &bpf_struct_ops[i]; } return NULL; } -const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id) +const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id) { unsigned int i; @@ -244,8 +245,8 @@ const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id) 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]; + if (bpf_struct_ops[i].type_id == type_id) + return &bpf_struct_ops[i]; } return NULL; @@ -305,7 +306,7 @@ static void *bpf_struct_ops_map_lookup_elem(struct bpf_map *map, void *key) static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map) { - const struct btf_type *t = st_map->st_ops->type; + const struct btf_type *t = st_map->st_ops_desc->type; u32 i; for (i = 0; i < btf_type_vlen(t); i++) { @@ -379,10 +380,11 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, void *value, u64 flags) { struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map; - const struct bpf_struct_ops *st_ops = st_map->st_ops; + const struct bpf_struct_ops_desc *st_ops_desc = st_map->st_ops_desc; + const struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; struct bpf_struct_ops_value *uvalue, *kvalue; const struct btf_member *member; - const struct btf_type *t = st_ops->type; + const struct btf_type *t = st_ops_desc->type; struct bpf_tramp_links *tlinks; void *udata, *kdata; int prog_fd, err; @@ -395,7 +397,7 @@ 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_desc->value_type, value); if (err) return err; @@ -487,7 +489,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, } if (prog->type != BPF_PROG_TYPE_STRUCT_OPS || - prog->aux->attach_btf_id != st_ops->type_id || + prog->aux->attach_btf_id != st_ops_desc->type_id || prog->expected_attach_type != i) { bpf_prog_put(prog); err = -EINVAL; @@ -583,7 +585,7 @@ static long bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key) BPF_STRUCT_OPS_STATE_TOBEFREE); switch (prev_state) { case BPF_STRUCT_OPS_STATE_INUSE: - st_map->st_ops->unreg(&st_map->kvalue.data); + st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data); bpf_map_put(map); return 0; case BPF_STRUCT_OPS_STATE_TOBEFREE: @@ -664,22 +666,22 @@ static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr) static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr) { - const struct bpf_struct_ops *st_ops; + const struct bpf_struct_ops_desc *st_ops_desc; size_t st_map_size; struct bpf_struct_ops_map *st_map; const struct btf_type *t, *vt; struct bpf_map *map; int ret; - st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id); - if (!st_ops) + st_ops_desc = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id); + if (!st_ops_desc) return ERR_PTR(-ENOTSUPP); - vt = st_ops->value_type; + vt = st_ops_desc->value_type; if (attr->value_size != vt->size) return ERR_PTR(-EINVAL); - t = st_ops->type; + t = st_ops_desc->type; st_map_size = sizeof(*st_map) + /* kvalue stores the @@ -691,7 +693,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr) if (!st_map) return ERR_PTR(-ENOMEM); - st_map->st_ops = st_ops; + st_map->st_ops_desc = st_ops_desc; map = &st_map->map; ret = bpf_jit_charge_modmem(PAGE_SIZE); @@ -729,8 +731,8 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr) static u64 bpf_struct_ops_map_mem_usage(const struct bpf_map *map) { struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map; - const struct bpf_struct_ops *st_ops = st_map->st_ops; - const struct btf_type *vt = st_ops->value_type; + const struct bpf_struct_ops_desc *st_ops_desc = st_map->st_ops_desc; + const struct btf_type *vt = st_ops_desc->value_type; u64 usage; usage = sizeof(*st_map) + @@ -804,7 +806,7 @@ static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link) /* st_link->map can be NULL if * bpf_struct_ops_link_create() fails to register. */ - st_map->st_ops->unreg(&st_map->kvalue.data); + st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data); bpf_map_put(&st_map->map); } kfree(st_link); @@ -851,7 +853,7 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map if (!bpf_struct_ops_valid_to_reg(new_map)) return -EINVAL; - if (!st_map->st_ops->update) + if (!st_map->st_ops_desc->st_ops->update) return -EOPNOTSUPP; mutex_lock(&update_mutex); @@ -864,12 +866,12 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map old_st_map = container_of(old_map, struct bpf_struct_ops_map, map); /* The new and old struct_ops must be the same type. */ - if (st_map->st_ops != old_st_map->st_ops) { + if (st_map->st_ops_desc != old_st_map->st_ops_desc) { err = -EINVAL; goto err_out; } - err = st_map->st_ops->update(st_map->kvalue.data, old_st_map->kvalue.data); + err = st_map->st_ops_desc->st_ops->update(st_map->kvalue.data, old_st_map->kvalue.data); if (err) goto err_out; @@ -920,7 +922,7 @@ int bpf_struct_ops_link_create(union bpf_attr *attr) if (err) goto err_out; - err = st_map->st_ops->reg(st_map->kvalue.data); + err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data); if (err) { bpf_link_cleanup(&link_primer); link = NULL; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a7178ecf676d..5c8029606dcc 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -19619,6 +19619,7 @@ static void print_verification_stats(struct bpf_verifier_env *env) static int check_struct_ops_btf_id(struct bpf_verifier_env *env) { const struct btf_type *t, *func_proto; + const struct bpf_struct_ops_desc *st_ops_desc; const struct bpf_struct_ops *st_ops; const struct btf_member *member; struct bpf_prog *prog = env->prog; @@ -19631,14 +19632,15 @@ 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); - if (!st_ops) { + st_ops_desc = bpf_struct_ops_find(btf_id); + if (!st_ops_desc) { verbose(env, "attach_btf_id %u is not a supported struct\n", btf_id); return -ENOTSUPP; } + st_ops = st_ops_desc->st_ops; - t = st_ops->type; + t = st_ops_desc->type; member_idx = prog->expected_attach_type; if (member_idx >= btf_type_vlen(t)) { verbose(env, "attach to invalid member idx %u of struct %s\n", diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index 5918d1b32e19..ffa224053a6c 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -17,6 +17,8 @@ struct bpf_dummy_ops_test_args { struct bpf_dummy_ops_state state; }; +static struct btf *bpf_dummy_ops_btf; + static struct bpf_dummy_ops_test_args * dummy_ops_init_args(const union bpf_attr *kattr, unsigned int nr) { @@ -85,9 +87,13 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr, void *image = NULL; unsigned int op_idx; int prog_ret; + u32 type_id; int err; - if (prog->aux->attach_btf_id != st_ops->type_id) + type_id = btf_find_by_name_kind(bpf_dummy_ops_btf, + bpf_bpf_dummy_ops.name, + BTF_KIND_STRUCT); + if (prog->aux->attach_btf_id != type_id) return -EOPNOTSUPP; func_proto = prog->aux->attach_func_proto; @@ -143,6 +149,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr, static int bpf_dummy_init(struct btf *btf) { + bpf_dummy_ops_btf = btf; return 0; } diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c index 39dcccf0f174..3c8b76578a2a 100644 --- a/net/ipv4/bpf_tcp_ca.c +++ b/net/ipv4/bpf_tcp_ca.c @@ -20,6 +20,7 @@ static u32 unsupported_ops[] = { static const struct btf_type *tcp_sock_type; static u32 tcp_sock_id, sock_id; +static const struct btf_type *tcp_congestion_ops_type; static int bpf_tcp_ca_init(struct btf *btf) { @@ -36,6 +37,11 @@ static int bpf_tcp_ca_init(struct btf *btf) tcp_sock_id = type_id; tcp_sock_type = btf_type_by_id(btf, tcp_sock_id); + type_id = btf_find_by_name_kind(btf, "tcp_congestion_ops", BTF_KIND_STRUCT); + if (type_id < 0) + return -EINVAL; + tcp_congestion_ops_type = btf_type_by_id(btf, type_id); + return 0; } @@ -149,7 +155,7 @@ static u32 prog_ops_moff(const struct bpf_prog *prog) u32 midx; midx = prog->expected_attach_type; - t = bpf_tcp_congestion_ops.type; + t = tcp_congestion_ops_type; m = &btf_type_member(t)[midx]; return __btf_member_bit_offset(t, m) / 8; -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration [not found] <20231022050335.2579051-1-thinker.li@gmail.com> 2023-10-22 5:03 ` [PATCH bpf-next v6 02/10] bpf, net: introduce bpf_struct_ops_desc thinker.li @ 2023-10-22 5:03 ` thinker.li 2023-10-26 21:02 ` Eduard Zingerman 1 sibling, 1 reply; 6+ messages in thread From: thinker.li @ 2023-10-22 5:03 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 per-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 | 36 +++++++-- include/linux/btf.h | 5 +- kernel/bpf/bpf_struct_ops.c | 123 ++++++++---------------------- kernel/bpf/bpf_struct_ops_types.h | 12 --- kernel/bpf/btf.c | 41 +++++++++- net/bpf/bpf_dummy_struct_ops.c | 14 +++- net/ipv4/bpf_tcp_ca.c | 16 +++- 7 files changed, 130 insertions(+), 117 deletions(-) delete mode 100644 kernel/bpf/bpf_struct_ops_types.h diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 26feb8a2da4f..a53b2181c845 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1644,7 +1644,6 @@ struct bpf_struct_ops_desc { #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL) #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA)) const struct bpf_struct_ops_desc *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); int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key, @@ -1690,10 +1689,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf * { return NULL; } -static inline void bpf_struct_ops_init(struct btf *btf, - struct bpf_verifier_log *log) -{ -} static inline bool bpf_try_module_get(const void *data, struct module *owner) { return try_module_get(owner); @@ -3212,4 +3207,35 @@ 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); + +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; +}; + +/* 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 { \ + struct bpf_struct_ops_common_value common; \ + struct _name data ____cacheline_aligned_in_smp; \ +} + +extern void bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc, + struct btf *btf, + struct bpf_verifier_log *log); + #endif /* _LINUX_BPF_H */ diff --git a/include/linux/btf.h b/include/linux/btf.h index 6a64b372b7a0..533db3f406b3 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, @@ -574,8 +577,6 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type struct bpf_struct_ops; struct bpf_struct_ops_desc; -struct bpf_struct_ops_desc * -btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops); const struct bpf_struct_ops_desc * btf_get_struct_ops(struct btf *btf, u32 *ret_cnt); diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 99ab61cc6cad..44412f95bc32 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -13,21 +13,8 @@ #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; + struct bpf_struct_ops_common_value common; 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_desc bpf_struct_ops[] = { -#define BPF_STRUCT_OPS_TYPE(_name) \ - [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name }, -#include "bpf_struct_ops_types.h" -#undef BPF_STRUCT_OPS_TYPE -}; - const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = { }; @@ -110,13 +68,22 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = { #endif }; -static const struct btf_type *module_type; -static const struct btf_type *common_value_type; +BTF_ID_LIST(st_ops_ids) +BTF_ID(struct, module) +BTF_ID(struct, bpf_struct_ops_common_value) + +enum { + idx_module_id, + idx_st_ops_common_value_id, +}; + +extern struct btf *btf_vmlinux; static bool is_valid_value_type(struct btf *btf, s32 value_id, const struct btf_type *type, const char *value_name) { + const struct btf_type *common_value_type; const struct btf_member *member; const struct btf_type *vt, *mt; @@ -128,6 +95,8 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id, } member = btf_type_member(vt); mt = btf_type_by_id(btf, member->type); + common_value_type = btf_type_by_id(btf_vmlinux, + st_ops_ids[idx_st_ops_common_value_id]); if (mt != common_value_type) { pr_warn("The first member of %s should be bpf_struct_ops_common_value\n", value_name); @@ -144,9 +113,9 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id, return true; } -static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc, - struct btf *btf, - struct bpf_verifier_log *log) +void bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc, + struct btf *btf, + struct bpf_verifier_log *log) { struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; const struct btf_member *member; @@ -232,51 +201,20 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc, } } -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) -{ - struct bpf_struct_ops_desc *st_ops_desc; - 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) { - pr_warn("Cannot find struct module in btf_vmlinux\n"); - 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_desc = &bpf_struct_ops[i]; - bpf_struct_ops_init_one(st_ops_desc, btf, log); - } -} - -extern struct btf *btf_vmlinux; - static const struct bpf_struct_ops_desc * bpf_struct_ops_find_value(struct btf *btf, u32 value_id) { + const struct bpf_struct_ops_desc *st_ops_list; unsigned int i; + u32 cnt = 0; - if (!value_id || !btf_vmlinux) + if (!value_id) 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) + return &st_ops_list[i]; } return NULL; @@ -285,14 +223,17 @@ bpf_struct_ops_find_value(struct btf *btf, u32 value_id) const struct bpf_struct_ops_desc * bpf_struct_ops_find(struct btf *btf, u32 type_id) { + const struct bpf_struct_ops_desc *st_ops_list; unsigned int i; + u32 cnt; - if (!type_id || !btf_vmlinux) + if (!type_id) 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) + return &st_ops_list[i]; } return NULL; @@ -429,6 +370,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, const struct bpf_struct_ops_desc *st_ops_desc = st_map->st_ops_desc; const struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; struct bpf_struct_ops_value *uvalue, *kvalue; + const struct btf_type *module_type; const struct btf_member *member; const struct btf_type *t = st_ops_desc->type; struct bpf_tramp_links *tlinks; @@ -485,6 +427,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, image = st_map->image; image_end = st_map->image + PAGE_SIZE; + module_type = btf_type_by_id(btf_vmlinux, st_ops_ids[idx_module_id]); for_each_member(i, t, member) { const struct btf_type *mtype, *ptype; struct bpf_prog *prog; 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 c1e2ed6c972e..c53888e8dddb 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -5790,8 +5790,6 @@ struct btf *btf_parse_vmlinux(void) /* btf_parse_vmlinux() runs under bpf_verifier_lock */ bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]); - bpf_struct_ops_init(btf, log); - refcount_set(&btf->refcnt, 1); err = btf_alloc_id(btf); @@ -7532,7 +7530,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; @@ -8672,3 +8670,40 @@ const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_c return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; } +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops) +{ + struct bpf_struct_ops_desc *desc; + struct bpf_verifier_log *log; + struct btf *btf; + int err = 0; + + if (st_ops == NULL) + return -EINVAL; + + btf = btf_get_module_btf(st_ops->owner); + if (!btf) + return -EINVAL; + + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); + if (!log) { + err = -ENOMEM; + goto errout; + } + + log->level = BPF_LOG_KERNEL; + + desc = btf_add_struct_ops(btf, st_ops); + if (IS_ERR(desc)) { + err = PTR_ERR(desc); + goto errout; + } + + bpf_struct_ops_init(desc, btf, log); + +errout: + kfree(log); + btf_put(btf); + + return err; +} +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index ffa224053a6c..148a5851c4fa 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, ...); @@ -223,11 +223,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, @@ -235,4 +237,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 3c8b76578a2a..b36a19274e5b 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), @@ -277,7 +277,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, @@ -287,10 +289,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] 6+ messages in thread
* Re: [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration 2023-10-22 5:03 ` [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration thinker.li @ 2023-10-26 21:02 ` Eduard Zingerman 2023-10-27 4:39 ` Kui-Feng Lee 0 siblings, 1 reply; 6+ messages in thread From: Eduard Zingerman @ 2023-10-26 21:02 UTC (permalink / raw) To: thinker.li, bpf, ast, martin.lau, song, kernel-team, andrii, drosen Cc: sinquersw, kuifeng, netdev On Sat, 2023-10-21 at 22:03 -0700, thinker.li@gmail.com wrote: > From: Kui-Feng Lee <thinker.li@gmail.com> > > Replace the static list of struct_ops types with per-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> Hello, I left two nitpicks below, feel free to ignore if you think is good as-is. > --- > include/linux/bpf.h | 36 +++++++-- > include/linux/btf.h | 5 +- > kernel/bpf/bpf_struct_ops.c | 123 ++++++++---------------------- > kernel/bpf/bpf_struct_ops_types.h | 12 --- > kernel/bpf/btf.c | 41 +++++++++- > net/bpf/bpf_dummy_struct_ops.c | 14 +++- > net/ipv4/bpf_tcp_ca.c | 16 +++- > 7 files changed, 130 insertions(+), 117 deletions(-) > delete mode 100644 kernel/bpf/bpf_struct_ops_types.h > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 26feb8a2da4f..a53b2181c845 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1644,7 +1644,6 @@ struct bpf_struct_ops_desc { > #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL) > #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA)) > const struct bpf_struct_ops_desc *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); > int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key, > @@ -1690,10 +1689,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf * > { > return NULL; > } > -static inline void bpf_struct_ops_init(struct btf *btf, > - struct bpf_verifier_log *log) > -{ > -} > static inline bool bpf_try_module_get(const void *data, struct module *owner) > { > return try_module_get(owner); > @@ -3212,4 +3207,35 @@ 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); > + > +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; > +}; > + > +/* 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 { \ > + struct bpf_struct_ops_common_value common; \ > + struct _name data ____cacheline_aligned_in_smp; \ > +} > + > +extern void bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc, > + struct btf *btf, > + struct bpf_verifier_log *log); > + > #endif /* _LINUX_BPF_H */ > diff --git a/include/linux/btf.h b/include/linux/btf.h > index 6a64b372b7a0..533db3f406b3 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, > @@ -574,8 +577,6 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type > struct bpf_struct_ops; > struct bpf_struct_ops_desc; > > -struct bpf_struct_ops_desc * > -btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops); > const struct bpf_struct_ops_desc * > btf_get_struct_ops(struct btf *btf, u32 *ret_cnt); > > diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c > index 99ab61cc6cad..44412f95bc32 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -13,21 +13,8 @@ > #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; > + struct bpf_struct_ops_common_value common; > 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_desc bpf_struct_ops[] = { > -#define BPF_STRUCT_OPS_TYPE(_name) \ > - [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name }, > -#include "bpf_struct_ops_types.h" > -#undef BPF_STRUCT_OPS_TYPE > -}; > - > const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = { > }; > > @@ -110,13 +68,22 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = { > #endif > }; > > -static const struct btf_type *module_type; > -static const struct btf_type *common_value_type; > +BTF_ID_LIST(st_ops_ids) > +BTF_ID(struct, module) > +BTF_ID(struct, bpf_struct_ops_common_value) > + > +enum { > + idx_module_id, > + idx_st_ops_common_value_id, > +}; > + > +extern struct btf *btf_vmlinux; > > static bool is_valid_value_type(struct btf *btf, s32 value_id, > const struct btf_type *type, > const char *value_name) > { > + const struct btf_type *common_value_type; > const struct btf_member *member; > const struct btf_type *vt, *mt; > > @@ -128,6 +95,8 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id, > } > member = btf_type_member(vt); > mt = btf_type_by_id(btf, member->type); > + common_value_type = btf_type_by_id(btf_vmlinux, > + st_ops_ids[idx_st_ops_common_value_id]); > if (mt != common_value_type) { > pr_warn("The first member of %s should be bpf_struct_ops_common_value\n", > value_name); > @@ -144,9 +113,9 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id, > return true; > } > > -static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc, > - struct btf *btf, > - struct bpf_verifier_log *log) > +void bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc, > + struct btf *btf, > + struct bpf_verifier_log *log) > { > struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; > const struct btf_member *member; > @@ -232,51 +201,20 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc, > } > } > > -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) > -{ > - struct bpf_struct_ops_desc *st_ops_desc; > - 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) { > - pr_warn("Cannot find struct module in btf_vmlinux\n"); > - 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_desc = &bpf_struct_ops[i]; > - bpf_struct_ops_init_one(st_ops_desc, btf, log); > - } > -} > - > -extern struct btf *btf_vmlinux; > - > static const struct bpf_struct_ops_desc * > bpf_struct_ops_find_value(struct btf *btf, u32 value_id) > { > + const struct bpf_struct_ops_desc *st_ops_list; > unsigned int i; > + u32 cnt = 0; > > - if (!value_id || !btf_vmlinux) > + if (!value_id) > 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) > + return &st_ops_list[i]; > } > > return NULL; > @@ -285,14 +223,17 @@ bpf_struct_ops_find_value(struct btf *btf, u32 value_id) > const struct bpf_struct_ops_desc * > bpf_struct_ops_find(struct btf *btf, u32 type_id) > { > + const struct bpf_struct_ops_desc *st_ops_list; > unsigned int i; > + u32 cnt; > > - if (!type_id || !btf_vmlinux) > + if (!type_id) > 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) > + return &st_ops_list[i]; > } > > return NULL; > @@ -429,6 +370,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > const struct bpf_struct_ops_desc *st_ops_desc = st_map->st_ops_desc; > const struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; > struct bpf_struct_ops_value *uvalue, *kvalue; > + const struct btf_type *module_type; > const struct btf_member *member; > const struct btf_type *t = st_ops_desc->type; > struct bpf_tramp_links *tlinks; > @@ -485,6 +427,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, > image = st_map->image; > image_end = st_map->image + PAGE_SIZE; > > + module_type = btf_type_by_id(btf_vmlinux, st_ops_ids[idx_module_id]); > for_each_member(i, t, member) { > const struct btf_type *mtype, *ptype; > struct bpf_prog *prog; > 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 c1e2ed6c972e..c53888e8dddb 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -5790,8 +5790,6 @@ struct btf *btf_parse_vmlinux(void) > /* btf_parse_vmlinux() runs under bpf_verifier_lock */ > bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]); > > - bpf_struct_ops_init(btf, log); > - > refcount_set(&btf->refcnt, 1); > > err = btf_alloc_id(btf); > @@ -7532,7 +7530,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; > @@ -8672,3 +8670,40 @@ const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_c > return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; > } > > +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops) > +{ > + struct bpf_struct_ops_desc *desc; > + struct bpf_verifier_log *log; > + struct btf *btf; > + int err = 0; > + > + if (st_ops == NULL) > + return -EINVAL; > + > + btf = btf_get_module_btf(st_ops->owner); > + if (!btf) > + return -EINVAL; > + > + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); > + if (!log) { > + err = -ENOMEM; > + goto errout; > + } > + > + log->level = BPF_LOG_KERNEL; Nit: maybe use bpf_vlog_init() here to avoid breaking encapsulation? > + > + desc = btf_add_struct_ops(btf, st_ops); > + if (IS_ERR(desc)) { > + err = PTR_ERR(desc); > + goto errout; > + } > + > + bpf_struct_ops_init(desc, btf, log); Nit: I think bpf_struct_ops_init() could be changed to return 'int', then register_bpf_struct_ops() could report to calling module if something went wrong on the last phase, wdyt? > + > +errout: > + kfree(log); > + btf_put(btf); > + > + return err; > +} > +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); > diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c > index ffa224053a6c..148a5851c4fa 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, ...); > @@ -223,11 +223,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, > @@ -235,4 +237,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 3c8b76578a2a..b36a19274e5b 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), > @@ -277,7 +277,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, > @@ -287,10 +289,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); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration 2023-10-26 21:02 ` Eduard Zingerman @ 2023-10-27 4:39 ` Kui-Feng Lee 2023-10-27 21:32 ` Kui-Feng Lee 0 siblings, 1 reply; 6+ messages in thread From: Kui-Feng Lee @ 2023-10-27 4:39 UTC (permalink / raw) To: Eduard Zingerman, thinker.li, bpf, ast, martin.lau, song, kernel-team, andrii, drosen Cc: kuifeng, netdev On 10/26/23 14:02, Eduard Zingerman wrote: > On Sat, 2023-10-21 at 22:03 -0700, thinker.li@gmail.com wrote: >> From: Kui-Feng Lee <thinker.li@gmail.com> >> >> Replace the static list of struct_ops types with per-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> > > Hello, > > I left two nitpicks below, feel free to ignore if you think is good as-is. > >> --- >> include/linux/bpf.h | 36 +++++++-- >> include/linux/btf.h | 5 +- >> kernel/bpf/bpf_struct_ops.c | 123 ++++++++---------------------- >> kernel/bpf/bpf_struct_ops_types.h | 12 --- >> kernel/bpf/btf.c | 41 +++++++++- >> net/bpf/bpf_dummy_struct_ops.c | 14 +++- >> net/ipv4/bpf_tcp_ca.c | 16 +++- >> 7 files changed, 130 insertions(+), 117 deletions(-) >> delete mode 100644 kernel/bpf/bpf_struct_ops_types.h >> >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h >> index 26feb8a2da4f..a53b2181c845 100644 >> --- a/include/linux/bpf.h >> +++ b/include/linux/bpf.h >> @@ -1644,7 +1644,6 @@ struct bpf_struct_ops_desc { >> #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL) >> #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA)) >> const struct bpf_struct_ops_desc *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); >> int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key, >> @@ -1690,10 +1689,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf * >> { >> return NULL; >> } >> -static inline void bpf_struct_ops_init(struct btf *btf, >> - struct bpf_verifier_log *log) >> -{ >> -} >> static inline bool bpf_try_module_get(const void *data, struct module *owner) >> { >> return try_module_get(owner); >> @@ -3212,4 +3207,35 @@ 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); >> + >> +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; >> +}; >> + >> +/* 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 { \ >> + struct bpf_struct_ops_common_value common; \ >> + struct _name data ____cacheline_aligned_in_smp; \ >> +} >> + >> +extern void bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc, >> + struct btf *btf, >> + struct bpf_verifier_log *log); >> + >> #endif /* _LINUX_BPF_H */ >> diff --git a/include/linux/btf.h b/include/linux/btf.h >> index 6a64b372b7a0..533db3f406b3 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, >> @@ -574,8 +577,6 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type >> struct bpf_struct_ops; >> struct bpf_struct_ops_desc; >> >> -struct bpf_struct_ops_desc * >> -btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops); >> const struct bpf_struct_ops_desc * >> btf_get_struct_ops(struct btf *btf, u32 *ret_cnt); >> >> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c >> index 99ab61cc6cad..44412f95bc32 100644 >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -13,21 +13,8 @@ >> #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; >> + struct bpf_struct_ops_common_value common; >> 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_desc bpf_struct_ops[] = { >> -#define BPF_STRUCT_OPS_TYPE(_name) \ >> - [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name }, >> -#include "bpf_struct_ops_types.h" >> -#undef BPF_STRUCT_OPS_TYPE >> -}; >> - >> const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = { >> }; >> >> @@ -110,13 +68,22 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = { >> #endif >> }; >> >> -static const struct btf_type *module_type; >> -static const struct btf_type *common_value_type; >> +BTF_ID_LIST(st_ops_ids) >> +BTF_ID(struct, module) >> +BTF_ID(struct, bpf_struct_ops_common_value) >> + >> +enum { >> + idx_module_id, >> + idx_st_ops_common_value_id, >> +}; >> + >> +extern struct btf *btf_vmlinux; >> >> static bool is_valid_value_type(struct btf *btf, s32 value_id, >> const struct btf_type *type, >> const char *value_name) >> { >> + const struct btf_type *common_value_type; >> const struct btf_member *member; >> const struct btf_type *vt, *mt; >> >> @@ -128,6 +95,8 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id, >> } >> member = btf_type_member(vt); >> mt = btf_type_by_id(btf, member->type); >> + common_value_type = btf_type_by_id(btf_vmlinux, >> + st_ops_ids[idx_st_ops_common_value_id]); >> if (mt != common_value_type) { >> pr_warn("The first member of %s should be bpf_struct_ops_common_value\n", >> value_name); >> @@ -144,9 +113,9 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id, >> return true; >> } >> >> -static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc, >> - struct btf *btf, >> - struct bpf_verifier_log *log) >> +void bpf_struct_ops_init(struct bpf_struct_ops_desc *st_ops_desc, >> + struct btf *btf, >> + struct bpf_verifier_log *log) >> { >> struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; >> const struct btf_member *member; >> @@ -232,51 +201,20 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops_desc *st_ops_desc, >> } >> } >> >> -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log) >> -{ >> - struct bpf_struct_ops_desc *st_ops_desc; >> - 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) { >> - pr_warn("Cannot find struct module in btf_vmlinux\n"); >> - 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_desc = &bpf_struct_ops[i]; >> - bpf_struct_ops_init_one(st_ops_desc, btf, log); >> - } >> -} >> - >> -extern struct btf *btf_vmlinux; >> - >> static const struct bpf_struct_ops_desc * >> bpf_struct_ops_find_value(struct btf *btf, u32 value_id) >> { >> + const struct bpf_struct_ops_desc *st_ops_list; >> unsigned int i; >> + u32 cnt = 0; >> >> - if (!value_id || !btf_vmlinux) >> + if (!value_id) >> 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) >> + return &st_ops_list[i]; >> } >> >> return NULL; >> @@ -285,14 +223,17 @@ bpf_struct_ops_find_value(struct btf *btf, u32 value_id) >> const struct bpf_struct_ops_desc * >> bpf_struct_ops_find(struct btf *btf, u32 type_id) >> { >> + const struct bpf_struct_ops_desc *st_ops_list; >> unsigned int i; >> + u32 cnt; >> >> - if (!type_id || !btf_vmlinux) >> + if (!type_id) >> 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) >> + return &st_ops_list[i]; >> } >> >> return NULL; >> @@ -429,6 +370,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, >> const struct bpf_struct_ops_desc *st_ops_desc = st_map->st_ops_desc; >> const struct bpf_struct_ops *st_ops = st_ops_desc->st_ops; >> struct bpf_struct_ops_value *uvalue, *kvalue; >> + const struct btf_type *module_type; >> const struct btf_member *member; >> const struct btf_type *t = st_ops_desc->type; >> struct bpf_tramp_links *tlinks; >> @@ -485,6 +427,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key, >> image = st_map->image; >> image_end = st_map->image + PAGE_SIZE; >> >> + module_type = btf_type_by_id(btf_vmlinux, st_ops_ids[idx_module_id]); >> for_each_member(i, t, member) { >> const struct btf_type *mtype, *ptype; >> struct bpf_prog *prog; >> 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 c1e2ed6c972e..c53888e8dddb 100644 >> --- a/kernel/bpf/btf.c >> +++ b/kernel/bpf/btf.c >> @@ -5790,8 +5790,6 @@ struct btf *btf_parse_vmlinux(void) >> /* btf_parse_vmlinux() runs under bpf_verifier_lock */ >> bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]); >> >> - bpf_struct_ops_init(btf, log); >> - >> refcount_set(&btf->refcnt, 1); >> >> err = btf_alloc_id(btf); >> @@ -7532,7 +7530,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; >> @@ -8672,3 +8670,40 @@ const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_c >> return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops; >> } >> >> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops) >> +{ >> + struct bpf_struct_ops_desc *desc; >> + struct bpf_verifier_log *log; >> + struct btf *btf; >> + int err = 0; >> + >> + if (st_ops == NULL) >> + return -EINVAL; >> + >> + btf = btf_get_module_btf(st_ops->owner); >> + if (!btf) >> + return -EINVAL; >> + >> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); >> + if (!log) { >> + err = -ENOMEM; >> + goto errout; >> + } >> + >> + log->level = BPF_LOG_KERNEL; > > Nit: maybe use bpf_vlog_init() here to avoid breaking encapsulation? Agree! > >> + >> + desc = btf_add_struct_ops(btf, st_ops); >> + if (IS_ERR(desc)) { >> + err = PTR_ERR(desc); >> + goto errout; >> + } >> + >> + bpf_struct_ops_init(desc, btf, log); > > Nit: I think bpf_struct_ops_init() could be changed to return 'int', > then register_bpf_struct_ops() could report to calling module if > something went wrong on the last phase, wdyt? Agree! > >> + >> +errout: >> + kfree(log); >> + btf_put(btf); >> + >> + return err; >> +} >> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); >> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c >> index ffa224053a6c..148a5851c4fa 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, ...); >> @@ -223,11 +223,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, >> @@ -235,4 +237,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 3c8b76578a2a..b36a19274e5b 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), >> @@ -277,7 +277,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, >> @@ -287,10 +289,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); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration 2023-10-27 4:39 ` Kui-Feng Lee @ 2023-10-27 21:32 ` Kui-Feng Lee 2023-10-27 22:02 ` Eduard Zingerman 0 siblings, 1 reply; 6+ messages in thread From: Kui-Feng Lee @ 2023-10-27 21:32 UTC (permalink / raw) To: Eduard Zingerman, thinker.li, bpf, ast, martin.lau, song, kernel-team, andrii, drosen Cc: kuifeng, netdev On 10/26/23 21:39, Kui-Feng Lee wrote: > > > On 10/26/23 14:02, Eduard Zingerman wrote: >> On Sat, 2023-10-21 at 22:03 -0700, thinker.li@gmail.com wrote: >>> From: Kui-Feng Lee <thinker.li@gmail.com> [...] >>> + >>> + btf = btf_get_module_btf(st_ops->owner); >>> + if (!btf) >>> + return -EINVAL; >>> + >>> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); >>> + if (!log) { >>> + err = -ENOMEM; >>> + goto errout; >>> + } >>> + >>> + log->level = BPF_LOG_KERNEL; >> >> Nit: maybe use bpf_vlog_init() here to avoid breaking encapsulation? > > Agree! > I don't use bpf_vlog_init() eventually. I found bpf_vlog_init() is not for BPF_LOG_KERNEL. According to the comment next to BPF_LOG_KERNEL, it is an internal log level. According to the code of bpf_vlog_init(), the level passing to bpf_vlog_init() should be covered by BPF_LOG_MASK. BPF_LOG_KERNEL is defined as BPF_LOG_MASK + 1. So, it is intended not being used with bpf_vlog_init(). >> >>> + >>> + desc = btf_add_struct_ops(btf, st_ops); >>> + if (IS_ERR(desc)) { >>> + err = PTR_ERR(desc); >>> + goto errout; >>> + } >>> + >>> + bpf_struct_ops_init(desc, btf, log); >> >> Nit: I think bpf_struct_ops_init() could be changed to return 'int', >> then register_bpf_struct_ops() could report to calling module if >> something went wrong on the last phase, wdyt? > > > Agree! > >> >>> + >>> +errout: >>> + kfree(log); >>> + btf_put(btf); >>> + >>> + return err; >>> +} >>> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); >>> diff --git a/net/bpf/bpf_dummy_struct_ops.c >>> b/net/bpf/bpf_dummy_struct_ops.c >>> index ffa224053a6c..148a5851c4fa 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, ...); >>> @@ -223,11 +223,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, >>> @@ -235,4 +237,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 3c8b76578a2a..b36a19274e5b 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), >>> @@ -277,7 +277,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, >>> @@ -287,10 +289,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); >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration 2023-10-27 21:32 ` Kui-Feng Lee @ 2023-10-27 22:02 ` Eduard Zingerman 0 siblings, 0 replies; 6+ messages in thread From: Eduard Zingerman @ 2023-10-27 22:02 UTC (permalink / raw) To: Kui-Feng Lee, thinker.li, bpf, ast, martin.lau, song, kernel-team, andrii, drosen Cc: kuifeng, netdev On Fri, 2023-10-27 at 14:32 -0700, Kui-Feng Lee wrote: > > On 10/26/23 21:39, Kui-Feng Lee wrote: > > > > > > On 10/26/23 14:02, Eduard Zingerman wrote: > > > On Sat, 2023-10-21 at 22:03 -0700, thinker.li@gmail.com wrote: > > > > From: Kui-Feng Lee <thinker.li@gmail.com> > [...] > > > > + > > > > + btf = btf_get_module_btf(st_ops->owner); > > > > + if (!btf) > > > > + return -EINVAL; > > > > + > > > > + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); > > > > + if (!log) { > > > > + err = -ENOMEM; > > > > + goto errout; > > > > + } > > > > + > > > > + log->level = BPF_LOG_KERNEL; > > > > > > Nit: maybe use bpf_vlog_init() here to avoid breaking encapsulation? > > > > Agree! > > > > I don't use bpf_vlog_init() eventually. > > I found bpf_vlog_init() is not for BPF_LOG_KERNEL. > According to the comment next to BPF_LOG_KERNEL, it > is an internal log level. > According to the code of bpf_vlog_init(), the level passing to > bpf_vlog_init() should be covered by BPF_LOG_MASK. BPF_LOG_KERNEL is > defined as BPF_LOG_MASK + 1. So, it is intended not being used with > bpf_vlog_init(). I see, looks like btf_parse_vmlinux does the same, sorry should have checked there. Thank you for looking into it. > > > > > > > > + > > > > + desc = btf_add_struct_ops(btf, st_ops); > > > > + if (IS_ERR(desc)) { > > > > + err = PTR_ERR(desc); > > > > + goto errout; > > > > + } > > > > + > > > > + bpf_struct_ops_init(desc, btf, log); > > > > > > Nit: I think bpf_struct_ops_init() could be changed to return 'int', > > > then register_bpf_struct_ops() could report to calling module if > > > something went wrong on the last phase, wdyt? > > > > > > Agree! > > > > > > > > > + > > > > +errout: > > > > + kfree(log); > > > > + btf_put(btf); > > > > + > > > > + return err; > > > > +} > > > > +EXPORT_SYMBOL_GPL(register_bpf_struct_ops); > > > > diff --git a/net/bpf/bpf_dummy_struct_ops.c > > > > b/net/bpf/bpf_dummy_struct_ops.c > > > > index ffa224053a6c..148a5851c4fa 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, ...); > > > > @@ -223,11 +223,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, > > > > @@ -235,4 +237,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 3c8b76578a2a..b36a19274e5b 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), > > > > @@ -277,7 +277,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, > > > > @@ -287,10 +289,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); > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-27 22:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231022050335.2579051-1-thinker.li@gmail.com>
2023-10-22 5:03 ` [PATCH bpf-next v6 02/10] bpf, net: introduce bpf_struct_ops_desc thinker.li
2023-10-22 5:03 ` [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration thinker.li
2023-10-26 21:02 ` Eduard Zingerman
2023-10-27 4:39 ` Kui-Feng Lee
2023-10-27 21:32 ` Kui-Feng Lee
2023-10-27 22:02 ` Eduard Zingerman
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).