Date:   Fri, 14 Jan 2022 10:19:50 +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 04:02:11AM IST, Alexei Starovoitov wrote:
> 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
>

Ok, will do.

> >  		 */
> > -		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?
>

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.

But I'm probably being overtly cautious, please let me know why.

> > +	/* 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.

Ok, I'll drop this.

Thanks!
--
Kartikeya

Powered by blists - more mailing lists