[RFC bpf-next 0/3] bpf: Introduce module helper functions

[RFC bpf-next 0/3] bpf: Introduce module helper functions

[Usama Arif]

This patchset is a working prototype that adds support for calling helper
functions in eBPF applications that have been defined in a kernel module.

It would require further work including code refractoring (not included in
the patchset) to rename functions, data structures and variables that are
used for kfunc as well to be appropriately renamed for module helper usage.
If the idea of module helper functions and the approach used in this patchset
is acceptable to the bpf community, I can send a follow up patchset with the
code refractoring included to make it ready for review.

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 helper functions needing to be
in-kernel 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.

This patchset also includes a very simple example selftest showing the
working of module helpers.

Usama Arif (3):
  bpf: btf: Introduce infrastructure for module helpers
  bpf: add support for module helpers in verifier
  selftests/bpf: add test for module helper

 include/linux/btf.h                           | 44 ++++++++++
 kernel/bpf/btf.c                              | 88 +++++++++++++++++++
 kernel/bpf/verifier.c                         | 50 ++++++++---
 tools/testing/selftests/bpf/Makefile          |  3 +-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 21 +++++
 .../selftests/bpf/prog_tests/helper_module.c  | 59 +++++++++++++
 .../selftests/bpf/progs/test_helper_module.c  | 18 ++++
 7 files changed, 271 insertions(+), 12 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/helper_module.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_helper_module.c

-- 
2.25.1

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

[Usama Arif]

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)) \
+}
+#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;
+}
+#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;
 
 	/* 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)) {
+		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;
+		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
+	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;
+		}
+	}
+	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)) {
+		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) {
+			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

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

[kernel test robot]

Hi Usama,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a004-20220117 (https://download.01.org/0day-ci/archive/20220122/202201220628.8IcADV9U-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ca60b90025819a8a03818e86e2105bd15576d134
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
        git checkout ca60b90025819a8a03818e86e2105bd15576d134
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/bpf/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from net/bpf/test_run.c:5:
>> include/linux/btf.h:391:5: warning: no previous prototype for function 'register_mod_helper' [-Wmissing-prototypes]
   int register_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:391:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int register_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:395:5: warning: no previous prototype for function 'unregister_mod_helper' [-Wmissing-prototypes]
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:395:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:399:30: warning: no previous prototype for function 'get_mod_helper_proto' [-Wmissing-prototypes]
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
                                ^
   include/linux/btf.h:399:7: note: declare 'static' if the function is not intended to be used outside of this translation unit
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
         ^
   static 
   net/bpf/test_run.c:171:14: warning: no previous prototype for function 'bpf_fentry_test1' [-Wmissing-prototypes]
   int noinline bpf_fentry_test1(int a)
                ^
   net/bpf/test_run.c:171:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test1(int a)
   ^
   static 
   net/bpf/test_run.c:178:14: warning: no previous prototype for function 'bpf_fentry_test2' [-Wmissing-prototypes]
   int noinline bpf_fentry_test2(int a, u64 b)
                ^
   net/bpf/test_run.c:178:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test2(int a, u64 b)
   ^
   static 
   net/bpf/test_run.c:183:14: warning: no previous prototype for function 'bpf_fentry_test3' [-Wmissing-prototypes]
   int noinline bpf_fentry_test3(char a, int b, u64 c)
                ^
   net/bpf/test_run.c:183:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test3(char a, int b, u64 c)
   ^
   static 
   net/bpf/test_run.c:188:14: warning: no previous prototype for function 'bpf_fentry_test4' [-Wmissing-prototypes]
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
                ^
   net/bpf/test_run.c:188:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
   ^
   static 
   net/bpf/test_run.c:193:14: warning: no previous prototype for function 'bpf_fentry_test5' [-Wmissing-prototypes]
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
                ^
   net/bpf/test_run.c:193:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
   ^
   static 
   net/bpf/test_run.c:198:14: warning: no previous prototype for function 'bpf_fentry_test6' [-Wmissing-prototypes]
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
                ^
   net/bpf/test_run.c:198:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
   ^
   static 
   net/bpf/test_run.c:207:14: warning: no previous prototype for function 'bpf_fentry_test7' [-Wmissing-prototypes]
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:207:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:212:14: warning: no previous prototype for function 'bpf_fentry_test8' [-Wmissing-prototypes]
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:212:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:217:14: warning: no previous prototype for function 'bpf_modify_return_test' [-Wmissing-prototypes]
   int noinline bpf_modify_return_test(int a, int *b)
                ^
   net/bpf/test_run.c:217:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_modify_return_test(int a, int *b)
   ^
   static 
   net/bpf/test_run.c:223:14: warning: no previous prototype for function 'bpf_kfunc_call_test1' [-Wmissing-prototypes]
   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
                ^
   net/bpf/test_run.c:223:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
   ^
   static 
   net/bpf/test_run.c:228:14: warning: no previous prototype for function 'bpf_kfunc_call_test2' [-Wmissing-prototypes]
   int noinline bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
                ^
   net/bpf/test_run.c:228:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
   ^
   static 
   net/bpf/test_run.c:233:24: warning: no previous prototype for function 'bpf_kfunc_call_test3' [-Wmissing-prototypes]
   struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
                          ^
   net/bpf/test_run.c:233:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
   ^
   static 
   net/bpf/test_run.c:251:1: warning: no previous prototype for function 'bpf_kfunc_call_test_acquire' [-Wmissing-prototypes]
   bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
   ^
   net/bpf/test_run.c:250:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline struct prog_test_ref_kfunc *
            ^
            static 
   net/bpf/test_run.c:259:15: warning: no previous prototype for function 'bpf_kfunc_call_test_release' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
                 ^


vim +/register_mod_helper +391 include/linux/btf.h

   373	
   374	#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
   375	int register_mod_helper(struct btf_mod_helper *mod_helper);
   376	int unregister_mod_helper(struct btf_mod_helper *mod_helper);
   377	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   378		const u32 kfunc_btf_id);
   379	
   380	#define DEFINE_MOD_HELPER(mod_helper, owner, helper_func, func_proto) \
   381	BTF_SET_START(helper_func##__id_set) \
   382	BTF_ID(func, helper_func) \
   383	BTF_SET_END(helper_func##__id_set) \
   384	struct btf_mod_helper mod_helper = { \
   385		LIST_HEAD_INIT(mod_helper.list), \
   386		(owner), \
   387		(&(helper_func##__id_set)), \
   388		(&(func_proto)) \
   389	}
   390	#else
 > 391	int register_mod_helper(struct btf_mod_helper *mod_helper)
   392	{
   393		return -EPERM;
   394	}
 > 395	int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   396	{
   397		return -EPERM;
   398	}
 > 399	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   400		const u32 kfunc_btf_id)
   401	{
   402		return NULL;
   403	}
   404	#endif
   405	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 9640 bytes --]

Hi Usama,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: x86_64-randconfig-a004-20220117 (https://download.01.org/0day-ci/archive/20220122/202201220628.8IcADV9U-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ca60b90025819a8a03818e86e2105bd15576d134
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
        git checkout ca60b90025819a8a03818e86e2105bd15576d134
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash net/bpf/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from net/bpf/test_run.c:5:
>> include/linux/btf.h:391:5: warning: no previous prototype for function 'register_mod_helper' [-Wmissing-prototypes]
   int register_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:391:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int register_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:395:5: warning: no previous prototype for function 'unregister_mod_helper' [-Wmissing-prototypes]
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:395:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:399:30: warning: no previous prototype for function 'get_mod_helper_proto' [-Wmissing-prototypes]
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
                                ^
   include/linux/btf.h:399:7: note: declare 'static' if the function is not intended to be used outside of this translation unit
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
         ^
   static 
   net/bpf/test_run.c:171:14: warning: no previous prototype for function 'bpf_fentry_test1' [-Wmissing-prototypes]
   int noinline bpf_fentry_test1(int a)
                ^
   net/bpf/test_run.c:171:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test1(int a)
   ^
   static 
   net/bpf/test_run.c:178:14: warning: no previous prototype for function 'bpf_fentry_test2' [-Wmissing-prototypes]
   int noinline bpf_fentry_test2(int a, u64 b)
                ^
   net/bpf/test_run.c:178:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test2(int a, u64 b)
   ^
   static 
   net/bpf/test_run.c:183:14: warning: no previous prototype for function 'bpf_fentry_test3' [-Wmissing-prototypes]
   int noinline bpf_fentry_test3(char a, int b, u64 c)
                ^
   net/bpf/test_run.c:183:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test3(char a, int b, u64 c)
   ^
   static 
   net/bpf/test_run.c:188:14: warning: no previous prototype for function 'bpf_fentry_test4' [-Wmissing-prototypes]
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
                ^
   net/bpf/test_run.c:188:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test4(void *a, char b, int c, u64 d)
   ^
   static 
   net/bpf/test_run.c:193:14: warning: no previous prototype for function 'bpf_fentry_test5' [-Wmissing-prototypes]
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
                ^
   net/bpf/test_run.c:193:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test5(u64 a, void *b, short c, int d, u64 e)
   ^
   static 
   net/bpf/test_run.c:198:14: warning: no previous prototype for function 'bpf_fentry_test6' [-Wmissing-prototypes]
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
                ^
   net/bpf/test_run.c:198:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test6(u64 a, void *b, short c, int d, void *e, u64 f)
   ^
   static 
   net/bpf/test_run.c:207:14: warning: no previous prototype for function 'bpf_fentry_test7' [-Wmissing-prototypes]
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:207:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test7(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:212:14: warning: no previous prototype for function 'bpf_fentry_test8' [-Wmissing-prototypes]
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
                ^
   net/bpf/test_run.c:212:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_fentry_test8(struct bpf_fentry_test_t *arg)
   ^
   static 
   net/bpf/test_run.c:217:14: warning: no previous prototype for function 'bpf_modify_return_test' [-Wmissing-prototypes]
   int noinline bpf_modify_return_test(int a, int *b)
                ^
   net/bpf/test_run.c:217:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_modify_return_test(int a, int *b)
   ^
   static 
   net/bpf/test_run.c:223:14: warning: no previous prototype for function 'bpf_kfunc_call_test1' [-Wmissing-prototypes]
   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
                ^
   net/bpf/test_run.c:223:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   u64 noinline bpf_kfunc_call_test1(struct sock *sk, u32 a, u64 b, u32 c, u64 d)
   ^
   static 
   net/bpf/test_run.c:228:14: warning: no previous prototype for function 'bpf_kfunc_call_test2' [-Wmissing-prototypes]
   int noinline bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
                ^
   net/bpf/test_run.c:228:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int noinline bpf_kfunc_call_test2(struct sock *sk, u32 a, u32 b)
   ^
   static 
   net/bpf/test_run.c:233:24: warning: no previous prototype for function 'bpf_kfunc_call_test3' [-Wmissing-prototypes]
   struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
                          ^
   net/bpf/test_run.c:233:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   struct sock * noinline bpf_kfunc_call_test3(struct sock *sk)
   ^
   static 
   net/bpf/test_run.c:251:1: warning: no previous prototype for function 'bpf_kfunc_call_test_acquire' [-Wmissing-prototypes]
   bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
   ^
   net/bpf/test_run.c:250:10: note: declare 'static' if the function is not intended to be used outside of this translation unit
   noinline struct prog_test_ref_kfunc *
            ^
            static 
   net/bpf/test_run.c:259:15: warning: no previous prototype for function 'bpf_kfunc_call_test_release' [-Wmissing-prototypes]
   noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
                 ^


vim +/register_mod_helper +391 include/linux/btf.h

   373	
   374	#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
   375	int register_mod_helper(struct btf_mod_helper *mod_helper);
   376	int unregister_mod_helper(struct btf_mod_helper *mod_helper);
   377	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   378		const u32 kfunc_btf_id);
   379	
   380	#define DEFINE_MOD_HELPER(mod_helper, owner, helper_func, func_proto) \
   381	BTF_SET_START(helper_func##__id_set) \
   382	BTF_ID(func, helper_func) \
   383	BTF_SET_END(helper_func##__id_set) \
   384	struct btf_mod_helper mod_helper = { \
   385		LIST_HEAD_INIT(mod_helper.list), \
   386		(owner), \
   387		(&(helper_func##__id_set)), \
   388		(&(func_proto)) \
   389	}
   390	#else
 > 391	int register_mod_helper(struct btf_mod_helper *mod_helper)
   392	{
   393		return -EPERM;
   394	}
 > 395	int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   396	{
   397		return -EPERM;
   398	}
 > 399	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   400		const u32 kfunc_btf_id)
   401	{
   402		return NULL;
   403	}
   404	#endif
   405	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

[kernel test robot]

Hi Usama,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: arm-spear3xx_defconfig (https://download.01.org/0day-ci/archive/20220122/202201220614.duEAa9pD-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/ca60b90025819a8a03818e86e2105bd15576d134
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
        git checkout ca60b90025819a8a03818e86e2105bd15576d134
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash net/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from net/core/filter.c:50:
>> include/linux/btf.h:391:5: warning: no previous prototype for function 'register_mod_helper' [-Wmissing-prototypes]
   int register_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:391:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int register_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:395:5: warning: no previous prototype for function 'unregister_mod_helper' [-Wmissing-prototypes]
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:395:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:399:30: warning: no previous prototype for function 'get_mod_helper_proto' [-Wmissing-prototypes]
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
                                ^
   include/linux/btf.h:399:7: note: declare 'static' if the function is not intended to be used outside of this translation unit
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
         ^
   static 
   3 warnings generated.


vim +/register_mod_helper +391 include/linux/btf.h

   373	
   374	#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
   375	int register_mod_helper(struct btf_mod_helper *mod_helper);
   376	int unregister_mod_helper(struct btf_mod_helper *mod_helper);
   377	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   378		const u32 kfunc_btf_id);
   379	
   380	#define DEFINE_MOD_HELPER(mod_helper, owner, helper_func, func_proto) \
   381	BTF_SET_START(helper_func##__id_set) \
   382	BTF_ID(func, helper_func) \
   383	BTF_SET_END(helper_func##__id_set) \
   384	struct btf_mod_helper mod_helper = { \
   385		LIST_HEAD_INIT(mod_helper.list), \
   386		(owner), \
   387		(&(helper_func##__id_set)), \
   388		(&(func_proto)) \
   389	}
   390	#else
 > 391	int register_mod_helper(struct btf_mod_helper *mod_helper)
   392	{
   393		return -EPERM;
   394	}
 > 395	int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   396	{
   397		return -EPERM;
   398	}
 > 399	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   400		const u32 kfunc_btf_id)
   401	{
   402		return NULL;
   403	}
   404	#endif
   405	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4186 bytes --]

Hi Usama,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on bpf-next/master]

url:    https://github.com/0day-ci/linux/commits/Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: arm-spear3xx_defconfig (https://download.01.org/0day-ci/archive/20220122/202201220614.duEAa9pD-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 7b3d30728816403d1fd73cc5082e9fb761262bce)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/ca60b90025819a8a03818e86e2105bd15576d134
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Usama-Arif/bpf-Introduce-module-helper-functions/20220122-034203
        git checkout ca60b90025819a8a03818e86e2105bd15576d134
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash net/core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from net/core/filter.c:50:
>> include/linux/btf.h:391:5: warning: no previous prototype for function 'register_mod_helper' [-Wmissing-prototypes]
   int register_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:391:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int register_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:395:5: warning: no previous prototype for function 'unregister_mod_helper' [-Wmissing-prototypes]
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
       ^
   include/linux/btf.h:395:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   ^
   static 
>> include/linux/btf.h:399:30: warning: no previous prototype for function 'get_mod_helper_proto' [-Wmissing-prototypes]
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
                                ^
   include/linux/btf.h:399:7: note: declare 'static' if the function is not intended to be used outside of this translation unit
   const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
         ^
   static 
   3 warnings generated.


vim +/register_mod_helper +391 include/linux/btf.h

   373	
   374	#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
   375	int register_mod_helper(struct btf_mod_helper *mod_helper);
   376	int unregister_mod_helper(struct btf_mod_helper *mod_helper);
   377	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   378		const u32 kfunc_btf_id);
   379	
   380	#define DEFINE_MOD_HELPER(mod_helper, owner, helper_func, func_proto) \
   381	BTF_SET_START(helper_func##__id_set) \
   382	BTF_ID(func, helper_func) \
   383	BTF_SET_END(helper_func##__id_set) \
   384	struct btf_mod_helper mod_helper = { \
   385		LIST_HEAD_INIT(mod_helper.list), \
   386		(owner), \
   387		(&(helper_func##__id_set)), \
   388		(&(func_proto)) \
   389	}
   390	#else
 > 391	int register_mod_helper(struct btf_mod_helper *mod_helper)
   392	{
   393		return -EPERM;
   394	}
 > 395	int unregister_mod_helper(struct btf_mod_helper *mod_helper)
   396	{
   397		return -EPERM;
   398	}
 > 399	const struct bpf_func_proto *get_mod_helper_proto(const struct btf *btf,
   400		const u32 kfunc_btf_id)
   401	{
   402		return NULL;
   403	}
   404	#endif
   405	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

[Kumar Kartikeya Dwivedi]

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
>

[RFC bpf-next 2/3] bpf: add support for module helpers in verifier

[Usama Arif]

After the kernel module registers the helper, its BTF id
and func_proto are available during verification. During
verification, it is checked to see if insn->imm is available
in the list of module helper btf ids. If it is,
check_helper_call is called, otherwise check_kfunc_call.
The module helper function proto is obtained in check_helper_call
via get_mod_helper_proto function.

Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 kernel/bpf/verifier.c | 50 +++++++++++++++++++++++++++++++++----------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 8c5a46d41f28..bf7605664b95 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6532,19 +6532,39 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 	int insn_idx = *insn_idx_p;
 	bool changes_data;
 	int i, err, func_id;
+	const struct btf_type *func;
+	const char *func_name;
+	struct btf *desc_btf;
 
 	/* find function prototype */
 	func_id = insn->imm;
-	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
-		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
-			func_id);
-		return -EINVAL;
-	}
 
 	if (env->ops->get_func_proto)
 		fn = env->ops->get_func_proto(func_id, env->prog);
-	if (!fn) {
-		verbose(env, "unknown func %s#%d\n", func_id_name(func_id),
+
+	if (func_id >= __BPF_FUNC_MAX_ID) {
+		desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
+		if (IS_ERR(desc_btf))
+			return PTR_ERR(desc_btf);
+
+		fn = get_mod_helper_proto(desc_btf, func_id);
+		if (!fn) {
+			func = btf_type_by_id(desc_btf, func_id);
+			func_name = btf_name_by_offset(desc_btf, func->name_off);
+			verbose(env, "unknown module helper func %s#%d\n", func_name,
+				func_id);
+			return -EACCES;
+		}
+	} else if (func_id >= 0) {
+		if (env->ops->get_func_proto)
+			fn = env->ops->get_func_proto(func_id, env->prog);
+		if (!fn) {
+			verbose(env, "unknown in-kernel helper func %s#%d\n", func_id_name(func_id),
+				func_id);
+			return -EINVAL;
+		}
+	} else {
+		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
 			func_id);
 		return -EINVAL;
 	}
@@ -11351,6 +11371,7 @@ static int do_check(struct bpf_verifier_env *env)
 	int insn_cnt = env->prog->len;
 	bool do_print_state = false;
 	int prev_insn_idx = -1;
+	struct btf *desc_btf;
 
 	for (;;) {
 		struct bpf_insn *insn;
@@ -11579,10 +11600,17 @@ static int do_check(struct bpf_verifier_env *env)
 				}
 				if (insn->src_reg == BPF_PSEUDO_CALL)
 					err = check_func_call(env, insn, &env->insn_idx);
-				else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
-					err = check_kfunc_call(env, insn, &env->insn_idx);
-				else
-					err = check_helper_call(env, insn, &env->insn_idx);
+				else {
+					desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
+					if (IS_ERR(desc_btf))
+						return PTR_ERR(desc_btf);
+
+					if (insn->src_reg == BPF_K ||
+					   get_mod_helper_proto(desc_btf, insn->imm))
+						err = check_helper_call(env, insn, &env->insn_idx);
+					else
+						err = check_kfunc_call(env, insn, &env->insn_idx);
+				}
 				if (err)
 					return err;
 			} else if (opcode == BPF_JA) {
-- 
2.25.1

Re: [RFC bpf-next 2/3] bpf: add support for module helpers in verifier

[Kumar Kartikeya Dwivedi]

On Sat, Jan 22, 2022 at 01:09:55AM IST, Usama Arif wrote:
> After the kernel module registers the helper, its BTF id
> and func_proto are available during verification. During
> verification, it is checked to see if insn->imm is available
> in the list of module helper btf ids. If it is,
> check_helper_call is called, otherwise check_kfunc_call.
> The module helper function proto is obtained in check_helper_call
> via get_mod_helper_proto function.
>
> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
> ---
>  kernel/bpf/verifier.c | 50 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8c5a46d41f28..bf7605664b95 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6532,19 +6532,39 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	int insn_idx = *insn_idx_p;
>  	bool changes_data;
>  	int i, err, func_id;
> +	const struct btf_type *func;
> +	const char *func_name;
> +	struct btf *desc_btf;
>
>  	/* find function prototype */
>  	func_id = insn->imm;
> -	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
> -		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
> -			func_id);
> -		return -EINVAL;
> -	}
>
>  	if (env->ops->get_func_proto)
>  		fn = env->ops->get_func_proto(func_id, env->prog);
> -	if (!fn) {
> -		verbose(env, "unknown func %s#%d\n", func_id_name(func_id),
> +
> +	if (func_id >= __BPF_FUNC_MAX_ID) {
> +		desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);

I am not sure this is right, even if we reached this point. add_kfunc_call would
not be called for a helper call, which means the kfunc_btf_tab will not be
populated. I think this code is not reachable from your test, which is why you
didn't see this. More below.

> +		if (IS_ERR(desc_btf))
> +			return PTR_ERR(desc_btf);
> +
> +		fn = get_mod_helper_proto(desc_btf, func_id);
> +		if (!fn) {
> +			func = btf_type_by_id(desc_btf, func_id);
> +			func_name = btf_name_by_offset(desc_btf, func->name_off);
> +			verbose(env, "unknown module helper func %s#%d\n", func_name,
> +				func_id);
> +			return -EACCES;
> +		}
> +	} else if (func_id >= 0) {
> +		if (env->ops->get_func_proto)
> +			fn = env->ops->get_func_proto(func_id, env->prog);
> +		if (!fn) {
> +			verbose(env, "unknown in-kernel helper func %s#%d\n", func_id_name(func_id),
> +				func_id);
> +			return -EINVAL;
> +		}
> +	} else {
> +		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
>  			func_id);
>  		return -EINVAL;
>  	}
> @@ -11351,6 +11371,7 @@ static int do_check(struct bpf_verifier_env *env)
>  	int insn_cnt = env->prog->len;
>  	bool do_print_state = false;
>  	int prev_insn_idx = -1;
> +	struct btf *desc_btf;
>
>  	for (;;) {
>  		struct bpf_insn *insn;
> @@ -11579,10 +11600,17 @@ static int do_check(struct bpf_verifier_env *env)
>  				}
>  				if (insn->src_reg == BPF_PSEUDO_CALL)
>  					err = check_func_call(env, insn, &env->insn_idx);
> -				else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
> -					err = check_kfunc_call(env, insn, &env->insn_idx);
> -				else
> -					err = check_helper_call(env, insn, &env->insn_idx);
> +				else {
> +					desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
> +					if (IS_ERR(desc_btf))
> +						return PTR_ERR(desc_btf);
> +

I didn't get this part at all.

At this point src_reg can be BPF_PSEUDO_KFUNC_CALL, or 0 (for helper call). If
it is a helper call, then find_kfunc_desc_btf using insn->imm and insn->off
would be a bug.

> +					if (insn->src_reg == BPF_K ||

Why are you comparing it to BPF_K? I think your patch is not going through your
logic in check_helper_call at all.

In your selftest, you declare it using __ksym. This means src_reg will be
encoded as BPF_PSEUDO_KFUNC_CALL (2), this if condition will never be hit
(because BPF_K is 0), and you will do check_kfunc_call for it.

TLDR; I think it is being checked as a normal kfunc call by the verifier.

What am I missing?

> +					   get_mod_helper_proto(desc_btf, insn->imm))
> +						err = check_helper_call(env, insn, &env->insn_idx);
> +					else
> +						err = check_kfunc_call(env, insn, &env->insn_idx);
> +				}
>  				if (err)
>  					return err;
>  			} else if (opcode == BPF_JA) {
> --
> 2.25.1
>

Re: [RFC bpf-next 2/3] bpf: add support for module helpers in verifier

[Kumar Kartikeya Dwivedi]

On Sat, Jan 22, 2022 at 09:01:33AM IST, Kumar Kartikeya Dwivedi wrote:
> On Sat, Jan 22, 2022 at 01:09:55AM IST, Usama Arif wrote:
> > After the kernel module registers the helper, its BTF id
> > and func_proto are available during verification. During
> > verification, it is checked to see if insn->imm is available
> > in the list of module helper btf ids. If it is,
> > check_helper_call is called, otherwise check_kfunc_call.
> > The module helper function proto is obtained in check_helper_call
> > via get_mod_helper_proto function.
> >
> > Signed-off-by: Usama Arif <usama.arif@bytedance.com>
> > ---
> >  kernel/bpf/verifier.c | 50 +++++++++++++++++++++++++++++++++----------
> >  1 file changed, 39 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 8c5a46d41f28..bf7605664b95 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -6532,19 +6532,39 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >  	int insn_idx = *insn_idx_p;
> >  	bool changes_data;
> >  	int i, err, func_id;
> > +	const struct btf_type *func;
> > +	const char *func_name;
> > +	struct btf *desc_btf;
> >
> >  	/* find function prototype */
> >  	func_id = insn->imm;
> > -	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
> > -		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
> > -			func_id);
> > -		return -EINVAL;
> > -	}
> >
> >  	if (env->ops->get_func_proto)
> >  		fn = env->ops->get_func_proto(func_id, env->prog);
> > -	if (!fn) {
> > -		verbose(env, "unknown func %s#%d\n", func_id_name(func_id),
> > +
> > +	if (func_id >= __BPF_FUNC_MAX_ID) {
> > +		desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
>
> I am not sure this is right, even if we reached this point. add_kfunc_call would
> not be called for a helper call, which means the kfunc_btf_tab will not be
> populated. I think this code is not reachable from your test, which is why you
> didn't see this. More below.
>
> > +		if (IS_ERR(desc_btf))
> > +			return PTR_ERR(desc_btf);
> > +
> > +		fn = get_mod_helper_proto(desc_btf, func_id);
> > +		if (!fn) {
> > +			func = btf_type_by_id(desc_btf, func_id);
> > +			func_name = btf_name_by_offset(desc_btf, func->name_off);
> > +			verbose(env, "unknown module helper func %s#%d\n", func_name,
> > +				func_id);
> > +			return -EACCES;
> > +		}
> > +	} else if (func_id >= 0) {
> > +		if (env->ops->get_func_proto)
> > +			fn = env->ops->get_func_proto(func_id, env->prog);
> > +		if (!fn) {
> > +			verbose(env, "unknown in-kernel helper func %s#%d\n", func_id_name(func_id),
> > +				func_id);
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
> >  			func_id);
> >  		return -EINVAL;
> >  	}
> > @@ -11351,6 +11371,7 @@ static int do_check(struct bpf_verifier_env *env)
> >  	int insn_cnt = env->prog->len;
> >  	bool do_print_state = false;
> >  	int prev_insn_idx = -1;
> > +	struct btf *desc_btf;
> >
> >  	for (;;) {
> >  		struct bpf_insn *insn;
> > @@ -11579,10 +11600,17 @@ static int do_check(struct bpf_verifier_env *env)
> >  				}
> >  				if (insn->src_reg == BPF_PSEUDO_CALL)
> >  					err = check_func_call(env, insn, &env->insn_idx);
> > -				else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
> > -					err = check_kfunc_call(env, insn, &env->insn_idx);
> > -				else
> > -					err = check_helper_call(env, insn, &env->insn_idx);
> > +				else {
> > +					desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
> > +					if (IS_ERR(desc_btf))
> > +						return PTR_ERR(desc_btf);
> > +
>
> I didn't get this part at all.
>
> At this point src_reg can be BPF_PSEUDO_KFUNC_CALL, or 0 (for helper call). If
> it is a helper call, then find_kfunc_desc_btf using insn->imm and insn->off
> would be a bug.
>
> > +					if (insn->src_reg == BPF_K ||
>
> [...]
>

Ah, I think I see what you are doing: BPF_K is zero, so either when it is a
helper call or it is a module helper (which will be a kfunc), you call
check_helper_call. get_mod_helper_proto would return true in that case.

But if it is an in-kernel helper, calling find_kfunc_desc_btf would still be a
bug, since imm encodes func_id.

It's also a bit confusing that check_helper_call is called for a kfunc.

> > +					   get_mod_helper_proto(desc_btf, insn->imm))
> > +						err = check_helper_call(env, insn, &env->insn_idx);
> > +					else
> > +						err = check_kfunc_call(env, insn, &env->insn_idx);
> > +				}
> >  				if (err)
> >  					return err;
> >  			} else if (opcode == BPF_JA) {
> > --
> > 2.25.1
> >

Re: [RFC bpf-next 2/3] bpf: add support for module helpers in verifier

[Usama Arif]

On 22/01/2022 03:56, Kumar Kartikeya Dwivedi wrote:
> On Sat, Jan 22, 2022 at 09:01:33AM IST, Kumar Kartikeya Dwivedi wrote:
>> On Sat, Jan 22, 2022 at 01:09:55AM IST, Usama Arif wrote:
>>> After the kernel module registers the helper, its BTF id
>>> and func_proto are available during verification. During
>>> verification, it is checked to see if insn->imm is available
>>> in the list of module helper btf ids. If it is,
>>> check_helper_call is called, otherwise check_kfunc_call.
>>> The module helper function proto is obtained in check_helper_call
>>> via get_mod_helper_proto function.
>>>
>>> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
>>> ---
>>>   kernel/bpf/verifier.c | 50 +++++++++++++++++++++++++++++++++----------
>>>   1 file changed, 39 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 8c5a46d41f28..bf7605664b95 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -6532,19 +6532,39 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>>>   	int insn_idx = *insn_idx_p;
>>>   	bool changes_data;
>>>   	int i, err, func_id;
>>> +	const struct btf_type *func;
>>> +	const char *func_name;
>>> +	struct btf *desc_btf;
>>>
>>>   	/* find function prototype */
>>>   	func_id = insn->imm;
>>> -	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
>>> -		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
>>> -			func_id);
>>> -		return -EINVAL;
>>> -	}
>>>
>>>   	if (env->ops->get_func_proto)
>>>   		fn = env->ops->get_func_proto(func_id, env->prog);
>>> -	if (!fn) {
>>> -		verbose(env, "unknown func %s#%d\n", func_id_name(func_id),
>>> +
>>> +	if (func_id >= __BPF_FUNC_MAX_ID) {
>>> +		desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
>>
>> I am not sure this is right, even if we reached this point. add_kfunc_call would
>> not be called for a helper call, which means the kfunc_btf_tab will not be
>> populated. I think this code is not reachable from your test, which is why you
>> didn't see this. More below.
>>
>>> +		if (IS_ERR(desc_btf))
>>> +			return PTR_ERR(desc_btf);
>>> +
>>> +		fn = get_mod_helper_proto(desc_btf, func_id);
>>> +		if (!fn) {
>>> +			func = btf_type_by_id(desc_btf, func_id);
>>> +			func_name = btf_name_by_offset(desc_btf, func->name_off);
>>> +			verbose(env, "unknown module helper func %s#%d\n", func_name,
>>> +				func_id);
>>> +			return -EACCES;
>>> +		}
>>> +	} else if (func_id >= 0) {
>>> +		if (env->ops->get_func_proto)
>>> +			fn = env->ops->get_func_proto(func_id, env->prog);
>>> +		if (!fn) {
>>> +			verbose(env, "unknown in-kernel helper func %s#%d\n", func_id_name(func_id),
>>> +				func_id);
>>> +			return -EINVAL;
>>> +		}
>>> +	} else {
>>> +		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
>>>   			func_id);
>>>   		return -EINVAL;
>>>   	}
>>> @@ -11351,6 +11371,7 @@ static int do_check(struct bpf_verifier_env *env)
>>>   	int insn_cnt = env->prog->len;
>>>   	bool do_print_state = false;
>>>   	int prev_insn_idx = -1;
>>> +	struct btf *desc_btf;
>>>
>>>   	for (;;) {
>>>   		struct bpf_insn *insn;
>>> @@ -11579,10 +11600,17 @@ static int do_check(struct bpf_verifier_env *env)
>>>   				}
>>>   				if (insn->src_reg == BPF_PSEUDO_CALL)
>>>   					err = check_func_call(env, insn, &env->insn_idx);
>>> -				else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
>>> -					err = check_kfunc_call(env, insn, &env->insn_idx);
>>> -				else
>>> -					err = check_helper_call(env, insn, &env->insn_idx);
>>> +				else {
>>> +					desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
>>> +					if (IS_ERR(desc_btf))
>>> +						return PTR_ERR(desc_btf);
>>> +
>>
>> I didn't get this part at all.
>>
>> At this point src_reg can be BPF_PSEUDO_KFUNC_CALL, or 0 (for helper call). If
>> it is a helper call, then find_kfunc_desc_btf using insn->imm and insn->off
>> would be a bug.
>>
>>> +					if (insn->src_reg == BPF_K ||
>>
>> [...]
>>
> 
> Ah, I think I see what you are doing: BPF_K is zero, so either when it is a
> helper call or it is a module helper (which will be a kfunc), you call
> check_helper_call. get_mod_helper_proto would return true in that case.
> 
> But if it is an in-kernel helper, calling find_kfunc_desc_btf would still be a
> bug, since imm encodes func_id.
> 
> It's also a bit confusing that check_helper_call is called for a kfunc.
> 

Hi, Thanks for the reviews!

The patchset does require refractoring in relation to kfunc to make it 
mergable as mentioned in cover letter, but it is a working prototype for 
all situations in-kernel/module helper/kfunc.

There are 3 situations:
- if its an in-kernel helper call, insn->src_reg is BPF_K and 
check_helper_call will be called. get_mod_helper_proto will return NULL 
in this case but that doesnt matter as its an OR statement.
- if its a "module helper call" (essentially a kfunc with 
bpf_func_proto) and it has been registered, get_mod_helper_proto will 
return the function proto and check_helper_call will be called. 
insn->src_reg will be BPF_PSEUDO_KFUNC_CALL (requires refractoring) but 
that wont matter as its an OR statement.
- if its a kfunc call, insn->src_reg is BPF_PSEUDO_KFUNC_CALL and 
get_mod_helper_proto will return NULL, so check_kfunc_call will be called.

So all the cases will be covered. I didnt include the refractor as its 
quite big and just wanted to check if something like this will be 
considered by the bpf community before progressing further.

>>> +					   get_mod_helper_proto(desc_btf, insn->imm))
>>> +						err = check_helper_call(env, insn, &env->insn_idx);
>>> +					else
>>> +						err = check_kfunc_call(env, insn, &env->insn_idx);
>>> +				}
>>>   				if (err)
>>>   					return err;
>>>   			} else if (opcode == BPF_JA) {
>>> --
>>> 2.25.1
>>>

[RFC bpf-next 3/3] selftests/bpf: add test for module helper

[Usama Arif]

This is a simple test for a module hepler that accepts
2 pointers to integer, prints them (using printk which isn't
directly accessible to eBPF applications) and returns their sum.
The test has been adapted from test_ksyms_module.

Signed-off-by: Usama Arif <usama.arif@bytedance.com>
---
 tools/testing/selftests/bpf/Makefile          |  3 +-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 21 +++++++
 .../selftests/bpf/prog_tests/helper_module.c  | 59 +++++++++++++++++++
 .../selftests/bpf/progs/test_helper_module.c  | 18 ++++++
 4 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/helper_module.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_helper_module.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 42ffc24e9e71..34df13cdfb05 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -332,7 +332,8 @@ LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
 	test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
 	map_ptr_kern.c core_kern.c
 # Generate both light skeleton and libbpf skeleton for these
-LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c
+LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c kfunc_call_test_subprog.c \
+	test_helper_module.c
 SKEL_BLACKLIST += $$(LSKELS)
 
 test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index bdbacf5adcd2..38d344e2d12d 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2020 Facebook */
+#include <linux/bpf.h>
 #include <linux/btf.h>
 #include <linux/btf_ids.h>
 #include <linux/error-injection.h>
@@ -120,11 +121,29 @@ static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
 
 extern int bpf_fentry_test1(int a);
 
+int bpf_helper_print_add(int *input1, int *input2)
+{
+	printk(KERN_INFO "input numbers for module helper %d %d\n", *input1, *input2);
+	return *input1 + *input2;
+}
+
+struct bpf_func_proto bpf_helper_print_add_proto = {
+	.gpl_only       = false,
+	.ret_type       = RET_INTEGER,
+	.arg1_type      = ARG_PTR_TO_INT,
+	.arg2_type      = ARG_PTR_TO_INT,
+};
+
+DEFINE_MOD_HELPER(mod_helper, THIS_MODULE, bpf_helper_print_add, bpf_helper_print_add_proto);
+
 static int bpf_testmod_init(void)
 {
 	int ret;
 
 	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_testmod_kfunc_set);
+	if (ret < 0)
+		return ret;
+	ret = register_mod_helper(&mod_helper);
 	if (ret < 0)
 		return ret;
 	if (bpf_fentry_test1(0) < 0)
@@ -134,6 +153,8 @@ static int bpf_testmod_init(void)
 
 static void bpf_testmod_exit(void)
 {
+	unregister_mod_helper(&mod_helper);
+
 	return sysfs_remove_bin_file(kernel_kobj, &bin_attr_bpf_testmod_file);
 }
 
diff --git a/tools/testing/selftests/bpf/prog_tests/helper_module.c b/tools/testing/selftests/bpf/prog_tests/helper_module.c
new file mode 100644
index 000000000000..d8e8600ab3be
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/helper_module.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include <network_helpers.h>
+#include "test_helper_module.lskel.h"
+#include "test_helper_module.skel.h"
+
+void test_helper_module_lskel(void)
+{
+	struct test_helper_module_lskel *skel;
+	int retval;
+	int err;
+
+	if (!env.has_testmod) {
+		test__skip();
+		return;
+	}
+
+	skel = test_helper_module_lskel__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_helper_module_lskel__open_and_load"))
+		return;
+	err = bpf_prog_test_run(skel->progs.load.prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, (__u32 *)&retval, NULL);
+	if (!ASSERT_OK(err, "bpf_prog_test_run"))
+		goto cleanup;
+	ASSERT_EQ(retval, 7, "retval");
+cleanup:
+	test_helper_module_lskel__destroy(skel);
+}
+
+void test_helper_module_libbpf(void)
+{
+	struct test_helper_module *skel;
+	int retval, err;
+
+	if (!env.has_testmod) {
+		test__skip();
+		return;
+	}
+
+	skel = test_helper_module__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_helper_module__open"))
+		return;
+	err = bpf_prog_test_run(bpf_program__fd(skel->progs.load), 1, &pkt_v4,
+				sizeof(pkt_v4), NULL, NULL, (__u32 *)&retval, NULL);
+	if (!ASSERT_OK(err, "bpf_prog_test_run"))
+		goto cleanup;
+	ASSERT_EQ(retval, 7, "retval");
+cleanup:
+	test_helper_module__destroy(skel);
+}
+
+void test_helper_module(void)
+{
+	if (test__start_subtest("lskel"))
+		test_helper_module_lskel();
+	if (test__start_subtest("libbpf"))
+		test_helper_module_libbpf();
+}
diff --git a/tools/testing/selftests/bpf/progs/test_helper_module.c b/tools/testing/selftests/bpf/progs/test_helper_module.c
new file mode 100644
index 000000000000..66dadd317498
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_helper_module.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+extern int bpf_helper_print_add(int *a, int *b) __ksym;
+
+SEC("tc")
+int load(struct __sk_buff *skb)
+{
+	int a, b;
+
+	a = 3;
+	b = 4;
+	return bpf_helper_print_add(&a, &b);
+}
+
+char LICENSE[] SEC("license") = "GPL";
-- 
2.25.1

Re: [RFC bpf-next 0/3] bpf: Introduce module helper functions

[Alexei Starovoitov]

On Fri, Jan 21, 2022 at 07:39:53PM +0000, Usama Arif wrote:
> This patchset is a working prototype that adds support for calling helper
> functions in eBPF applications that have been defined in a kernel module.
> 
> It would require further work including code refractoring (not included in
> the patchset) to rename functions, data structures and variables that are
> used for kfunc as well to be appropriately renamed for module helper usage.
> If the idea of module helper functions and the approach used in this patchset
> is acceptable to the bpf community, I can send a follow up patchset with the
> code refractoring included to make it ready for review.
> 
> Module helpers are useful as:
> - They support more argument and return types when compared to module
> kfunc.

What exactly is missing?

> - 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.

Could you give an example of something that would be "too specialized" that
it's worth maintaining in a module, but not worth maintaining in the kernel?

Also see:
https://www.kernel.org/doc/html/latest/bpf/bpf_design_QA.html#q-new-functionality-via-kernel-modules

Why do you think we made that rule years ago?

Re: [RFC bpf-next 0/3] bpf: Introduce module helper functions

[Kumar Kartikeya Dwivedi]

On Sat, Jan 22, 2022 at 04:18:13AM IST, Alexei Starovoitov wrote:
> On Fri, Jan 21, 2022 at 07:39:53PM +0000, Usama Arif wrote:
> > This patchset is a working prototype that adds support for calling helper
> > functions in eBPF applications that have been defined in a kernel module.
> >
> > It would require further work including code refractoring (not included in
> > the patchset) to rename functions, data structures and variables that are
> > used for kfunc as well to be appropriately renamed for module helper usage.
> > If the idea of module helper functions and the approach used in this patchset
> > is acceptable to the bpf community, I can send a follow up patchset with the
> > code refractoring included to make it ready for review.
> >
> > Module helpers are useful as:
> > - They support more argument and return types when compared to module
> > kfunc.
>
> What exactly is missing?
>

I looked at the set. I think the only difference between existing support and
this series is that you are using bpf_func_proto for argument checks, right? Is
there anything else it is adding?

We discussed whether to use bpf_func_proto for kfunc in [0], the conclusion was
that BTF has enough info that we don't have to use it. The only thing missing
is making the verifier assume type of argument from kernel BTF rather than
passed in argument register.

e.g. same argument can currently work with PTR_TO_BTF_ID and PTR_TO_MEM. On
Alexei's suggestion, we disabled the bad cases by limiting PTR_TO_MEM support
to struct with scalars. For current usecase that works fine.

I think once BTF tags are supported, we will be able to tag the function
argument as __arg_mem or __arg_btf_id and make the verifier more strict.
Same can be done for the return value (instead of ret_null_set as it is now).

  [0]: https://lore.kernel.org/bpf/20211105204908.4cqxk2nbkas6bduw@ast-mbp.dhcp.thefacebook.com/

> > - 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.
>
> Could you give an example of something that would be "too specialized" that
> it's worth maintaining in a module, but not worth maintaining in the kernel?
>
> Also see:
> https://www.kernel.org/doc/html/latest/bpf/bpf_design_QA.html#q-new-functionality-via-kernel-modules
>
> Why do you think we made that rule years ago?

Re: [External] Re: [RFC bpf-next 0/3] bpf: Introduce module helper functions

[Usama Arif]

On 22/01/2022 04:04, Kumar Kartikeya Dwivedi wrote:
> On Sat, Jan 22, 2022 at 04:18:13AM IST, Alexei Starovoitov wrote:
>> On Fri, Jan 21, 2022 at 07:39:53PM +0000, Usama Arif wrote:
>>> This patchset is a working prototype that adds support for calling helper
>>> functions in eBPF applications that have been defined in a kernel module.
>>>
>>> It would require further work including code refractoring (not included in
>>> the patchset) to rename functions, data structures and variables that are
>>> used for kfunc as well to be appropriately renamed for module helper usage.
>>> If the idea of module helper functions and the approach used in this patchset
>>> is acceptable to the bpf community, I can send a follow up patchset with the
>>> code refractoring included to make it ready for review.
>>>
>>> Module helpers are useful as:
>>> - They support more argument and return types when compared to module
>>> kfunc.
>>
>> What exactly is missing?
>>
> 
> I looked at the set. I think the only difference between existing support and
> this series is that you are using bpf_func_proto for argument checks, right? Is
> there anything else it is adding?
> 
> We discussed whether to use bpf_func_proto for kfunc in [0], the conclusion was
> that BTF has enough info that we don't have to use it. The only thing missing
> is making the verifier assume type of argument from kernel BTF rather than
> passed in argument register.
> 
> e.g. same argument can currently work with PTR_TO_BTF_ID and PTR_TO_MEM. On
> Alexei's suggestion, we disabled the bad cases by limiting PTR_TO_MEM support
> to struct with scalars. For current usecase that works fine.
> 
> I think once BTF tags are supported, we will be able to tag the function
> argument as __arg_mem or __arg_btf_id and make the verifier more strict.
> Same can be done for the return value (instead of ret_null_set as it is now).
> 
>    [0]: https://lore.kernel.org/bpf/20211105204908.4cqxk2nbkas6bduw@ast-mbp.dhcp.thefacebook.com/
>

Hi,

Thanks for the replies and the reviews!

Yes, as Kumar said, using bpf_func_proto for argument checking and 
return register setting. In check_helper_call, the argument registers 
are checked (for e.g. at 
https://elixir.bootlin.com/linux/latest/source/kernel/bpf/verifier.c#L5324) 
and return registers are setup (for e.g. at 
https://elixir.bootlin.com/linux/latest/source/kernel/bpf/verifier.c#L6551) 
according to the arg_type and ret_type in bpf_func_proto. But i don't 
see the checks and setups done in check_kfunc_call for *all* 
bpf_return_type and bpf_reg_type.

I do see now that PTR_TO_BTF_ID ret_type was added to kfunc last week, 
but in addition to Kumars answer I think there are also more types that 
might not be handled.
For e.g. the return register types for RET_PTR_TO_SOCKET vs 
RET_PTR_TO_TCP_SOCK are set to different values depending on ret_type in 
bpf_func_proto. Similarly in check_func_arg called by check_helper_call 
only, the checks are different whether its ARG_PTR_TO_MAP_VALUE or 
ARG_PTR_TO_UNINIT_MAP_VALUE or other argument type. These aren't done in 
check_kfunc_call as there is no bpf_func_proto to distinguish between 
them. But maybe i am missing something over here?

I guess the other way to support the additional types above is as Kumar 
said by using BTF tags (I guess they they are being worked on as I didnt 
see any mention of it in code?) with kfunc. But wouldn't it be better to 
have consistency between the kfunc and helper functions in how different 
arguments and return types are handled? Dealing with many btf tags which 
will give same information about argument and return types as 
bpf_func_proto will likely just result in repeated code in 
check_helper_call and check_kfunc_call and further code to handle the tags?


>>> - 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.
>>
>> Could you give an example of something that would be "too specialized" that
>> it's worth maintaining in a module, but not worth maintaining in the kernel?
>>
>> Also see:
>> https://www.kernel.org/doc/html/latest/bpf/bpf_design_QA.html#q-new-functionality-via-kernel-modules
>>
>> Why do you think we made that rule years ago?

I guess the rule was made (I might be missing something over here!) as 
by moving functionality to kernel modules it will determine whether the 
eBPF application will work or not depending on the module, which makes 
the application less compatible across different systems, which is 
definitely not good. But i believe the above point does not remain since 
module kfuncs were introduced. I think that helpers on a very high level 
are similar to kfuncs with function protos for argument and return 
checking (ignoring implementation details of using helper id in 
bpf_helper_defs.h vs using ksysm section with BTF id in kfunc). So i 
think module helpers have the same affect from a compatibility 
perspective when compared to module kfunc, with the advantages of having 
bpf_func_proto.


Thanks again for the replies! I guess only worth addressing the comments 
and sending further patchsets with refractor for kfunc, if the idea is 
acceptable to the community and maintainers.

Thanks,
Usama