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

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

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