Re: [PATCH bpf-next 02/13] bpf: fetch the vmlinux BTF where kernel types enter a program

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 7735 bytes --]

> bpf: fetch the vmlinux BTF where kernel types enter a program

TL;DR:
- check_ptr_to_map_access(): without BTF, the map_ptr error now names the map
  type, not the missing BTF, and the new !btf branch looks unreachable
- Co-developed-by is followed by a placeholder, not a Signed-off-by (commit
  message)

> bpf: fetch the vmlinux BTF where kernel types enter a program
>
> bpf_check() fetches the vmlinux BTF up front for every program, whether
> the program uses kernel types or not.  That is free today because the
> BTF is parsed at boot anyway.  Once it can be parsed on first use
> (CONFIG_DEBUG_INFO_BTF_LAZY, later in this series), it would make every
> program load pay for parsing 5-7 MB of BTF, including the socket filters
> systemd loads at boot.
> Drop the up-front fetch and fetch where kernel types enter the verifier
> state instead:
>  - bpf_add_kfunc_call(), for the first kfunc call of a program;
>  - check_ptr_to_map_access(), for map_ptr field access;
>  - check_pseudo_btf_id(), for ldimm64 of a kernel variable (ksym);
>  - check_helper_call(), for helpers whose prototype takes
>    ARG_PTR_TO_BTF_ID or returns RET_PTR_TO_BTF_ID, and for
>    bpf_snprintf_btf() and bpf_seq_printf_btf(), which take a kernel
>    type id inside struct btf_ptr (helper_uses_vmlinux_btf()).
> The other ways in already fetch the BTF themselves: attach_btf in
> bpf_prog_load(), struct_ops map creation, CO-RE candidate lookup,
> bpf_find_btf_id(), btf_prepare_func_args() for kernel-typed arguments
> and the kernel context type lookups.  The remaining reads of the
> btf_vmlinux global in the verifier are on paths dominated by one of
> these fetches.  A program that uses none of them, such as a socket
> filter, does not need the vmlinux BTF any more, and verifies the same
> whether or not it has been parsed.
> Four related changes keep it that way:
>  - The "in-kernel BTF is malformed" check in bpf_check() read the
>    global right after the up-front fetch.  Without the fetch it would
>    pass or fail depending on whether something else had parsed the BTF
>    before, so drop it; every fetch point reports a missing BTF itself
>    and passes on an error of the parse, as that check did.
>  - check_ptr_to_map_access() checks allow_ptr_leaks before it fetches
>    the BTF, and bpf_prog_load() falls back to the vmlinux BTF for an
>    attach_btf_id only with CAP_BPF, which all program types that take
>    one need, so that an unprivileged load cannot make the kernel parse
>    it only to be refused.  The map_ptr permission error can then not
>    name the map's struct any more and says "map_ptr access is allowed
>    only to CAP_PERFMON and CAP_SYS_ADMIN".  A ksym, which unprivileged
>    programs may use, still parses it.
>  - Add bpf_peek_btf_vmlinux(), which returns the vmlinux BTF only if it
>    is parsed already, and use it in bpf_btf_printf_prepare().  The BTF
>    printf helpers run in program context, which may be atomic, and the
>    verifier has fetched the BTF for every program that calls them.
>  - is_kfunc_dynptr_write(), reached from the argument checks that
>    helpers and kfuncs share, compared meta->btf with the global, which
>    a helper's NULL matches while the global is NULL.  Check that the
>    call is a kfunc first, as the other kfunc predicates there do.
> Helpers taking ARG_PTR_TO_BTF_ID_SOCK_COMMON are left out on purpose:
> they take sockets without BTF types too, the shared argument check reads
> the global only for a PTR_TO_BTF_ID register, and every source of such
> a register fetched the BTF.
> No functional change with CONFIG_DEBUG_INFO_BTF=y, where kfunc
> registrations at boot parse the BTF, other than that error message and
> that, if the vmlinux BTF fails to parse (a broken toolchain), programs
> that use no kernel types now load instead of failing.  Without BTF the
> new helper check is compiled out and the existing error paths are
> unchanged.
> Co-developed-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
> Signed-off-by: Jay Wang <wanjay@amazon.com>

This isn't a bug, but the Co-developed-by: tag is followed by a placeholder
line, "TODO-NEEDS-IHOR: Signed-off-by: Ihor Solodrai
<ihor.solodrai@linux.dev>", rather than a real Signed-off-by: line.

Documentation/process/submitting-patches.rst asks for a Co-developed-by: to
be immediately followed by that co-author's Signed-off-by:.  Should the
placeholder be replaced with the real tag before this is applied?

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1b6c0674ff6a..18b2bd009d58 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -6867,30 +6883,33 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,
>  	enum bpf_type_flag flag = 0;
>  	const struct btf_type *t;
>  	const char *tname;
> +	struct btf *btf;
>  	u32 btf_id;
>  	int ret;
>
> -	if (!btf_vmlinux) {
> -		verbose(env, "map_ptr access not supported without CONFIG_DEBUG_INFO_BTF\n");
> -		return -ENOTSUPP;
> -	}
> -
>  	if (!map->ops->map_btf_id || !*map->ops->map_btf_id) {
>  		verbose(env, "map_ptr access not supported for map type %d\n",
>  			map->map_type);
>  		return -ENOTSUPP;
>  	}
>
> -	t = btf_type_by_id(btf_vmlinux, *map->ops->map_btf_id);
> -	tname = btf_name_by_offset(btf_vmlinux, t->name_off);
> -
> +	/* before the BTF fetch: unprivileged loads must not parse it */
>  	if (!env->allow_ptr_leaks) {
> -		verbose(env,
> -			"'struct %s' access is allowed only to CAP_PERFMON and CAP_SYS_ADMIN\n",
> -			tname);
> +		verbose(env, "map_ptr access is allowed only to CAP_PERFMON and CAP_SYS_ADMIN\n");
>  		return -EPERM;
>  	}
>
> +	btf = bpf_get_btf_vmlinux();
> +	if (IS_ERR(btf))
> +		return vmlinux_btf_err(env, btf);
> +	if (!btf) {
> +		verbose(env, "map_ptr access not supported without CONFIG_DEBUG_INFO_BTF\n");
> +		return -ENOTSUPP;
> +	}
> +
> +	t = btf_type_by_id(btf, *map->ops->map_btf_id);
> +	tname = btf_name_by_offset(btf, t->name_off);
> +
>  	if (off < 0) {

Can the new "if (!btf)" branch ever be reached?

Without CONFIG_DEBUG_INFO_BTF, every map type's .map_btf_id points at a
zero-filled array:

include/linux/btf_ids.h:
    #define BTF_ID_LIST_SINGLE(name, prefix, typename) static u32 __maybe_unused name[1];

so "!*map->ops->map_btf_id" is true for every map, and the function returns
at the first check with "map_ptr access not supported for map type %d".

With CONFIG_DEBUG_INFO_BTF=y, bpf_get_btf_vmlinux() returns either a BTF or
an ERR_PTR, never NULL, so the branch is unreachable in that configuration
too.

Before this patch, "if (!btf_vmlinux)" was the first test.  On a kernel
without BTF, a privileged program doing "r0 = *(u32 *)(map_ptr + off)" was
told:

    map_ptr access not supported without CONFIG_DEBUG_INFO_BTF

It is now told, for example:

    map_ptr access not supported for map type 1

That message points at the map type rather than at the missing BTF.  The
errno (-ENOTSUPP) is unchanged.

This also looks at odds with the commit message statement "Without BTF the
new helper check is compiled out and the existing error paths are
unchanged."

Would an early IS_ENABLED(CONFIG_DEBUG_INFO_BTF) test with the old message,
placed before the map_btf_id check, be a way to keep the old diagnostic?  It
would not fetch or parse anything, so the allow_ptr_leaks-before-fetch
ordering would stay as is.

No later commit in the series changes check_ptr_to_map_access().


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41205212136