Re: [PATCH bpf-next v1 2/2] bpf: Fix is_tracing_prog_type() to cover fentry/fexit/fmod_ret

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Tracing programs can bypass the bpf_spin_lock restriction using bpf_map_update_elem() with BPF_F_LOCK, leading to per-CPU IRQ state corruption.
- [Medium] EXT programs extending iterators are incorrectly classified as tracing programs, restricting them from using spin locks, lists, and rbtrees.

Pre-existing issues:
- [High] Kernel warning and memory leak in `bpf_obj_free_fields` when freeing map elements containing lists or rbtrees protected by `bpf_res_spin_lock`.
--

--- Patch [2]: [PATCH bpf-next v1 2/2] bpf: Fix is_tracing_prog_type() to cover fentry/fexit/fmod_ret ---
commit 94d71df738bb27d6ad27a15a0ce402fd7032420d
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Fix is_tracing_prog_type() to cover fentry/fexit/fmod_ret

This patch tightens the tracing program type checks and delays the evaluation
of map spin lock and list restrictions to the point of use rather than during
map compatibility checks.

> Fix this by tightening the check, and moving predicates checking for the
> restriction closer to the point of use. This change might affect
> existing programs using them in unsafe contexts, but in such cases, it
> is recommended for users to switch over to bpf_res_spin_lock instead.

[Severity: High]
This is a pre-existing issue, but recommending users switch to
bpf_res_spin_lock exposes an issue in bpf_obj_free_fields() when freeing
map elements containing lists or rbtrees.

When btf_parse_fields() encounters BPF_RES_SPIN_LOCK, it sets
res_spin_lock_off but leaves spin_lock_off as -EINVAL:

kernel/bpf/btf.c:btf_parse_fields() {
    ...
    rec->spin_lock_off = -EINVAL;
    rec->res_spin_lock_off = -EINVAL;
    ...
}

Later, bpf_obj_free_fields() unconditionally checks spin_lock_off:

kernel/bpf/syscall.c:bpf_obj_free_fields() {
    ...
    if (WARN_ON_ONCE(rec->spin_lock_off < 0))
        continue;
    ...
}

Could this trigger a WARN_ON_ONCE and leak memory for list/rbtree nodes
because the check skips the freeing logic when a bpf_res_spin_lock is
used instead of a standard bpf_spin_lock?


> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17769,9 +17780,11 @@ static int check_pseudo_btf_id(struct bpf_verifier_env *env,
[ ... ]
> -static bool is_tracing_prog_type(enum bpf_prog_type type)
> +static bool is_tracing_prog_type(struct bpf_prog *prog)
>  {
> -	switch (type) {
> +	switch (resolve_prog_type(prog)) {
> +	case BPF_PROG_TYPE_TRACING:
> +		return prog->expected_attach_type != BPF_TRACE_ITER;

[Severity: Medium]
Does this check correctly handle extension programs (BPF_PROG_TYPE_EXT)?

For an extension program, the resolved type is BPF_PROG_TYPE_TRACING, but
its expected_attach_type is its own attach type (typically 0), not the
target's attach type which is stored in saved_dst_attach_type.

Could this cause valid extension programs that extend iterators to be
erroneously rejected by the verifier when trying to use spin locks, lists,
or rbtrees?


> @@ -17802,14 +17815,6 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
[ ... ]
> -	if (btf_record_has_field(map->record, BPF_SPIN_LOCK)) {
> -		if (is_tracing_prog_type(prog_type)) {
> -			verbose(env, "tracing progs cannot use bpf_spin_lock yet\n");
> -			return -EINVAL;
> -		}
> -	}
> +	/*
> +	 * Restrictions on using BPF list,rbtree,spin_lock is checked later upon
> +	 * use, since rejecting accesing map containing them in programs is too
> +	 * conservative.
> +	 */

[Severity: High]
Does removing this map-level check allow tracing programs to bypass the
lock restriction using bpf_map_update_elem() with the BPF_F_LOCK flag?

While the explicit lock helper calls are now blocked in process_spin_lock(),
the BPF_F_LOCK flag bypasses those verifier checks and executes
copy_map_value_locked():

kernel/bpf/helpers.c:copy_map_value_locked() {
    ...
    __bpf_spin_lock_irqsave(lock);
    ...
}

Can this corrupt the irqsave_flags per-CPU variable when tracing programs
execute in NMI or arbitrary IRQ contexts, potentially leading to deadlocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719113551.1294284-1-memxor@gmail.com?part=2