lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <f2c33ec4-339d-464d-893e-4f5ba0b9c294@gmail.com> Date: Thu, 26 Oct 2023 21:39:58 -0700 From: Kui-Feng Lee <sinquersw@...il.com> To: Eduard Zingerman <eddyz87@...il.com>, thinker.li@...il.com, bpf@...r.kernel.org, ast@...nel.org, martin.lau@...ux.dev, song@...nel.org, kernel-team@...a.com, andrii@...nel.org, drosen@...gle.com Cc: kuifeng@...a.com, netdev@...r.kernel.org Subject: Re: [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration On 10/26/23 14:02, Eduard Zingerman wrote: > On Sat, 2023-10-21 at 22:03 -0700, thinker.li@...il.com wrote: >> From: Kui-Feng Lee <thinker.li@...il.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@...r.kernel.org >> Signed-off-by: Kui-Feng Lee <thinker.li@...il.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); >
Powered by blists - more mailing lists