Re: [PATCH bpf v1 1/2] bpf: Fix effective prog replacement with BPF_F_PREORDER

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Incomplete fix leaves `purge_effective_progs()` vulnerable to an incorrect array index calculation, leading to a Use-After-Free.
--

commit 66001d04979b50f447c6655c1c7db45dc464ee71
Author: Amery Hung <ameryhung@gmail.com>

bpf: Fix effective prog replacement with BPF_F_PREORDER

The patch fixes replace_effective_prog() to handle BPF_F_PREORDER array
indexing. By introducing effective_prog_pos(), it properly replays
compute_effective_progs()'s placement instead of relying on a linear count.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 83ce66296ac19..b12e44ec5d8cb 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c

[ ... ]

> @@ -960,22 +1006,10 @@ static void replace_effective_prog(struct cgroup *cgrp,
>  		if (percpu_ref_is_zero(&desc->bpf.refcnt))
>  			continue;
>  
> -		/* find position of link in effective progs array */
> -		for (pos = 0, cg = desc; cg; cg = cgroup_parent(cg)) {
> -			if (pos && !(cg->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
> -				continue;
> -
> -			head = &cg->bpf.progs[atype];
> -			hlist_for_each_entry(pl, head, node) {
> -				if (!prog_list_prog(pl))
> -					continue;
> -				if (pl->link == link)
> -					goto found;
> -				pos++;
> -			}
> -		}
> -found:
> -		BUG_ON(!cg);
>  		pos = effective_prog_pos(desc, atype, link);
>  		if (WARN_ON_ONCE(pos < 0))
>  			continue;

[Severity: Critical]
This is a pre-existing issue, but does purge_effective_progs() still
compute the array index using the naive linear increment that was removed
here?

Looking at purge_effective_progs() in kernel/bpf/cgroup.c:

	/* find position of link or prog in effective progs array */
	for (pos = 0, cg = desc; cg; cg = cgroup_parent(cg)) {
		if (pos && !(cg->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
			continue;

		head = &cg->bpf.progs[atype];
		hlist_for_each_entry(pl, head, node) {
			if (!prog_list_prog(pl))
				continue;
			if (pl->prog == prog && pl->link == link)
				goto found;
			pos++;
		}
	}

Since purge_effective_progs() acts as a fallback when
update_effective_progs() fails, could this incorrect array index calculation
lead to a use-after-free?

If bpf_prog_array_delete_safe_at(progs, pos) overwrites the wrong program
slot, __cgroup_bpf_detach() might later call bpf_prog_put() on a program
that is still active in the effective array.

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