netdev - Re: [PATCH bpf-next] bpf: fix register_btf_kfunc_id_set for !CONFIG_DEBUG_INFO

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] [day] [month] [year] [list]

Message-ID: <20220125234936.qal3xzsigkpauofj@apollo.legion>
Date:   Wed, 26 Jan 2022 05:19:36 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     sdf@...gle.com
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org
Subject: Re: [PATCH bpf-next] bpf: fix register_btf_kfunc_id_set for
 !CONFIG_DEBUG_INFO_BTF

On Tue, Jan 25, 2022 at 10:20:51PM IST, sdf@...gle.com wrote:
> On 01/25, Kumar Kartikeya Dwivedi wrote:
> > On Tue, Jan 25, 2022 at 06:08:45AM IST, Stanislav Fomichev wrote:
> > > Commit dee872e124e8 ("bpf: Populate kfunc BTF ID sets in struct btf")
> > > breaks loading of some modules when CONFIG_DEBUG_INFO_BTF is not set.
> > > register_btf_kfunc_id_set returns -ENOENT to the callers when
> > > there is no module btf. Let's return 0 (success) instead to let
> > > those modules work in !CONFIG_DEBUG_INFO_BTF cases.
> > >
> > > Cc: Kumar Kartikeya Dwivedi <memxor@...il.com>
> > > Fixes: dee872e124e8 ("bpf: Populate kfunc BTF ID sets in struct btf")
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
>
> > Thanks for the fix.
>
> > >  kernel/bpf/btf.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index 57f5fd5af2f9..24205c2d4f7e 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6741,7 +6741,7 @@ int register_btf_kfunc_id_set(enum bpf_prog_type
> > prog_type,
> > >
> > >  	btf = btf_get_module_btf(kset->owner);
> > >  	if (IS_ERR_OR_NULL(btf))
> > > -		return btf ? PTR_ERR(btf) : -ENOENT;
> > > +		return btf ? PTR_ERR(btf) : 0;
>
> > I think it should still be an error when CONFIG_DEBUG_INFO_BTF is enabled.
>
> > How about doing it differently:
>
> > Make register_btf_kfunc_id_set, btf_kfunc_id_set_contains, and functions
> > only
> > called by them all dependent upon CONFIG_DEBUG_INFO_BTF. Then code picks
> > the
> > static inline definition from the header and it works fine with 'return
> > 0' and
> > 'return false'.
>
> > In case CONFIG_DEBUG_INFO_BTF is enabled, but
> > CONFIG_DEBUG_INFO_BTF_MODULES is
> > disabled, we can do the error upgrade but inside btf_get_module_btf.
>
> > I.e. extend the comment it has to say that when it returns NULL, it
> > means there
> > is no BTF (hence nothing to do), but it never returns NULL when
> > DEBUF_INFO_BTF*
> > is enabled, but upgrades the btf == NULL to a PTR_ERR(-ENOENT), because
> > the btf
> > should be there when the options are enabled.
>
> > e.g. If CONFIG_DEBUG_INFO_BTF=y but CONFIG_DEBUG_INFO_BTF_MODULES=n, it
> > can
> > return NULL for owner == <some module ptr>, but not for owner == NULL
> > (vmlinux),
> > because CONFIG_DEBUG_INFO_BTF is set. If both are disabled, it can
> > return NULL
> > for both. If both are set, it will never return NULL.
>
> > Then the caller can just special case NULL depending on their usage.
>
> > And your current diff remains same combined with the above changes.
>
> > WDYT? Does this look correct or did I miss something important?
>
> I initially started with this approach, adding ifdef
> CONFIG_DEBUG_INFO_BTF/CONFIG_DEBUG_INFO_BTF_MODULES, but it quickly
> became a bit ugly :-( I can retry if you prefer, but how about, instead,
> we handle it explicitly this way in the caller?
>
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 24205c2d4f7e..e66f60b288d0 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6740,8 +6740,19 @@ int register_btf_kfunc_id_set(enum bpf_prog_type
> prog_type,
>  	int ret;
>
>  	btf = btf_get_module_btf(kset->owner);
> -	if (IS_ERR_OR_NULL(btf))
> -		return btf ? PTR_ERR(btf) : 0;
> +	if (!btf) {
> +		if (!kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF)) {
> +			pr_err("missing vmlinux BTF\n");
> +			return -ENOENT;
> +		}
> +		if (kset->owner && IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES)) {
> +			pr_err("missing module BTF\n");
> +			return -ENOENT;
> +		}
> +		return 0;
> +	}
> +	if (IS_ERR(btf))
> +		return PTR_ERR(btf);
>
>  	hook = bpf_prog_type_to_kfunc_hook(prog_type);
>  	ret = btf_populate_kfunc_set(btf, hook, kset);
>
> Basically, treat as error the cases we care about:
> - non-module && CONFIG_DEBUG_INFO_BTF -> ENOENT
> - module && CONFIG_DEBUG_INFO_BTF_MODULES -> ENOENT
>
> Also give the user some hint on what went wrong; insmod gave me "Unknown
> symbol in module, or unknown parameter (see dmesg)" for ENOENT (and
> dmesg was empty).

Alright, LGTM. Also maybe we can say "missing vmlinux BTF, cannot register
kfuncs" instead.

--
Kartikeya