Re: [PATCH bpf-next v7 02/10] bpf: Populate kfunc BTF ID sets in struct btf
From: Alexei Starovoitov <hidden>
Date: 2022-01-13 22:32:19
Also in:
bpf, netfilter-devel
On Tue, Jan 11, 2022 at 11:34:20PM +0530, Kumar Kartikeya Dwivedi wrote:
quoted hunk ↗ jump to hunk
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- include/linux/btf.h | 46 ++++++++ include/linux/btf_ids.h | 13 ++- kernel/bpf/btf.c | 253 +++++++++++++++++++++++++++++++++++++++- kernel/bpf/verifier.c | 1 + 4 files changed, 305 insertions(+), 8 deletions(-)diff --git a/include/linux/btf.h b/include/linux/btf.h index 0c74348cbc9d..95a8238272af 100644 --- a/include/linux/btf.h +++ b/include/linux/btf.h@@ -12,11 +12,40 @@ #define BTF_TYPE_EMIT(type) ((void)(type *)0) #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val) +enum btf_kfunc_hook { + BTF_KFUNC_HOOK_XDP, + BTF_KFUNC_HOOK_TC, + BTF_KFUNC_HOOK_STRUCT_OPS, + BTF_KFUNC_HOOK_MAX, +};
The enum doesn't have to be in .h, right? Would be cleaner to reduce its scope to btf.c
*/
- if ((btf_mod->flags & BTF_MODULE_F_LIVE) && try_module_get(btf_mod->module))
+ if ((btf_mod->flags & BTF_MODULE_F_LIVE) && try_module_get(btf_mod->module)) {
+ /* pairs with smp_wmb in register_btf_kfunc_id_set */
+ smp_rmb();Doesn't look necessary. More below.
+/* This function must be invoked only from initcalls/module init functions */
+int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
+ const struct btf_kfunc_id_set *kset)
+{
+ enum btf_kfunc_hook hook;
+ struct btf *btf;
+ int ret;
+
+ btf = btf_get_module_btf(kset->owner);
+ if (IS_ERR_OR_NULL(btf))
+ return btf ? PTR_ERR(btf) : -ENOENT;
+
+ hook = bpf_prog_type_to_kfunc_hook(prog_type);
+ ret = btf_populate_kfunc_set(btf, hook, kset);
+ /* Make sure all updates are visible before we go to MODULE_STATE_LIVE,
+ * pairs with smp_rmb in btf_try_get_module (for success case).
+ *
+ * btf_populate_kfunc_set(...)
+ * smp_wmb() <-----------.
+ * mod->state = LIVE | if (mod->state == LIVE)
+ * | atomic_inc_nz(mod)
+ * `---------> smp_rmb()
+ * btf_kfunc_id_set_contains(...)
+ */
+ smp_wmb();This comment somehow implies that mod->state = LIVE and if (mod->state == LIVE && try_mod_get) can race. That's not the case. The patch 1 closed the race. btf_kfunc_id_set_contains() will be called only on LIVE modules. At that point all __init funcs of the module including register_btf_kfunc_id_set() have completed. This smp_wmb/rmb pair serves no purpose. Unless I'm missing something?
quoted hunk ↗ jump to hunk
+ /* reference is only taken for module BTF */ + if (btf_is_module(btf)) + btf_put(btf); + return ret; +} +EXPORT_SYMBOL_GPL(register_btf_kfunc_id_set); #ifdef CONFIG_DEBUG_INFO_BTF_MODULESdiff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index bfb45381fb3f..b5ea73560a4d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c@@ -1783,6 +1783,7 @@ static struct btf *__find_kfunc_desc_btf(struct bpf_verifier_env *env, mod = btf_try_get_module(btf); if (!mod) { + verbose(env, "failed to get reference for BTF's module\n");
This one is highly unlikely, right? One can see it only with a specially crafted test like patch 10. Normal users will never see it. Why add it then? Also there are two places in verifier.c that calls btf_try_get_module(). If it's a real concern, both places should have verbose(). But I would not add it in either.