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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Jan 2022 07:40:08 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org
Subject: Re: [PATCH bpf-next] bpf: fix register_btf_kfunc_id_set for
 !CONFIG_DEBUG_INFO_BTF

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