netdev - Re: [PATCH bpf-next v7 02/10] bpf: Populate kfunc BTF ID sets in struct btf

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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