Re: [PATCH bpf-next v2 2/5] libbpf: BPF programs dynamic loading and attaching

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] `bpf_program__load_dynamically` overwrites `prog->fd` without closing the old one, leading to a file descriptor leak if the program is already loaded.
- [Medium] Public API `bpf_program__load_dynamically` returns a negative error code directly on failure instead of wrapping it in `libbpf_err()`, violating LIBBPF-001.
- [Medium] `bpf_program__load_dynamically` permanently corrupts the user-configured `log_level` state.
- [Medium] The internal static function `bpf_program__set_dynamicload` incorrectly uses the `libbpf_err()` wrapper.
- [Medium] The public API `bpf_program__load_dynamically` dereferences `prog` before verifying it is not NULL.
--

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

libbpf: BPF programs dynamic loading and attaching

BPF programs designated as dynamically loaded can be loaded and
attached independently after the initial bpf_object loading and
attaching. This patch enables these programs to be reloaded and
reattached multiple times, reducing load times for utilities with
many conditionally needed programs.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 821d854d43a9f..c3fe1de432108 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
[ ... ]
> @@ -15220,12 +15239,73 @@ void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s)
>  	free(s);
>  }
>  
> +static int bpf_program__set_dynamicload(struct bpf_program *prog)
> +{
> +	struct bpf_object *obj;
> +	const char *attach_name;
> +
> +	obj = prog->obj;
> +	if (!obj)
> +		return libbpf_err(-EINVAL);

[Severity: Medium]
Is this internal static function improperly invoking the public API error
wrapper libbpf_err() instead of returning a negative error code? Because the
internal function wraps the error, errno is set prematurely, and the public
API returns an unwrapped error result.

[ ... ]
> +/*
> + * This function must be called after bpf_object__load_progs.
> + * Dynamically-loaded program data is initialized on object load.
> + * Post-load initialization is not supported.
> + */
> +int
> +bpf_program__load_dynamically(struct bpf_program *prog, int extra_log_level)
> +{
> +	int err;
> +	struct bpf_object *obj;
> +
> +	obj = prog->obj;

[Severity: Medium]
Can this cause a segmentation fault by dereferencing prog before verifying it
is not NULL? Expected behavior for libbpf public APIs is to handle NULL
arguments gracefully (as done in bpf_program__unload_dynamically).

> +	if (!obj || obj->state < OBJ_LOADED)
> +		return libbpf_err(-EINVAL);
> +
> +	if (prog_is_subprog(obj, prog) || prog->load_type != BPF_PROG_LOAD_TYPE_DYNAMIC)
> +		return libbpf_err(-EINVAL);
> +
> +	prog->log_level |= extra_log_level;
> +
> +	err = bpf_object_load_prog(obj, prog, prog->insns, prog->insns_cnt,
> +				   obj->license, obj->kern_version, &prog->fd);

[Severity: High]
Could this lead to a file descriptor leak if the program is already loaded?
The call appears to unconditionally overwrite the prog->fd memory location
without closing the previous file descriptor.

> +	if (err) {
> +		pr_warn("prog '%s': failed to dynamically load: %d\n", prog->name, err);
> +		prog->log_level &= ~extra_log_level;
> +		return err;

[Severity: Medium]
Are we missing a libbpf_err() wrapper here? This return path directly exposes
a raw negative error to the caller, which may leave errno improperly set.

> +	}
> +
> +	prog->log_level &= ~extra_log_level;

[Severity: Medium]
Will this bitwise operation permanently corrupt the user-configured log
level? If the user previously set bits in prog->log_level that overlap with
extra_log_level, those original bits would be incorrectly cleared here
instead of being restored to their previous state.

> +	return 0;
> +}

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