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

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

[Stanislav Fomichev]

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>
---
 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;
 
 	hook = bpf_prog_type_to_kfunc_hook(prog_type);
 	ret = btf_populate_kfunc_set(btf, hook, kset);
-- 
2.35.0.rc0.227.g00780c9af4-goog

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

[Kumar Kartikeya Dwivedi]

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?

PS: While we are at it, maybe also add a NULL check for btf and return false
from btf_kfunc_id_set_contains, even though on current inspection it doesn't
seem to be a problem, since all users use find_kfunc_desc_btf which handles that
case.

>
>  	hook = bpf_prog_type_to_kfunc_hook(prog_type);
>  	ret = btf_populate_kfunc_set(btf, hook, kset);
> --
> 2.35.0.rc0.227.g00780c9af4-goog
>

--
Kartikeya

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

[sdf]

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).

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

[Kumar Kartikeya Dwivedi]

On Tue, Jan 25, 2022 at 10:20:51PM IST, sdf@google.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@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).

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

--
Kartikeya