Re: [PATCH] bpf: Fix preorder-unaware index in replace_effective_prog and purge_effective_progs replace_effective_prog() and purge_effective_progs() compute the target index in the effective program array by sequentially walking the hlist (pos++). However, since commit 4b82b181a26c ("bpf: Allow pre-ordering for bpf cgroup progs"), compute_effective_progs() partitions the effective array: preorder programs occupy indices 0..preorder_cnt-1 and normal programs occupy indices preorder_cnt..end. The sequential hlist walk does not account for this partitioning, so when BPF_F_PREORDER programs coexist with normal programs the computed index diverges from the actual position in the effective array. In replace_effective_prog() this causes BPF_LINK_UPDATE to overwrite the wrong slot: the freed program pointer remains in the correct slot while an unrelated neighbor is clobbered. Fix both functions by searching the effective array directly for the target program pointer instead of computing an index from the hlist. This is correct regardless of array layout since it matches by pointer identity. For purge_effective_progs(), resolve the effective prog from the link when the direct prog pointer is NULL (link-based detach path). Fixes: 4b82b181a26c ("bpf: Allow pre-ordering for bpf cgroup progs") Signed-off-by: Himanshu Anand <anand.himanshu17@gmail.com>

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Replacing or purging BPF programs by direct pointer comparison in the effective array incorrectly targets the wrong program when the same `bpf_prog` is attached multiple times.
- [High] `purge_effective_progs()` passes an absolute array index to `bpf_prog_array_delete_safe_at()`, which requires a logical index.
- [Low] Multi-line comments do not follow the mandatory BPF subsystem comment style.
--

commit 3ec87c2281e918de9e836230ea5c1ffd6604e516
Author: Himanshu Anand <anand.himanshu17@gmail.com>

bpf: Fix preorder-unaware index in replace_effective_prog and purge_effective_progs

This patch modifies replace_effective_prog() and purge_effective_progs() to
use direct pointer comparison instead of sequential index computation when
locating target BPF programs in the effective array. This is done to correctly
handle the partitioning of preorder and normal programs introduced in a
previous commit.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 876f6a81a9b61..437e1e873bf8e 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -923,15 +923,12 @@ static int cgroup_bpf_attach(struct cgroup *cgrp,

[ ... ]

> -found:
> -		BUG_ON(!cg);
> +		/* find the old prog in effective array by direct pointer
> +		 * comparison, since compute_effective_progs() partitions
> +		 * the array (preorder progs first) and the hlist walk
> +		 * order does not match the effective array order.
> +		 */

[Severity: Low]
This isn't a bug, but multi-line comments in the BPF subsystem should have the
opening /* on its own line.

>  		progs = rcu_dereference_protected(
>  				desc->bpf.effective[atype],
>  				lockdep_is_held(&cgroup_mutex));
> -		item = &progs->items[pos];
> -		WRITE_ONCE(item->prog, link->link.prog);
> +		for (item = &progs->items[0]; item->prog; item++) {
> +			if (item->prog == old_prog) {
> +				WRITE_ONCE(item->prog, link->link.prog);
> +				break;
> +			}
> +		}

[Severity: High]
Can this overwrite the wrong program slot if the same BPF program is attached
multiple times in the cgroup hierarchy?

The BPF link API allows the same program to be attached multiple times (since
find_attach_entry() skips duplicate checks for link-based attachments), which
means item->prog could have duplicate pointers in the effective array.

A linear search might match the first occurrence belonging to an entirely
different link, associating the remaining programs with the wrong
cgroup_storage or bpf_cookie.

>  	}
>  }

[ ... ]

> @@ -1078,11 +1068,9 @@ static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
>  				  struct bpf_cgroup_link *link,
>  				  enum cgroup_bpf_attach_type atype)
>  {

[ ... ]

> -		/* no link or prog match, skip the cgroup of this layer */
> -		continue;
> -found:
> +		/* find the prog in the effective array by direct pointer
> +		 * comparison instead of computing index from hlist walk,
> +		 * since compute_effective_progs() partitions the array.
> +		 * Use target_prog which resolves link->link.prog for
> +		 * link-based entries where prog is NULL.
> +		 */

[Severity: Low]
This isn't a bug, but multi-line comments in the BPF subsystem should have the
opening /* on its own line.

>  		progs = rcu_dereference_protected(
>  				desc->bpf.effective[atype],
>  				lockdep_is_held(&cgroup_mutex));
>  
> +		for (pos = 0; progs->items[pos].prog; pos++) {
> +			if (progs->items[pos].prog == target_prog)
> +				break;
> +		}
> +
> +		if (!progs->items[pos].prog)
> +			continue;
> +
>  		/* Remove the program from the array */
>  		WARN_ONCE(bpf_prog_array_delete_safe_at(progs, pos),
>  			  "Failed to purge a prog from array at index %d", pos);

[Severity: High]
Does this pass an absolute index to bpf_prog_array_delete_safe_at() when it
expects a logical index?

The loop computes pos as an absolute index by walking progs->items, including
any dummy programs. However, bpf_prog_array_delete_safe_at() passes the index
to bpf_prog_array_update_at(), which explicitly skips dummy programs, treating
the index as logical.

If the array contains dummy programs from previous fallback purges, could the
absolute index exceed the logical index and cause bpf_prog_array_update_at()
to overshoot the target and return -ENOENT?

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