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

[sdf@google.com]

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@gmail.com>
> > Fixes: dee872e124e8 ("bpf: Populate kfunc BTF ID sets in struct btf")
> > Signed-off-by: Stanislav Fomichev <sdf@google.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).