Re: [PATCH] bpf: Fix cgroup link update after a failed effective array rebuild

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] bpf: wrong slot in replace_effective_prog() after override
  OOM detach
- [Low] bpf: multi-line comment above bpf_cgroup_array_item_at()
  violates style
--

commit 417086c03fb15b10d49180c23398c0440bbc2a1b
Author: Shihuang Liu <shlomojune6@gmail.com>

bpf: Fix cgroup link update after a failed effective array rebuild

This commit introduces bpf_cgroup_array_item_at() to skip dummy entries
left by a failed effective array rebuild when looking up a program slot in
bpf_cgroup_array_delete_safe_at() and replace_effective_prog().

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 57a785241d42f..db3f173e83c69 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -403,6 +403,28 @@ static int bpf_cgroup_array_length(struct bpf_prog_array *array,
>  	return cnt;
>  }
>  
> +/* Return the entry at logical position @index in an effective program
> + * array.  Dummy entries left behind by a failed rebuild are skipped because
> + * they do not correspond to an entry in the program list.
> + */

[Severity: Low]
This isn't a bug, but BPF coding style places the opening /* of a
multi-line comment on its own line.

[ ... ]

> @@ -1175,7 +1193,9 @@ static void replace_effective_prog(struct cgroup *cgrp,
>  		progs = rcu_dereference_protected(
>  				desc->bpf.effective[atype],
>  				lockdep_is_held(&cgroup_mutex));
> -		item = &progs->items[pos];
> +		item = bpf_cgroup_array_item_at(progs, pos, atype);
> +		if (WARN_ON_ONCE(!item))
> +			continue;
>  		prog_list_replace_item(pl, item);
>  	}
>  }

[Severity: Medium]
Can bpf_cgroup_array_item_at() still resolve the wrong slot in
desc->bpf.effective[atype] when a descendant cgroup overrides an ancestor
BPF_F_ALLOW_OVERRIDE program and then detaches its last program via the
purge_effective_progs() fallback?

In a 3-level hierarchy P -> C -> GC where P has BPF_F_ALLOW_MULTI links,
C has a BPF_F_ALLOW_OVERRIDE program prog_C, and GC has an overriding
program prog_GC, GC's effective array contains [prog_GC, link_P1, ...] and
omits prog_C.  If detaching prog_GC fails to allocate in
update_effective_progs(), __cgroup_bpf_detach() replaces items[0] with a
dummy via purge_effective_progs() and removes prog_GC from
GC->bpf.progs[atype], leaving GC->bpf.effective[atype] as
[dummy, link_P1, ...].

When link_P1 is later updated, effective_prog_pos() sees an empty
GC->bpf.progs[atype] (cnt == 0), so it no longer skips C:

kernel/bpf/cgroup.c:effective_prog_pos() {
	...
	do {
		if (cnt > 0 && !(p->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
			continue;
	...
}

effective_prog_pos() counts prog_C at index 0 and returns pos = 1 for
link_P1.  bpf_cgroup_array_item_at(progs, 1, atype) then skips items[0]
(dummy) and treats items[1] (link_P1) as index 0, either overwriting the
next link at items[2] or hitting WARN_ON_ONCE(!item).  In either case,
items[1] retains old_prog after __cgroup_bpf_replace() drops its
reference, causing a use-after-free when GC's effective array runs.

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