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