lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220113223211.s2m5fkvafd6fk4tv@ast-mbp.dhcp.thefacebook.com> Date: Thu, 13 Jan 2022 14:32:11 -0800 From: Alexei Starovoitov <alexei.starovoitov@...il.com> To: Kumar Kartikeya Dwivedi <memxor@...il.com> Cc: bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, netdev@...r.kernel.org, netfilter-devel@...r.kernel.org, Martin KaFai Lau <kafai@...com>, Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>, John Fastabend <john.fastabend@...il.com>, Maxim Mikityanskiy <maximmi@...dia.com>, Pablo Neira Ayuso <pablo@...filter.org>, Florian Westphal <fw@...len.de>, Jesper Dangaard Brouer <brouer@...hat.com>, Toke Høiland-Jørgensen <toke@...hat.com> Subject: Re: [PATCH bpf-next v7 02/10] bpf: Populate kfunc BTF ID sets in struct btf On Tue, Jan 11, 2022 at 11:34:20PM +0530, Kumar Kartikeya Dwivedi wrote: > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.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? > + /* 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.
Powered by blists - more mailing lists