On Sat, Jan 22, 2022 at 01:09:54AM IST, Usama Arif wrote:
> This adds support for calling helper functions in eBPF applications
> that have been declared in a kernel module. The corresponding
> verifier changes for module helpers will be added in a later patch.
>
> Module helpers are useful as:
> - They support more argument and return types when compared to module
> kfunc.
> - This adds a way to have helper functions that would be too specialized
> for a specific usecase to merge upstream, but are functions that can have
> a constant API and can be maintained in-kernel modules.
> - The number of in-kernel helpers have grown to a large number
> (187 at the time of writing this commit). Having module helper functions
> could possibly reduce the number of in-kernel helper functions growing
> in the future and maintained upstream.
>
> When the kernel module registers the helper, the module owner,
> BTF id set of the function and function proto is stored as part of a
> btf_mod_helper entry in a btf_mod_helper_list which is part of
> struct btf. This entry can be removed in the unregister function
> while exiting the module, and can be used by the bpf verifier to
> check the helper call and get function proto.
>
> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxxxxxx>
> ---
> include/linux/btf.h | 44 +++++++++++++++++++++++
> kernel/bpf/btf.c | 88 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 132 insertions(+)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index b12cfe3b12bb..c3a814404112 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -40,6 +40,18 @@ struct btf_kfunc_id_set {
> };
> };
>
> +struct btf_mod_helper {
> + struct list_head list;
> + struct module *owner;
> + struct btf_id_set *set;
> + struct bpf_func_proto *func_proto;
> +};
> +
> +struct btf_mod_helper_list {
> + struct list_head list;
> + struct mutex mutex;
> +};
> +
> extern const struct file_operations btf_fops;
>
> void btf_get(struct btf *btf);
> @@ -359,4 +371,36 @@ static inline int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
> }
> #endif
>
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> +int register_mod_helper(struct btf_mod_helper *mod_helper);
> +int unregister_mod_helper(struct btf_mod_helper *mod_helper);
> +const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
> + const u32 kfunc_btf_id);
> +
> +#define DEFINE_MOD_HELPER(mod_helper, owner, helper_func, func_proto) \
> +BTF_SET_START(helper_func##__id_set) \
> +BTF_ID(func, helper_func) \
> +BTF_SET_END(helper_func##__id_set) \
> +struct btf_mod_helper mod_helper = { \
> + LIST_HEAD_INIT(mod_helper.list), \
> + (owner), \
> + (&(helper_func##__id_set)), \
> + (&(func_proto)) \
> +}
This macro needs to be outside the ifdef, otherwise when
CONFIG_DEBUG_INFO_BTF_MODULES is not set, code will fail to compile. Also, I
would use static and const on the variable, so that compiler can optimize it out
when the config option is disabled.
> +#else
> +int register_mod_helper(struct btf_mod_helper *mod_helper)
> +{
> + return -EPERM;
> +}
> +int unregister_mod_helper(struct btf_mod_helper *mod_helper)
> +{
> + return -EPERM;
> +}
> +const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
> + const u32 kfunc_btf_id)
> +{
> + return NULL;
> +}
In the else block, these need to be static inline functions, otherwise you'll
get a warning and link time error because the same symbol will be present in
multiple objects.
> +#endif
> +
> #endif
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 57f5fd5af2f9..f9aa6ba85f3f 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -228,6 +228,7 @@ struct btf {
> u32 id;
> struct rcu_head rcu;
> struct btf_kfunc_set_tab *kfunc_set_tab;
> + struct btf_mod_helper_list *mod_helper_list;
What will free this struct when btf goes away? I don't see any cleanup code in
btf_free.
>
> /* split BTF support */
> struct btf *base_btf;
> @@ -6752,6 +6753,93 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
> }
> EXPORT_SYMBOL_GPL(register_btf_kfunc_id_set);
>
> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> +int register_mod_helper(struct btf_mod_helper *mod_helper)
> +{
> + struct btf_mod_helper *s;
> + struct btf *btf;
> + struct btf_mod_helper_list *mod_helper_list;
> +
> + btf = btf_get_module_btf(mod_helper->owner);
> + if (!btf_is_module(btf)) {
You need to check for IS_ERR_OR_NULL before calling btf_is_module.
Also, this would fail if the module is compiled as built-in, because
mod_helper->owner will be NULL, and btf_get_module_btf will return btf_vmlinux.
> + pr_err("%s can only be called from kernel module", __func__);
> + return -EINVAL;
> + }
> +
> + if (IS_ERR_OR_NULL(btf))
> + return btf ? PTR_ERR(btf) : -ENOENT;
> +
> + mod_helper_list = btf->mod_helper_list;
> + if (!mod_helper_list) {
> + mod_helper_list = kzalloc(sizeof(*mod_helper_list), GFP_KERNEL | __GFP_NOWARN);
> + if (!mod_helper_list)
> + return -ENOMEM;
Here, you are not doing btf_put for module btf. Also, you'll have to guard the
btf_put with `if (btf_is_module(btf))` because reference is not raised for
btf_vmlinux.
> + INIT_LIST_HEAD(&mod_helper_list->list);
> + mutex_init(&mod_helper_list->mutex);
> + btf->mod_helper_list = mod_helper_list;
> + }
> +
> + // Check if btf id is already registered
No C++ style comments.
> + mutex_lock(&mod_helper_list->mutex);
> + list_for_each_entry(s, &mod_helper_list->list, list) {
> + if (mod_helper->set->ids[0] == s->set->ids[0]) {
> + pr_warn("Dynamic helper %u is already registered\n", s->set->ids[0]);
> + mutex_unlock(&mod_helper_list->mutex);
> + return -EINVAL;
Need to free mod_helper_list and conditionally btf_put before returning.
> + }
> + }
> + list_add(&mod_helper->list, &mod_helper_list->list);
> + mutex_unlock(&mod_helper_list->mutex);
> + btf_put(btf);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_mod_helper);
> +
> +int unregister_mod_helper(struct btf_mod_helper *mod_helper)
> +{
> + struct btf *btf;
> + struct btf_mod_helper_list *mod_helper_list;
> +
> + btf = btf_get_module_btf(mod_helper->owner);
> + if (!btf_is_module(btf)) {
Same error as above, need the IS_ERR_OR_NULL check to be before this.
> + pr_err("%s can only be called from kernel module", __func__);
> + return -EINVAL;
> + }
> +
> + if (IS_ERR_OR_NULL(btf))
> + return btf ? PTR_ERR(btf) : -ENOENT;
> +
> + mod_helper_list = btf->mod_helper_list;
> + mutex_lock(&mod_helper_list->mutex);
> + list_del(&mod_helper->list);
> + mutex_unlock(&mod_helper_list->mutex);
> + btf_put(btf);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(unregister_mod_helper);
> +const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf, const u32 kfunc_btf_id)
> +{
> + struct btf_mod_helper *s;
> + struct btf_mod_helper_list *mod_helper_list;
> +
> + mod_helper_list = btf->mod_helper_list;
> + if (!mod_helper_list)
> + return NULL;
> +
> + mutex_lock(&mod_helper_list->mutex);
> + list_for_each_entry(s, &mod_helper_list->list, list) {
> + if (s->set->ids[0] == kfunc_btf_id) {
If there is only one BTF ID, I think you can just use BTF_ID_LIST instead.
> + mutex_unlock(&mod_helper_list->mutex);
> + return s->func_proto;
> + }
> + }
> + mutex_unlock(&mod_helper_list->mutex);
> + return NULL;
> +}
> +#endif /* CONFIG_DEBUG_INFO_BTF_MODULES */
> +
> int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
> const struct btf *targ_btf, __u32 targ_id)
> {
> --
> 2.25.1
>
