Re: [PATCH bpf-next v2 4/5] libbpf: Support declarative dynamic load via BTF decl_tag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Applying __load_dynamic to programs requiring BTF target resolution causes object opening to unconditionally fail.
- [Medium] prog_has_dynload_tag() aborts object loading and falsely blames an unrelated program if any invalid use of __load_dynamic exists in the BTF.
- [Medium] The __load_dynamic BTF tag is silently ignored for programs with unrecognized or custom ELF section names.
- [Low] Multi-line comment for __load_dynamic in bpf_helpers.h violates the strict BPF subsystem comment style.
--

commit 2d3fcf27d117a0a3d873c0c1212bb77a4447fc91
Author: Andrey Grodzovsky <andrey.grodzovsky@crowdstrike.com>

libbpf: Support declarative dynamic load via BTF decl_tag

This commit adds a loadtype:dynamic BTF decl_tag, exposed as __load_dynamic
in bpf_helpers.h, letting a program declare itself dynamically-loaded in
its source instead of requiring an imperative call. The tag is checked
in the existing per-program loop in bpf_object_init_progs().

> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 9d160b5b9c0ee..52978f0e04539 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -218,6 +218,11 @@ enum libbpf_tristate {
>  #define __arg_untrusted __attribute((btf_decl_tag("arg:untrusted")))
>  #define __arg_arena __attribute((btf_decl_tag("arg:arena")))
>  
> +/* Mark a BPF program to be loaded dynamically instead of autoloaded,
> + * equivalent to bpf_program__set_load_type(prog, BPF_PROG_LOAD_TYPE_DYNAMIC).
> + */

[Severity: Low]
This isn't a bug, but does this multi-line comment style violate the BPF
subsystem rules? The BPF subsystem coding style guide requires multi-line
comments to have the opening `/*` on its own line.

> +#define __load_dynamic __attribute__((btf_decl_tag("loadtype:dynamic")))
> +
>  #ifndef ___bpf_concat
>  #define ___bpf_concat(a, b) a ## b
>  #endif
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index cd1d00c47841b..5d58dbf25234f 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
[ ... ]
> @@ -7156,6 +7158,50 @@ static int bpf_prog_assign_exc_cb(struct bpf_object *obj, struct bpf_program *pr
>  	return 0;
>  }
>  
> +#define DYNLOAD_DECL_TAG "loadtype:dynamic"
> +
> +/*
> + * Check whether a BPF program is annotated with the DYNLOAD_DECL_TAG BTF
> + * decl tag, requesting that it be loaded dynamically after the object load.
> + *
> + * Returns 1 if the tag is present, 0 if it is not, negative error otherwise.
> + */
> +static int prog_has_dynload_tag(struct bpf_object *obj, struct bpf_program *prog)
> +{
> +	int i, n;
> +
> +	if (!obj->btf)
> +		return 0;
> +
> +	n = btf__type_cnt(obj->btf);
> +	for (i = 1; i < n; i++) {
> +		const struct btf_type *tag_t, *fn_t;
> +
> +		tag_t = btf__type_by_id(obj->btf, i);
> +		if (!btf_is_decl_tag(tag_t) || btf_decl_tag(tag_t)->component_idx != -1)
> +			continue;
> +		if (strcmp(btf__str_by_offset(obj->btf, tag_t->name_off),
> +			   DYNLOAD_DECL_TAG) != 0)
> +			continue;
> +
> +		/* a decl tag's type field points at the entity it decorates */
> +		fn_t = btf__type_by_id(obj->btf, tag_t->type);
> +		if (!btf_is_func(fn_t) || btf_func_linkage(fn_t) != BTF_FUNC_GLOBAL) {
> +			pr_warn("prog '%s': " DYNLOAD_DECL_TAG
> +				" decl tag must be applied to a global function\n",
> +				prog->name);
> +			return -EINVAL;
> +		}
> +
> +		if (strcmp(btf__str_by_offset(obj->btf, fn_t->name_off), prog->name) != 0)
> +			continue;   /* tag belongs to a different program */

[Severity: Medium]
Will prog_has_dynload_tag() abort object initialization and falsely blame the
current program if an invalid use of the __load_dynamic tag exists elsewhere
in the BTF?

Because the function linkage check is performed before verifying if the tag
actually belongs to the current program being processed, it appears the first
program processed will trigger the failure.

> +
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
[ ... ]
> @@ -8467,6 +8516,22 @@ static int bpf_object_init_progs(struct bpf_object *obj, const struct bpf_object
>  				return err;
>  			}
>  		}

[Severity: Medium]
Is the __load_dynamic BTF tag silently ignored for programs with unrecognized
or custom ELF section names?

Looking slightly earlier in bpf_object_init_progs(), there is a check:

		prog->sec_def = find_sec_def(prog->sec_name);
		if (!prog->sec_def) {
			/* couldn't guess, but user might manually specify */
			pr_debug("prog '%s': unrecognized ELF section name '%s'\n",
				prog->name, prog->sec_name);
			continue;
		}

Because the tag parsing added here is placed after this continue statement,
it seems the declarative dynamic load feature will be bypassed for programs
with custom section names.

> +
> +		if (prog_is_subprog(obj, prog))
> +			continue;
> +
> +		err = prog_has_dynload_tag(obj, prog);
> +		if (err < 0)
> +			return err;
> +		if (err > 0) {
> +			err = bpf_program__set_load_type(prog, BPF_PROG_LOAD_TYPE_DYNAMIC);
> +			if (err) {
> +				pr_warn("prog '%s': failed to apply " DYNLOAD_DECL_TAG
> +					" decl tag: %s\n",
> +					prog->name, errstr(err));
> +				return err;
> +			}
> +		}
>  	}
>  
>  	return 0;

[Severity: High]
Does applying __load_dynamic to programs requiring BTF target resolution cause
object opening to unconditionally fail?

When bpf_object_init_progs() calls bpf_program__set_load_type(), which routes
through bpf_program__set_dynamicload(), it hits this check:

		long flags = prog->sec_def ?
			(long)prog->sec_def->cookie : SEC_ATTACH_BTF;
		if ((flags & SEC_ATTACH_BTF) && !prog->attach_btf_id)
			return libbpf_err(-EINVAL);

Since the declarative tag is processed during early object initialization, long
before attach_btf_id is resolved by libbpf or can be set by the user,
attach_btf_id is always zero here. Doesn't this cause the object open phase to
unconditionally abort with -EINVAL?

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