On Tue, Jan 25, 2022 at 10:20:51PM IST, sdf@xxxxxxxxxx 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@xxxxxxxxx>
> > > Fixes: dee872e124e8 ("bpf: Populate kfunc BTF ID sets in struct btf")
> > > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > > ---
>
> > 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
