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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 11, 2022 at 11:34:20PM +0530, Kumar Kartikeya Dwivedi wrote:
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
>  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.



[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux