On Tue, Jan 11, 2022 at 11:34:20PM +0530, Kumar Kartikeya Dwivedi wrote:
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> 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?
> + /* 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_MODULES
>
> diff --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.
