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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Jan 2022 10:51:29 +0530 From: Kumar Kartikeya Dwivedi <memxor@...il.com> To: Alexei Starovoitov <alexei.starovoitov@...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 Fri, Jan 14, 2022 at 10:19:50AM IST, Kumar Kartikeya Dwivedi wrote: > On Fri, Jan 14, 2022 at 04:02:11AM IST, Alexei Starovoitov wrote: > > On Tue, Jan 11, 2022 at 11:34:20PM +0530, Kumar Kartikeya Dwivedi wrote: > > > [...] > > > + /* 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? > > > > Right, I'm no expert on memory ordering, but even if we closed the race, to me > there seems to be no reason why the CPU cannot reorder the stores to tab (or its > hook/type slot) with mod->state = LIVE store. > > Usually, the visibility is handled by whatever lock is used to register the > module somewhere in some subsystem, as the next acquirer can see all updates > from the previous registration. > > In this case, we're directly assigning a pointer without holding any locks etc. > While it won't be concurrently accessed until module state is LIVE, it is > necessary to make all updates visible in the right order (that is, once state is > LIVE, everything stored previously in struct btf for module is also visible). > > Once mod->state = LIVE is visible, we will start accessing kfunc_set_tab, but if > previous stores to it were not visible by then, we'll access a badly-formed > kfunc_set_tab. > > For this particular case, you can think of mod->state = LIVE acting as a release > store, and the read for mod->state == LIVE acting as an acquire load. > Also, to be more precise, we're now synchronizing using btf_mod->flags, not mod->state, so I should atleast update the comment, but the idea is the same. > But I'm probably being overtly cautious, please let me know why. > -- Kartikeya
Powered by blists - more mailing lists