Re: [RFC bpf-next 1/3] bpf: btf: Introduce infrastructure for module helpers

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

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@bytedance.com>
> ---
>  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
>