Re: [PATCH bpf-next v5 1/2] libbpf: support selective kernel module BTF loading via bpf_object_open_opts

[bot+bpf-ci@kernel.org]

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

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b749c01742ee0..a466aabf35e1c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -5851,6 +5854,99 @@ int bpf_core_add_cands(struct bpf_core_cand *local_cand,
>  	return 0;
>  }
>
> +static void bpf_object_free_btf_module_names(struct bpf_object *obj)
> +{

Andrii Nakryiko raised a concern in v3 about the naming convention used
here. Internal helper functions should not use double underscore naming, as
that convention is reserved for extern method-like APIs. Would
bpf_object_free_module_names and bpf_object_init_module_names be more
appropriate, following the single underscore pattern for internal helpers?

Reference: https://lore.kernel.org/bpf/CAEf4BzbbdjeSW88EEk2xRvAxq-HWUJ7qTw4BYnL3kLH0f0uhwg@mail.gmail.com/

> +	size_t i;
> +
> +	if (!obj->btf_module_names)
> +		return;
> +
> +	for (i = 0; i < obj->nr_btf_module_names; i++)
> +		zfree(&obj->btf_module_names[i]);
> +	zfree(&obj->btf_module_names);
> +	obj->nr_btf_module_names = 0;
> +}
> +
> +static int bpf_object_init_btf_module_names(struct bpf_object *obj,
> +					    const struct bpf_object_open_opts *opts)
> +{
> +	const char **names;
> +	size_t i, j, cnt;
> +	int err;
> +
> +	names = OPTS_GET(opts, btf_module_names, NULL);
> +	if (!names)
> +		return 0;
> +
> +	cnt = OPTS_GET(opts, nr_btf_module_names, 0);
> +
> +	/*
> +	 * Allocate one entry for an empty list to distinguish it from the
> +	 * default behavior.
> +	 */
> +	obj->btf_module_names = calloc(cnt ?: 1,
> +				       sizeof(*obj->btf_module_names));
> +	if (!obj->btf_module_names)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < cnt; i++) {
> +		if (!names[i] || !names[i][0]) {
> +			pr_warn("invalid kernel module BTF name at index %zu\n", i);
> +			err = -EINVAL;
> +			goto err_out;
> +		}
> +
> +		for (j = 0; j < i; j++) {
> +			if (strcmp(obj->btf_module_names[j], names[i]) == 0) {
> +				pr_warn("duplicate kernel module BTF name '%s'\n",
> +					names[i]);
> +				err = -EINVAL;
> +				goto err_out;
> +			}
> +		}

Andrii Nakryiko suggested in v3 that the module name list should be sorted
and duplicates detected in a single pass rather than using the O(n^2)
nested loop approach. The suggestion was to error out with -EINVAL if
duplicates are found after sorting. Would this optimization be worth
implementing?

Reference: https://lore.kernel.org/bpf/CAEf4BzbbdjeSW88EEk2xRvAxq-HWUJ7qTw4BYnL3kLH0f0uhwg@mail.gmail.com/

> +
> +		obj->btf_module_names[i] = strdup(names[i]);
> +		if (!obj->btf_module_names[i]) {
> +			err = -ENOMEM;
> +			goto err_out;
> +		}
> +
> +		obj->nr_btf_module_names++;
> +	}
> +	return 0;
> +
> +err_out:
> +	bpf_object_free_btf_module_names(obj);
> +	return err;
> +}

[ ... ]

> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index b965ad5715403..b8c1d98d69d85 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -224,10 +224,32 @@ struct bpf_object_open_opts {
>  	 * point (/sys/fs/bpf), in case this default behavior is undesirable.
>  	 */
>  	const char *bpf_token_path;
> +	/*
> +	 * Optional list of kernel module names whose BTFs should be loaded.
> +	 * nr_btf_module_names specifies the number of entries in
> +	 * btf_module_names.
> +	 *
> +	 * With btf_module_names:
> +	 * - when provided, only the BTFs of the specified modules are loaded;
> +	 * - when an empty list is provided, no module BTFs are loaded;
> +	 * - when NULL, all module BTFs are loaded as before.
> +	 *
> +	 * The list must not contain duplicate entries; otherwise -EINVAL is
> +	 * returned.
> +	 *
> +	 * This affects:
> +	 * - BPF CO-RE relocations against types defined in modules;
> +	 * - BTF-based resolution of function attach targets for
> +	 *   fentry/fexit/fmod_ret/freplace/LSM programs;
> +	 * - struct_ops kernel type resolution;
> +	 * - extern (ksym) resolution for kernel symbols defined in modules.
> +	 */

Looking at the 'This affects:' list and the -EINVAL contract, are there two
things missing?

First, load_module_btfs() has four callers in libbpf.c, but the list above
covers only three: bpf_core_find_cands (CO-RE relocations), find_kernel_btf_id
(fentry/fexit/etc), and find_ksym_btf_id (extern resolution). The fourth
caller is find_attach_module(), which is reached from tracing_multi_mod_fd()
and collect_func_ids_by_glob() for 'module:glob' attach patterns:

    mod = find_attach_module(obj, pattern);
    if (!mod) {
        err = -EINVAL;
        goto cleanup;
    }

So a user who sets btf_module_names and then attaches a tracing-multi
program with 'mymod:foo*' gets -EINVAL if mymod is not in the list, but the
documented behavior above doesn't hint that this option affects multi-attach
module resolution.

Second, the -EINVAL contract mentions only duplicates, but
bpf_object_init_btf_module_names() also rejects NULL and empty-string
entries:

    if (!names[i] || !names[i][0]) {
        pr_warn("invalid kernel module BTF name at index %zu\n", i);
        err = -EINVAL;

Would it be worth clarifying both of these in the documentation?

> +	const char **btf_module_names;
> +	size_t nr_btf_module_names;
>
>  	size_t :0;
>  };
> -#define bpf_object_open_opts__last_field bpf_token_path
> +#define bpf_object_open_opts__last_field nr_btf_module_names


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