Message ID | 20230913061449.1918219-3-thinker.li@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | BPF |
Headers | show |
Series | Registrating struct_ops types from modules | expand |
On 9/12/23 11:14 PM, thinker.li@gmail.com wrote: > +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod) > +{ > + struct bpf_struct_ops *st_ops = mod->st_ops; > + struct bpf_verifier_log *log; > + struct btf *btf; > + int err; > + > + if (mod->st_ops == NULL || > + mod->owner == NULL) > + return -EINVAL; > + > + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); > + if (!log) { > + err = -ENOMEM; > + goto errout; > + } > + > + log->level = BPF_LOG_KERNEL; > + > + btf = btf_get_module_btf(mod->owner); Where is btf_put called? It is not stored anywhere in patch 2, so a bit confusing. I quickly looked at the following patches but also don't see the bpf_put. > + if (!btf) { > + err = -EINVAL; > + goto errout; > + } > + > + bpf_struct_ops_init_one(st_ops, btf, log); > + err = add_struct_ops(st_ops); > + > +errout: > + kfree(log); > + > + return err; > +} > +EXPORT_SYMBOL(register_bpf_struct_ops); > + > +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod) It is not clear to me why the subsystem needs to explicitly call unregister_bpf_struct_ops(). Can it be done similar to the module kfunc support (the kfunc_set_tab goes away with the btf)? Related to this, does it need to maintain a global struct_ops array for all kernel module? Can the struct_ops be maintained under its corresponding module btf itself? > +{ > + struct bpf_struct_ops *st_ops = mod->st_ops; > + int err; > + > + err = remove_struct_ops(st_ops); > + if (!err && st_ops->uninit) > + err = st_ops->uninit(); > + > + return err; > +} > +EXPORT_SYMBOL(unregister_bpf_struct_ops);
On 9/15/23 17:05, Martin KaFai Lau wrote: > On 9/12/23 11:14 PM, thinker.li@gmail.com wrote: >> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod) >> +{ >> + struct bpf_struct_ops *st_ops = mod->st_ops; >> + struct bpf_verifier_log *log; >> + struct btf *btf; >> + int err; >> + >> + if (mod->st_ops == NULL || >> + mod->owner == NULL) >> + return -EINVAL; >> + >> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); >> + if (!log) { >> + err = -ENOMEM; >> + goto errout; >> + } >> + >> + log->level = BPF_LOG_KERNEL; >> + >> + btf = btf_get_module_btf(mod->owner); > > Where is btf_put called? > > It is not stored anywhere in patch 2, so a bit confusing. I quickly > looked at the following patches but also don't see the bpf_put. It is my fault to use it without calling btf_put(). I miss-understood the API, thought it doesn't increase refcount by mistake. > >> + if (!btf) { >> + err = -EINVAL; >> + goto errout; >> + } >> + >> + bpf_struct_ops_init_one(st_ops, btf, log); >> + err = add_struct_ops(st_ops); >> + >> +errout: >> + kfree(log); >> + >> + return err; >> +} >> +EXPORT_SYMBOL(register_bpf_struct_ops); >> + >> +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod) > > It is not clear to me why the subsystem needs to explicitly call > unregister_bpf_struct_ops(). Can it be done similar to the module kfunc > support (the kfunc_set_tab goes away with the btf)? It could be. However, registering to module notifier (register_module_notifier()) is more straight forward if we go this way. What do you think? > > Related to this, does it need to maintain a global struct_ops array for > all kernel module? Can the struct_ops be maintained under its > corresponding module btf itself? What is the purpose? We have a global struct_ops array already, although it is not per-module. For now, the number of struct_ops is pretty small. We have only one so far, and it is unlikely to grow fast in near future. It is probably a bit overkill to have per-module ones if this is what you mean. > >> +{ >> + struct bpf_struct_ops *st_ops = mod->st_ops; >> + int err; >> + >> + err = remove_struct_ops(st_ops); >> + if (!err && st_ops->uninit) >> + err = st_ops->uninit(); >> + >> + return err; >> +} >> +EXPORT_SYMBOL(unregister_bpf_struct_ops); > >
On 9/15/23 6:14 PM, Kui-Feng Lee wrote: > > > On 9/15/23 17:05, Martin KaFai Lau wrote: >> On 9/12/23 11:14 PM, thinker.li@gmail.com wrote: >>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod) >>> +{ >>> + struct bpf_struct_ops *st_ops = mod->st_ops; >>> + struct bpf_verifier_log *log; >>> + struct btf *btf; >>> + int err; >>> + >>> + if (mod->st_ops == NULL || >>> + mod->owner == NULL) >>> + return -EINVAL; >>> + >>> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); >>> + if (!log) { >>> + err = -ENOMEM; >>> + goto errout; >>> + } >>> + >>> + log->level = BPF_LOG_KERNEL; >>> + >>> + btf = btf_get_module_btf(mod->owner); >> >> Where is btf_put called? >> >> It is not stored anywhere in patch 2, so a bit confusing. I quickly looked at >> the following patches but also don't see the bpf_put. > > It is my fault to use it without calling btf_put(). > I miss-understood the API, thought it doesn't increase refcount by > mistake. > >> >>> + if (!btf) { >>> + err = -EINVAL; >>> + goto errout; >>> + } >>> + >>> + bpf_struct_ops_init_one(st_ops, btf, log); >>> + err = add_struct_ops(st_ops); >>> + >>> +errout: >>> + kfree(log); >>> + >>> + return err; >>> +} >>> +EXPORT_SYMBOL(register_bpf_struct_ops); >>> + >>> +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod) >> >> It is not clear to me why the subsystem needs to explicitly call >> unregister_bpf_struct_ops(). Can it be done similar to the module kfunc >> support (the kfunc_set_tab goes away with the btf)? > > It could be. However, registering to module notifier > (register_module_notifier()) is more straight forward if we go > this way. What do you think? Right, but not sure if struct_ops needs to create yet another notifier considering there is already a btf_module_notify(). It is why the earlier question on btf_put because I was trying to understand if the struct_ops can go away together during btf_free. More on this next. > >> >> Related to this, does it need to maintain a global struct_ops array for all >> kernel module? Can the struct_ops be maintained under its corresponding module >> btf itself? > > What is the purpose? > We have a global struct_ops array already, although it is not > per-module. For now, the number of struct_ops is pretty small. > We have only one so far, and it is unlikely to grow fast in > near future. It is probably a bit overkill to have > per-module ones if this is what you mean. The array size is not the concern. The global struct_ops array was created before btf supporting kernel module. Since then, btf module and kfunc module support were added. To maintain this global struct_ops array, it needs to register its own module notifier, maintains its own mutex_lock (in patch 5), and also the modified bpf_struct_ops_find*() is searching something under a specific btf module. afaict, the current btf kfunc support has the infrastructure to do all these (for example, the global LIST_HEAD(btf_modules), btf_module_mutex, btf_module_notify()...etc). Why struct_ops needs to be special and reinvent something which looks very similar to btf kfunc? Did I missing something that struct_ops needs special handling? > >> >>> +{ >>> + struct bpf_struct_ops *st_ops = mod->st_ops; >>> + int err; >>> + >>> + err = remove_struct_ops(st_ops); >>> + if (!err && st_ops->uninit) >>> + err = st_ops->uninit(); >>> + >>> + return err; >>> +} >>> +EXPORT_SYMBOL(unregister_bpf_struct_ops); >> >>
On 9/18/23 11:47, Martin KaFai Lau wrote: > On 9/15/23 6:14 PM, Kui-Feng Lee wrote: >> >> >> On 9/15/23 17:05, Martin KaFai Lau wrote: >>> On 9/12/23 11:14 PM, thinker.li@gmail.com wrote: >>>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod) >>>> +{ >>>> + struct bpf_struct_ops *st_ops = mod->st_ops; >>>> + struct bpf_verifier_log *log; >>>> + struct btf *btf; >>>> + int err; >>>> + >>>> + if (mod->st_ops == NULL || >>>> + mod->owner == NULL) >>>> + return -EINVAL; >>>> + >>>> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); >>>> + if (!log) { >>>> + err = -ENOMEM; >>>> + goto errout; >>>> + } >>>> + >>>> + log->level = BPF_LOG_KERNEL; >>>> + >>>> + btf = btf_get_module_btf(mod->owner); >>> >>> Where is btf_put called? >>> >>> It is not stored anywhere in patch 2, so a bit confusing. I quickly >>> looked at the following patches but also don't see the bpf_put. >> >> It is my fault to use it without calling btf_put(). >> I miss-understood the API, thought it doesn't increase refcount by >> mistake. >> >>> >>>> + if (!btf) { >>>> + err = -EINVAL; >>>> + goto errout; >>>> + } >>>> + >>>> + bpf_struct_ops_init_one(st_ops, btf, log); >>>> + err = add_struct_ops(st_ops); >>>> + >>>> +errout: >>>> + kfree(log); >>>> + >>>> + return err; >>>> +} >>>> +EXPORT_SYMBOL(register_bpf_struct_ops); >>>> + >>>> +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod) >>> >>> It is not clear to me why the subsystem needs to explicitly call >>> unregister_bpf_struct_ops(). Can it be done similar to the module >>> kfunc support (the kfunc_set_tab goes away with the btf)? >> >> It could be. However, registering to module notifier >> (register_module_notifier()) is more straight forward if we go >> this way. What do you think? > > Right, but not sure if struct_ops needs to create yet another notifier > considering there is already a btf_module_notify(). It is why the > earlier question on btf_put because I was trying to understand if the > struct_ops can go away together during btf_free. More on this next. In short, it is not necessary to have another notifier. The benefit with a separated notifier is loose coupling without touching btf code. I don't have a strong opinion on this. > >> >>> >>> Related to this, does it need to maintain a global struct_ops array >>> for all kernel module? Can the struct_ops be maintained under its >>> corresponding module btf itself? >> >> What is the purpose? >> We have a global struct_ops array already, although it is not >> per-module. For now, the number of struct_ops is pretty small. >> We have only one so far, and it is unlikely to grow fast in >> near future. It is probably a bit overkill to have >> per-module ones if this is what you mean. > > The array size is not the concern. > > The global struct_ops array was created before btf supporting kernel > module. Since then, btf module and kfunc module support were added. > > To maintain this global struct_ops array, it needs to register its own > module notifier, maintains its own mutex_lock (in patch 5), and also the > modified bpf_struct_ops_find*() is searching something under a specific > btf module. > > afaict, the current btf kfunc support has the infrastructure to do all > these (for example, the global LIST_HEAD(btf_modules), btf_module_mutex, > btf_module_notify()...etc). Why struct_ops needs to be special and > reinvent something which looks very similar to btf kfunc? Did I missing > something that struct_ops needs special handling? I don't think you missing anything. > >> >>> >>>> +{ >>>> + struct bpf_struct_ops *st_ops = mod->st_ops; >>>> + int err; >>>> + >>>> + err = remove_struct_ops(st_ops); >>>> + if (!err && st_ops->uninit) >>>> + err = st_ops->uninit(); >>>> + >>>> + return err; >>>> +} >>>> +EXPORT_SYMBOL(unregister_bpf_struct_ops); >>> >>> >
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 024e8b28c34b..10f98f0ddc77 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1561,6 +1561,8 @@ struct btf_member; * @init: A callback that is invoked a single time, and before any other * callback, to initialize the structure. A nonzero return value means * the subsystem could not be initialized. + * @uninit: A callback that is invoked after any other + * callback to release resources used by the subsystem. * @check_member: When defined, a callback invoked by the verifier to allow * the subsystem to determine if an entry in the struct_ops map * is valid. A nonzero return value means that the map is @@ -1601,6 +1603,7 @@ struct btf_member; struct bpf_struct_ops { const struct bpf_verifier_ops *verifier_ops; int (*init)(struct btf *btf); + int (*uninit)(void); int (*check_member)(const struct btf_type *t, const struct btf_member *member, const struct bpf_prog *prog); @@ -1619,6 +1622,11 @@ struct bpf_struct_ops { u32 value_id; }; +struct bpf_struct_ops_mod { + struct module *owner; + struct bpf_struct_ops *st_ops; +}; + #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL) #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA)) const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id); @@ -3183,4 +3191,9 @@ static inline gfp_t bpf_memcg_flags(gfp_t flags) return flags; } +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod); +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod); +#endif + #endif /* _LINUX_BPF_H */ diff --git a/include/linux/btf.h b/include/linux/btf.h index 928113a80a95..d6ed3d99ba41 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h @@ -200,6 +200,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, diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index 1662875e0ebe..9be6e07ccba5 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -92,12 +92,15 @@ enum { __NR_BPF_STRUCT_OPS_TYPE, }; -static struct bpf_struct_ops * const bpf_struct_ops[] = { +static struct bpf_struct_ops *bpf_struct_ops_static[] = { #define BPF_STRUCT_OPS_TYPE(_name) \ [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name, #include "bpf_struct_ops_types.h" #undef BPF_STRUCT_OPS_TYPE }; +static struct bpf_struct_ops **bpf_struct_ops; +static int bpf_struct_ops_num; +static int bpf_struct_ops_capacity; const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = { }; @@ -212,12 +215,116 @@ 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++) { + bpf_struct_ops_num = ARRAY_SIZE(bpf_struct_ops_static); + bpf_struct_ops_capacity = bpf_struct_ops_num; + bpf_struct_ops = bpf_struct_ops_static; + + for (i = 0; i < bpf_struct_ops_num; i++) { st_ops = bpf_struct_ops[i]; bpf_struct_ops_init_one(st_ops, btf, log); } } +static int add_struct_ops(struct bpf_struct_ops *st_ops) +{ + struct bpf_struct_ops **new_ops; + int i; + + for (i = 0; i < bpf_struct_ops_num; i++) { + if (bpf_struct_ops[i] == st_ops) + return -EEXIST; + if (strcmp(bpf_struct_ops[i]->name, st_ops->name) == 0) + return -EEXIST; + } + + if (bpf_struct_ops_num == bpf_struct_ops_capacity) { + if (bpf_struct_ops == bpf_struct_ops_static) { + new_ops = kmalloc_array(((bpf_struct_ops_capacity + 0x7) & ~0x7) * 2, + sizeof(*new_ops), + GFP_KERNEL); + if (!new_ops) + return -ENOMEM; + memcpy(new_ops, bpf_struct_ops, + sizeof(*new_ops) * bpf_struct_ops_num); + } else { + new_ops = krealloc_array(bpf_struct_ops, + bpf_struct_ops_capacity * 2, + sizeof(*new_ops), + GFP_KERNEL); + if (!new_ops) + return -ENOMEM; + } + bpf_struct_ops = new_ops; + bpf_struct_ops_capacity *= 2; + } + + bpf_struct_ops[bpf_struct_ops_num++] = st_ops; + return 0; +} + +static int remove_struct_ops(struct bpf_struct_ops *st_ops) +{ + int i; + + for (i = 0; i < bpf_struct_ops_num; i++) { + if (bpf_struct_ops[i] == st_ops) { + bpf_struct_ops_num--; + bpf_struct_ops[i] = bpf_struct_ops[bpf_struct_ops_num]; + return 0; + } + } + + return -ENOENT; +} + +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod) +{ + struct bpf_struct_ops *st_ops = mod->st_ops; + struct bpf_verifier_log *log; + struct btf *btf; + int err; + + if (mod->st_ops == NULL || + mod->owner == NULL) + return -EINVAL; + + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN); + if (!log) { + err = -ENOMEM; + goto errout; + } + + log->level = BPF_LOG_KERNEL; + + btf = btf_get_module_btf(mod->owner); + if (!btf) { + err = -EINVAL; + goto errout; + } + + bpf_struct_ops_init_one(st_ops, btf, log); + err = add_struct_ops(st_ops); + +errout: + kfree(log); + + return err; +} +EXPORT_SYMBOL(register_bpf_struct_ops); + +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod) +{ + struct bpf_struct_ops *st_ops = mod->st_ops; + int err; + + err = remove_struct_ops(st_ops); + if (!err && st_ops->uninit) + err = st_ops->uninit(); + + return err; +} +EXPORT_SYMBOL(unregister_bpf_struct_ops); + extern struct btf *btf_vmlinux; static const struct bpf_struct_ops * @@ -228,7 +335,7 @@ bpf_struct_ops_find_value(u32 value_id) if (!value_id || !btf_vmlinux) return NULL; - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { + for (i = 0; i < bpf_struct_ops_num; i++) { if (bpf_struct_ops[i]->value_id == value_id) return bpf_struct_ops[i]; } @@ -243,7 +350,7 @@ const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id) if (!type_id || !btf_vmlinux) return NULL; - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) { + for (i = 0; i < bpf_struct_ops_num; i++) { if (bpf_struct_ops[i]->type_id == type_id) return bpf_struct_ops[i]; } diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 1095bbe29859..55d76d85c6ec 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -7498,7 +7498,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;