Re: [RFC PATCH bpf-next v2 1/2] libbpf: support selective kernel module BTF loading via .kmod_btfs section

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 9d160b5b9c0ee..171ea055cd323 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -188,6 +188,20 @@ enum libbpf_tristate {
>  	TRI_MODULE = 2,
>  };
>
> +/* Helper typedef for declaring kernel module names that need BTF loading.
> + *
> + * Usage: define an array in the ".kmod_btfs" ELF section to specify
> + * which modules need BTF loading:
> + *
> + *   DEFINE_KMOD_BTFS(_needed_kmods)= { "module1", "module2", ... };
> + *
> + * This avoids unnecessary BTF loading and speeds up the BPF program
> + * load process.
> + */
> +#define KMOD_NAME_LEN 64
> +#define DEFINE_KMOD_BTFS(name) \
> +	SEC(".kmod_btfs") char name[][KMOD_NAME_LEN]

KMOD_NAME_LEN establishes the on-disk record size for the .kmod_btfs section
format, yet it's duplicated in two files with only a prose comment connecting
them:

  tools/lib/bpf/bpf_helpers.h:201   #define KMOD_NAME_LEN 64
  tools/lib/bpf/libbpf.c:707-708    /* Should match the definition in ... */
                                    #define KMOD_NAME_LEN 64

If the two ever diverge, bpf_object__collect_kmod_btf_names() will silently
mis-slice the section. The d_size % KMOD_NAME_LEN check only catches
non-multiples, not a mismatch between the two values. Could this use a shared
header or a static assert to ensure they stay in sync?

Also, the new macro has no #ifndef guard. This header is included by nearly
every BPF program alongside vmlinux.h and user headers. Other unprefixed
names this header exports (NULL at lines 78-80, KERNEL_VERSION at 82-84) all
have #ifndef wrappers. Should KMOD_NAME_LEN follow that pattern?

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 514e4e9daa825..9fe8a653e8b6b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -5803,6 +5888,11 @@ int bpf_core_add_cands(struct bpf_core_cand *local_cand,
>  	return 0;
>  }
>
> +static bool is_kmod_btf_needed(struct bpf_object *obj, const char *name)
> +{
> +	return hashmap_find(obj->kmod_btfs->hashmap, (long)name, NULL);
> +}

This calls the raw hashmap_find() with a manual (long) cast, rather than
the hashmap__find() wrapper that hashmap.h provides:

  hashmap.h:159
  #define hashmap__find(map, key, value) \
      hashmap_find((map), (long)(key), hashmap_cast_ptr(value))

The header comments at lines 50 and 54 say the intent is to use the wrapper
("behind auxiliary macros") so that the _Static_assert on the value pointer
runs. Bypassing the macro also bypasses that check.

This is the only direct caller of the unversioned function in the whole
library. The other function added by this same patch does use the wrapper
(bpf_object__collect_kmod_btf_names at libbpf.c:964 uses hashmap__find), so
the two halves of the change are inconsistent. Should this use the wrapper
too?

[ ... ]

> @@ -9030,6 +9130,12 @@ static void bpf_object_cleanup_btf(struct bpf_object *obj)
>  	/* clean up vmlinux BTF */
>  	btf__free(obj->btf_vmlinux);
>  	obj->btf_vmlinux = NULL;
> +
> +	if (obj->kmod_btfs) {
> +		hashmap__free(obj->kmod_btfs->hashmap);
> +		zfree(&obj->kmod_btfs->data);
> +		zfree(&obj->kmod_btfs);
> +	}
>  }

The .kmod_btfs filter list is ELF open-time data, but it's freed from
bpf_object_cleanup_btf(), which runs while the bpf_object is still alive and
load_module_btfs() can still be called. The filter therefore stops applying
part-way through the object's lifetime. Concrete path:

  bpf_object_load()                       libbpf.c:9182
    ... -> bpf_object_post_load_cleanup() libbpf.c:9229
             -> bpf_object_cleanup_btf()  libbpf.c:9147
                  obj->btf_modules_loaded = false;   (libbpf.c:9128)
                  zfree(&obj->kmod_btfs);            (libbpf.c:9137)

  bpf_program__attach_tracing_multi()     libbpf.c:12791
    -> collect_func_ids_by_glob()         libbpf.c:12752
         -> find_attach_module()          libbpf.c:7877
              -> load_module_btfs()       libbpf.c:5896

At this second entry into load_module_btfs(), btf_modules_loaded is false so
the full scan re-runs, but obj->kmod_btfs is NULL. The filter at
libbpf.c:5960 is skipped and every module BTF in the system is loaded,
exactly what the section is supposed to prevent. collect_func_ids_by_glob()
also calls bpf_object_cleanup_btf(obj) unconditionally at its cleanup: label
(libbpf.c:12786), so the list is destroyed again on every attach.

The other open-time members with the same lifetime (obj->jumptables_data,
obj->arena_data, obj->kconfig) are freed in bpf_object__close()
(libbpf.c:9720-9733), not in the BTF cleanup helper. Should obj->kmod_btfs
be freed there instead?


---
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/31664172915