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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

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

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.