Re: [PATCH bpf v2 1/2] bpf: Fix effective prog array index with BPF_F_PREORDER

[Yonghong Song <yonghong.song@linux.dev>]

On 6/18/26 11:46 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] This problem wasn't introduced by this patch, but replace_effective_prog() erroneously uses the logical index returned by effective_prog_pos() as a direct physical index into the effective BPF program array. If previous detach operations failed to reallocate the array and left dummy_bpf_prog elements in place, the physical index does not match the logical index. This causes bpf_link_update() to overwrite the wrong program slot (e.g., the dummy) leaving the original program untouched in the active array while its reference is dropped by __cgroup_bpf_replace(). Once the RCU grace period expires, this leads to a Use-After-Free when the kernel executes the freed program.
> --
>
> commit f837441f68b1fdb520696422a141ec421e2d50ae
> Author: Amery Hung <ameryhung@gmail.com>
>
> bpf: Fix effective prog array index with BPF_F_PREORDER
>
> This commit introduces a shared helper effective_prog_pos() to accurately
> determine a program's position in the effective array by replaying the
> placement logic. This fixes an issue where preorder programs caused
> replace_effective_prog() and purge_effective_progs() to access incorrect
> slots, corrupting the array or causing a use-after-free.
>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 83ce66296ac19..4355ccb78a9c6 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
> [ ... ]
>
>> @@ -960,27 +1006,15 @@ 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, pl);
>> +		if (WARN_ON_ONCE(pos < 0))
>> +			continue;
>>   
>>   		progs = rcu_dereference_protected(
>>   				desc->bpf.effective[atype],
>>   				lockdep_is_held(&cgroup_mutex));
>>   		item = &progs->items[pos];
>> -		WRITE_ONCE(item->prog, link->link.prog);
>> +		WRITE_ONCE(item->prog, pl->link->link.prog);
> [Severity: Critical]
> This isn't a bug introduced by this patch, but does replace_effective_prog()
> erroneously use the logical index returned by effective_prog_pos() as a direct
> physical index into the effective BPF program array?
>
> If previous detach operations failed to reallocate the array and left
> dummy_bpf_prog elements in place, the physical index would not match the
> logical index. This could cause bpf_link_update() to overwrite the wrong
> program slot (e.g., the dummy), leaving the original program untouched in the
> active array while its reference is dropped by __cgroup_bpf_replace().
>
> Once the RCU grace period expires, could this lead to a Use-After-Free when
> the kernel executes the freed program? Should this instead use
> bpf_prog_array_update_at(progs, pos, pl->link->link.prog) to correctly handle
> the presence of dummy programs?

Indeed this is a prefixing bug and can be fixed in another patch. If I undersand
correctly, the logic like below:
    __cgroup_bpf_detach
      update_effective_progs
        failed doe to out-of-memory:
           purge_effective_progs
               ...
               bpf_prog_array_delete_safe_at(...)
               /* dummy prog is in the effective array */
    __cgroup_bpf_replace
      replace_effective_prog
        ...
        item = &progs->items[pos];
        WRITE_ONCE(item->prog, pl->link->link.prog);
        /* dummy prog in effective array, but pos may ignore dummy prog during accouting. */

So for in replace_effective_prog(), bpf_prog_array_update_at() is the right way to fix.