Re: [RFC PATCH bpf-next 04/12] bpf: Remove unnecessary prog_list_prog() check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] replace_effective_prog() incorrectly uses absolute indexing into the bpf_prog_array, failing to account for dummy programs left by failed detach operations.
--

commit 77c3b439749a112ce3763d2ced5be03b93639458
Author: Martin KaFai Lau <martin.lau@kernel.org>

bpf: Remove unnecessary prog_list_prog() check

This patch removes the !prog_list_prog(pl) checks in replace_effective_prog()
and purge_effective_progs(). It cleans up conditions that are not expected
to happen, serving as preparation for attaching struct_ops to cgroups.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 876f6a81a9b61..71591f6dc4581 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -946,8 +946,6 @@ static void replace_effective_prog(struct cgroup *cgrp,
>  
>  			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++;

[Severity: Critical]
This is a pre-existing issue, but does using pos as an absolute index here
cause problems if a prior detach operation failed?

When __cgroup_bpf_detach() fails to allocate a new array via
update_effective_progs(), it falls back to purge_effective_progs().
This fallback uses bpf_prog_array_delete_safe_at() to replace the detached
program with a dummy program in the active array, and then removes the
detached program's node from the cgroup's hlist.

Later in replace_effective_prog(), we iterate the hlist to calculate pos.
Because the detached program is no longer in the hlist, pos represents the
logical index of the program (skipping the dummy program).

However, looking at what happens immediately after the found label:

found:
        BUG_ON(!cg);
        progs = rcu_dereference_protected(
                desc->bpf.effective[atype],
                lockdep_is_held(&cgroup_mutex));
        item = &progs->items[pos];
        WRITE_ONCE(item->prog, link->link.prog);

The code uses pos as an absolute index into the array (progs->items[pos]).
Because the array still contains the dummy program, the logical index
calculated from the hlist no longer matches the absolute index in the array.

Could this cause the wrong item to be overwritten, leaving the original
program pointer in the active effective array? Since the caller (like
__cgroup_bpf_replace()) drops the reference count of the replaced program,
would this directly lead to a use-after-free when the cgroup BPF hooks are
subsequently executed?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519215841.2984970-1-martin.lau@linux.dev?part=4