Re: [PATCH bpf-next v8 3/7] libbpf: BPF programs manual loading and attaching

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] libbpf: bpf_object__free_relocs() frees reloc_desc for manual progs
- [Medium] libbpf: bpf_program__load() reload fails for jump and func_ptr maps
- [Medium] libbpf: bpf_program__load() allows loading after failed prepare/load
- [Medium] libbpf: bpf_map__init_kern_struct_ops() clobbers manual load strategy
--

commit 2dd11a6f941c4fbe6a6982f6a14ae6a049d19390
Author: Slava Imameev <slava.imameev@crowdstrike.com>

libbpf: BPF programs manual loading and attaching

This commit adds BPF_PROG_LOAD_STRATEGY_MANUAL and bpf_program__load() so
that designated BPF programs can be skipped during bpf_object__load() and
loaded, unloaded, and reloaded on demand afterwards.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index e4cdbc2f467db..1d5d59b95c4e8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -882,12 +883,30 @@ void bpf_program__unload(struct bpf_program *prog)
>  	zfree(&prog->subprogs);
>  }
>  
> +void bpf_program__unload(struct bpf_program *prog)
> +{
> +	if (!prog)
> +		return;
> +
> +	/*
> +	 * MANUAL programs retain their data here so bpf_program__load()
> +	 * can reload them later; object teardown paths call
> +	 * bpf_program_unload_full() instead to always release it fully.
> +	 */
> +	if (prog->load_strategy == BPF_PROG_LOAD_STRATEGY_MANUAL) {
> +		zclose(prog->fd);
> +		return;
> +	}

[Severity: Medium]
What happens when a BPF_PROG_LOAD_STRATEGY_MANUAL program that uses jump
tables or indirect function call tables in read-only data is unloaded here
and then reloaded via bpf_program__load()?

During bpf_object_prepare() -> bpf_object__relocate() ->
bpf_object__relocate_data(), libbpf creates per-program frozen internal
maps once via create_jt_map() (for RELO_INSN_ARRAY) and
create_func_ptr_map() (when map->autocreate && map->func_ptr_cnt &&
prog->has_callx), patching their file descriptors directly into
prog->insns:

tools/lib/bpf/libbpf.c:bpf_object__relocate_data() {
    ...
    if (map->autocreate && map->func_ptr_cnt && prog->has_callx) {
        int map_fd;

        /* the program gets its own map with pointers to its functions */
        map_fd = create_func_ptr_map(obj, prog, relo->map_idx);
        ...
        insn[0].imm = map_fd;
    }
    ...
}

On the first bpf_program__load() call, the kernel verifier claims these
maps permanently:

1. For jump table maps, bpf_insn_array_init() sets insn_array->used to 1 via
   atomic_xchg(&insn_array->used, 1), which is only released on verifier
   failure and never cleared when a loaded program is later freed.

2. For function pointer maps, resolve_func_ptrs() sets BPF_MAP_USER_PATCHED
   on map->user and overwrites the subprogram byte offsets in the frozen
   map in-place with JITed kernel addresses; __bpf_free_used_maps() does
   not clear BPF_MAP_USER_PATCHED or restore the offsets when the program
   is closed.

When bpf_program__unload() closes prog->fd and bpf_program__load() is
called again, prog->insns still references those same map file descriptors.
Won't the kernel verifier reject the reload with -EBUSY in
bpf_insn_array_init() or bpf_map_claim()?

[ ... ]

> @@ -1228,6 +1247,14 @@ static int bpf_object_adjust_struct_ops_autoload(struct bpf_object *obj)
>  		if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
>  			continue;
>  
> +		/*
> +		 * If the user set the load strategy to MANUAL, don't guess it
> +		 * from map autocreate: a MANUAL program is loaded explicitly
> +		 * with bpf_program__load(), which rejects any other strategy.
> +		 */
> +		if (prog->load_strategy == BPF_PROG_LOAD_STRATEGY_MANUAL)
> +			continue;

[Severity: Medium]
Can bpf_map__init_kern_struct_ops() overwrite prog->load_strategy before
bpf_object_adjust_struct_ops_autoload() reaches this check?

In bpf_object_prepare(), bpf_object__init_kern_struct_ops_maps() ->
bpf_map__init_kern_struct_ops() runs immediately before
bpf_object_adjust_struct_ops_autoload(). Inside
bpf_map__init_kern_struct_ops(), if a struct_ops member is absent in the
running kernel's BTF and zeroed in shadow data, or if the slot was cleared
or replaced in shadow data, it unconditionally resets load_strategy to
BPF_PROG_LOAD_STRATEGY_DISABLED:

tools/lib/bpf/libbpf.c:bpf_map__init_kern_struct_ops() {
    ...
    if (!kern_member) {
        ...
        if (st_ops->progs[i]) {
            st_ops->progs[i]->load_strategy = BPF_PROG_LOAD_STRATEGY_DISABLED;
            st_ops->progs[i] = NULL;
        }
        ...
    }
    ...
    if (btf_is_ptr(mtype)) {
        prog = *(void **)mdata;
        if (st_ops->progs[i] && st_ops->progs[i] != prog)
            st_ops->progs[i]->load_strategy = BPF_PROG_LOAD_STRATEGY_DISABLED;
    ...
}

If the user explicitly set BPF_PROG_LOAD_STRATEGY_MANUAL on that program via
bpf_program__set_load_strategy(), won't bpf_map__init_kern_struct_ops()
clobber it to BPF_PROG_LOAD_STRATEGY_DISABLED (or cause
bpf_object_adjust_struct_ops_autoload() to set it to
BPF_PROG_LOAD_STRATEGY_AUTO if referenced by another autocreated map)?

[ ... ]

> @@ -16298,3 +16430,32 @@ enum bpf_prog_load_strategy bpf_program__load_strategy(const struct bpf_program
>  {
>  	return prog->load_strategy;
>  }
> +
> +int
> +bpf_program__load(struct bpf_program *prog)
> +{
> +	int err;
> +	struct bpf_object *obj = prog->obj;
> +
> +	/*
> +	 * This function cannot be called before OBJ_PREPARED -
> +	 * Program's data is initialized on object prepare.
> +	 */
> +	if (obj->state < OBJ_PREPARED)
> +		return libbpf_err(-EINVAL);

[Severity: Medium]
Is checking obj->state < OBJ_PREPARED sufficient if bpf_object_prepare() or
bpf_object_load() previously failed?

When bpf_object_prepare() or bpf_object_load() fails, it tears down the
object via bpf_object_unload(obj) and sets obj->state = OBJ_LOADED:

tools/lib/bpf/libbpf.c:bpf_object_prepare() {
    ...
    if (err) {
        bpf_object_unpin(obj);
        bpf_object_unload(obj);
        obj->state = OBJ_LOADED;
        return err;
    }
}

Because OBJ_LOADED is greater than OBJ_PREPARED, a subsequent call to
bpf_program__load() passes this state check and attempts to load the
program after bpf_object_unload() has already closed all map file
descriptors and called bpf_program_unload_full() (freeing prog->func_info,
prog->line_info, and prog->subprogs to NULL while leaving their counts
non-zero, or before prog->insns was relocated and sanitized).

> +
> +	if (prog_is_subprog(obj, prog) || prog->load_strategy != BPF_PROG_LOAD_STRATEGY_MANUAL)
> +		return libbpf_err(-EINVAL);
> +
> +	if (prog->fd >= 0)
> +		return libbpf_err(-EBUSY);
> +
> +	err = bpf_object_load_prog(obj, prog, prog->insns, prog->insns_cnt,
> +				   obj->license, obj->kern_version, &prog->fd);
> +	if (err) {
> +		pr_warn("prog '%s': failed to load: %s\n", prog->name, errstr(err));
> +		return libbpf_err(err);
> +	}

[Severity: Medium]
Could CO-RE relocation failure diagnostics in the verifier log be lost here
when bpf_program__load() is called after bpf_object__load()?

During bpf_object_prepare() -> bpf_object__relocate() ->
bpf_object__relocate_core(), CO-RE relocations are recorded in
prog->reloc_desc for all non-disabled programs, and unguarded failing CO-RE
instructions are poisoned with call 195896080.

However, at the end of bpf_object__load_progs(), libbpf calls
bpf_object__free_relocs(obj), which frees prog->reloc_desc and zeroes
prog->nr_reloc for all programs in the object, including
BPF_PROG_LOAD_STRATEGY_MANUAL programs that were skipped during the
autoload loop:

tools/lib/bpf/libbpf.c:bpf_object__free_relocs() {
    ...
    for (i = 0; i < obj->nr_programs; i++) {
        prog = &obj->programs[i];
        zfree(&prog->reloc_desc);
        prog->nr_reloc = 0;
    }
}

When bpf_program__load() later calls bpf_object_load_prog() and
verification fails on a poisoned CO-RE relocation, fixup_verifier_log() ->
fixup_log_failed_core_relo() -> find_relo_core() searches prog->reloc_desc
up to prog->nr_reloc, finds nothing, and leaves the raw poisoned call error
in the verifier log instead of reporting the failed CO-RE relocation
details.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024014514.2959514-1-andrey.grodzovsky@crowdstrike.com?part=3