All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf v2 0/2] Fix effective prog array indexing with BPF_F_PREORDER
@ 2026-06-19  6:35 Amery Hung
  2026-06-19  6:35 ` [PATCH bpf v2 1/2] bpf: Fix effective prog array index " Amery Hung
  2026-06-19  6:35 ` [PATCH bpf v2 2/2] selftests/bpf: Test cgroup link replace " Amery Hung
  0 siblings, 2 replies; 7+ messages in thread
From: Amery Hung @ 2026-06-19  6:35 UTC (permalink / raw)
  To: bpf
  Cc: alexei.starovoitov, andrii, daniel, eddyz87, memxor, martin.lau,
	ameryhung, kernel-team

Hi,

This patchset fixes a cgroup effective-array indexing bug where
replace_effective_prog() and purge_effective_progs() used a linear hlist
position that doesn't match the array layout when BPF_F_PREORDER
programs are present, corrupting the array on link update and risking a
use-after-free in the detach fallback. It computes the slot via a shared
effective_prog_pos() helper and adds a cgroup_preorder selftest. 


Changelog

v1 -> v2:
 - Also fix purge_effective_progs(), in addition to
   replace_effective_prog() (Sashiko).
 - selftest: Set err on bpf_link_create() failure so the failure is
   reported to the caller (Sashiko).

---

Amery Hung (2):
  bpf: Fix effective prog array index with BPF_F_PREORDER
  selftests/bpf: Test cgroup link replace with BPF_F_PREORDER

 kernel/bpf/cgroup.c                           | 108 ++++++++++--------
 .../bpf/prog_tests/cgroup_preorder.c          |  77 +++++++++++++
 2 files changed, 139 insertions(+), 46 deletions(-)

-- 
2.53.0-Meta


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH bpf v2 1/2] bpf: Fix effective prog array index with BPF_F_PREORDER
  2026-06-19  6:35 [PATCH bpf v2 0/2] Fix effective prog array indexing with BPF_F_PREORDER Amery Hung
@ 2026-06-19  6:35 ` Amery Hung
  2026-06-19  6:46   ` sashiko-bot
  2026-06-20 20:45   ` Yonghong Song
  2026-06-19  6:35 ` [PATCH bpf v2 2/2] selftests/bpf: Test cgroup link replace " Amery Hung
  1 sibling, 2 replies; 7+ messages in thread
From: Amery Hung @ 2026-06-19  6:35 UTC (permalink / raw)
  To: bpf
  Cc: alexei.starovoitov, andrii, daniel, eddyz87, memxor, martin.lau,
	ameryhung, kernel-team

replace_effective_prog() and purge_effective_progs() located the slot in
the effective array by walking the program hlist and counting entries
linearly. That count does not match the array layout: compute_effective_
progs() places BPF_F_PREORDER programs at the front (ancestor cgroup
first, attach order within a cgroup) and the rest after them (descendant
cgroup first). So when a preorder program is present, the linear hlist
position no longer equals the program's index in the effective array.

For replace_effective_prog() (bpf_link_update()) this overwrote the
wrong slot, corrupting the effective order. For purge_effective_progs(),
it could dummy out a slot belonging to a different program and leave the
detached program in the array while bpf_prog_put() drops its reference,
i.e. a use-after-free.

Fix both by replaying compute_effective_progs()'s placement (including
the per-cgroup preorder reversal) in a shared effective_prog_pos()
helper. Identify the entry by its struct bpf_prog_list pointer rather
than by (prog, link) value, so the lookup resolves to exactly the
attachment the syscall selected even when the same bpf_prog is attached
to several cgroups in the hierarchy.

Fixes: 4b82b181a26c ("bpf: Allow pre-ordering for bpf cgroup progs")
Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 kernel/bpf/cgroup.c | 108 +++++++++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 46 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 83ce66296ac1..4355ccb78a9c 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -939,19 +939,65 @@ static int cgroup_bpf_attach(struct cgroup *cgrp,
 	return ret;
 }
 
+static int effective_prog_pos(struct cgroup *cgrp,
+			      enum cgroup_bpf_attach_type atype,
+			      struct bpf_prog_list *target_pl)
+{
+	int cnt = 0, preorder_cnt = 0, fstart, bstart, init_bstart, pos = -1;
+	struct bpf_prog_list *pl;
+	struct cgroup *p = cgrp;
+
+	/* count effective programs to find where the preorder region ends */
+	do {
+		if (cnt == 0 || (p->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
+			cnt += prog_list_length(&p->bpf.progs[atype], &preorder_cnt);
+		p = cgroup_parent(p);
+	} while (p);
+
+	/* replay compute_effective_progs() placement and record target's slot */
+	cnt = 0;
+	p = cgrp;
+	fstart = preorder_cnt;
+	bstart = preorder_cnt - 1;
+	do {
+		if (cnt > 0 && !(p->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
+			continue;
+
+		init_bstart = bstart;
+		hlist_for_each_entry(pl, &p->bpf.progs[atype], node) {
+			if (!prog_list_prog(pl))
+				continue;
+
+			if (pl->flags & BPF_F_PREORDER) {
+				if (pl == target_pl)
+					pos = bstart;
+				bstart--;
+			} else {
+				if (pl == target_pl)
+					pos = fstart;
+				fstart++;
+			}
+			cnt++;
+		}
+
+		/* reverse pre-ordering progs at this cgroup level */
+		if (pos >= bstart + 1 && pos <= init_bstart)
+			pos = bstart + 1 + init_bstart - pos;
+	} while ((p = cgroup_parent(p)));
+
+	return pos;
+}
+
 /* Swap updated BPF program for given link in effective program arrays across
  * all descendant cgroups. This function is guaranteed to succeed.
  */
 static void replace_effective_prog(struct cgroup *cgrp,
 				   enum cgroup_bpf_attach_type atype,
-				   struct bpf_cgroup_link *link)
+				   struct bpf_prog_list *pl)
 {
 	struct bpf_prog_array_item *item;
 	struct cgroup_subsys_state *css;
 	struct bpf_prog_array *progs;
-	struct bpf_prog_list *pl;
-	struct hlist_head *head;
-	struct cgroup *cg;
 	int pos;
 
 	css_for_each_descendant_pre(css, &cgrp->self) {
@@ -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;
+		pos = effective_prog_pos(desc, atype, pl);
+		if (WARN_ON_ONCE(pos < 0))
+			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);
 		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);
 	}
 }
 
@@ -1024,7 +1058,7 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
 
 	cgrp->bpf.revisions[atype] += 1;
 	old_prog = xchg(&link->link.prog, new_prog);
-	replace_effective_prog(cgrp, atype, link);
+	replace_effective_prog(cgrp, atype, pl);
 	bpf_prog_put(old_prog);
 	return 0;
 }
@@ -1091,19 +1125,14 @@ static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
  *                           recomputing the array in place.
  *
  * @cgrp: The cgroup which descendants to travers
- * @prog: A program to detach or NULL
- * @link: A link to detach or NULL
+ * @pl: The prog_list entry being detached
  * @atype: Type of detach operation
  */
-static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
-				  struct bpf_cgroup_link *link,
+static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog_list *pl,
 				  enum cgroup_bpf_attach_type atype)
 {
 	struct cgroup_subsys_state *css;
 	struct bpf_prog_array *progs;
-	struct bpf_prog_list *pl;
-	struct hlist_head *head;
-	struct cgroup *cg;
 	int pos;
 
 	/* recompute effective prog array in place */
@@ -1113,24 +1142,11 @@ static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
 		if (percpu_ref_is_zero(&desc->bpf.refcnt))
 			continue;
 
-		/* 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++;
-			}
-		}
-
+		pos = effective_prog_pos(desc, atype, pl);
 		/* no link or prog match, skip the cgroup of this layer */
-		continue;
-found:
+		if (pos < 0)
+			continue;
+
 		progs = rcu_dereference_protected(
 				desc->bpf.effective[atype],
 				lockdep_is_held(&cgroup_mutex));
@@ -1196,7 +1212,7 @@ static int __cgroup_bpf_detach(struct cgroup *cgrp, struct bpf_prog *prog,
 		/* if update effective array failed replace the prog with a dummy prog*/
 		pl->prog = old_prog;
 		pl->link = link;
-		purge_effective_progs(cgrp, old_prog, link, atype);
+		purge_effective_progs(cgrp, pl, atype);
 	}
 
 	/* now can actually delete it from this cgroup list */
-- 
2.53.0-Meta


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bpf v2 2/2] selftests/bpf: Test cgroup link replace with BPF_F_PREORDER
  2026-06-19  6:35 [PATCH bpf v2 0/2] Fix effective prog array indexing with BPF_F_PREORDER Amery Hung
  2026-06-19  6:35 ` [PATCH bpf v2 1/2] bpf: Fix effective prog array index " Amery Hung
@ 2026-06-19  6:35 ` Amery Hung
  2026-06-20 20:46   ` Yonghong Song
  1 sibling, 1 reply; 7+ messages in thread
From: Amery Hung @ 2026-06-19  6:35 UTC (permalink / raw)
  To: bpf
  Cc: alexei.starovoitov, andrii, daniel, eddyz87, memxor, martin.lau,
	ameryhung, kernel-team

Add a cgroup_preorder case that attaches a normal and a BPF_F_PREORDER
program to a cgroup (effective order [2, 1]), then replaces the normal
link's program via bpf_link_update() and checks the effective order
becomes [2, 3] — i.e. only the non-preorder slot changes. Without the
replace_effective_prog() fix the array is corrupted and the order is
wrong.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 .../bpf/prog_tests/cgroup_preorder.c          | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c b/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c
index d4d583872fa2..d2ccf409dfe3 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_preorder.c
@@ -102,6 +102,82 @@ static int run_getsockopt_test(int cg_parent, int cg_child, int sock_fd, bool al
 	return err;
 }
 
+/*
+ * Replacing a link's program (bpf_link_update) must target the correct slot in
+ * the effective array even when a BPF_F_PREORDER program is attached to the
+ * same cgroup. All programs here are attached to a single cgroup; "parent" is
+ * reused only as a third distinct program.
+ *
+ * Attach child(1) normally and child_2(2) with BPF_F_PREORDER, so the effective
+ * order is [2, 1]. Then replace child(1)'s program with parent(3): only the
+ * non-preorder slot changes, giving [2, 3].
+ */
+static int run_link_replace_test(int cgroup_fd, int sock_fd)
+{
+	LIBBPF_OPTS(bpf_link_create_opts, create_opts);
+	int err = 0, normal_link = -1, preorder_link = -1;
+	struct cgroup_preorder *skel = NULL;
+	enum bpf_attach_type atype;
+	__u8 *result, buf = 0x00;
+	socklen_t optlen = 1;
+
+	skel = cgroup_preorder__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "cgroup_preorder__open_and_load"))
+		return -1;
+
+	err = setsockopt(sock_fd, SOL_IP, IP_TOS, &buf, 1);
+	if (!ASSERT_OK(err, "setsockopt"))
+		goto close_skel;
+
+	atype = bpf_program__expected_attach_type(skel->progs.child);
+
+	create_opts.flags = 0;
+	normal_link = bpf_link_create(bpf_program__fd(skel->progs.child),
+				      cgroup_fd, atype, &create_opts);
+	if (!ASSERT_GE(normal_link, 0, "create_normal_link")) {
+		err = normal_link;
+		goto close_skel;
+	}
+
+	create_opts.flags = BPF_F_PREORDER;
+	preorder_link = bpf_link_create(bpf_program__fd(skel->progs.child_2),
+					cgroup_fd, atype, &create_opts);
+	if (!ASSERT_GE(preorder_link, 0, "create_preorder_link")) {
+		err = preorder_link;
+		goto close_links;
+	}
+
+	result = skel->bss->result;
+	skel->bss->idx = 0;
+	memset(result, 0, 4);
+
+	err = getsockopt(sock_fd, SOL_IP, IP_TOS, &buf, &optlen);
+	if (!ASSERT_OK(err, "getsockopt-before"))
+		goto close_links;
+	ASSERT_TRUE(result[0] == 2 && result[1] == 1, "order before update");
+
+	/* Replace the normal link's program child(1) -> parent(3). */
+	err = bpf_link_update(normal_link, bpf_program__fd(skel->progs.parent), NULL);
+	if (!ASSERT_OK(err, "bpf_link_update"))
+		goto close_links;
+
+	skel->bss->idx = 0;
+	memset(result, 0, 4);
+
+	err = getsockopt(sock_fd, SOL_IP, IP_TOS, &buf, &optlen);
+	if (!ASSERT_OK(err, "getsockopt-after"))
+		goto close_links;
+	ASSERT_TRUE(result[0] == 2 && result[1] == 3, "order after update");
+
+close_links:
+	if (preorder_link >= 0)
+		close(preorder_link);
+	close(normal_link);
+close_skel:
+	cgroup_preorder__destroy(skel);
+	return err;
+}
+
 void test_cgroup_preorder(void)
 {
 	int cg_parent = -1, cg_child = -1, sock_fd = -1;
@@ -120,6 +196,7 @@ void test_cgroup_preorder(void)
 
 	ASSERT_OK(run_getsockopt_test(cg_parent, cg_child, sock_fd, false), "getsockopt_test_1");
 	ASSERT_OK(run_getsockopt_test(cg_parent, cg_child, sock_fd, true), "getsockopt_test_2");
+	ASSERT_OK(run_link_replace_test(cg_child, sock_fd), "link_replace_test");
 
 out:
 	close(sock_fd);
-- 
2.53.0-Meta


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v2 1/2] bpf: Fix effective prog array index with BPF_F_PREORDER
  2026-06-19  6:35 ` [PATCH bpf v2 1/2] bpf: Fix effective prog array index " Amery Hung
@ 2026-06-19  6:46   ` sashiko-bot
  2026-06-20 20:54     ` Yonghong Song
  2026-06-20 20:45   ` Yonghong Song
  1 sibling, 1 reply; 7+ messages in thread
From: sashiko-bot @ 2026-06-19  6:46 UTC (permalink / raw)
  To: Amery Hung; +Cc: bpf

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?

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v2 1/2] bpf: Fix effective prog array index with BPF_F_PREORDER
  2026-06-19  6:35 ` [PATCH bpf v2 1/2] bpf: Fix effective prog array index " Amery Hung
  2026-06-19  6:46   ` sashiko-bot
@ 2026-06-20 20:45   ` Yonghong Song
  1 sibling, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2026-06-20 20:45 UTC (permalink / raw)
  To: Amery Hung, bpf
  Cc: alexei.starovoitov, andrii, daniel, eddyz87, memxor, martin.lau,
	kernel-team



On 6/18/26 11:35 PM, Amery Hung wrote:
> replace_effective_prog() and purge_effective_progs() located the slot in
> the effective array by walking the program hlist and counting entries
> linearly. That count does not match the array layout: compute_effective_
> progs() places BPF_F_PREORDER programs at the front (ancestor cgroup
> first, attach order within a cgroup) and the rest after them (descendant
> cgroup first). So when a preorder program is present, the linear hlist
> position no longer equals the program's index in the effective array.
>
> For replace_effective_prog() (bpf_link_update()) this overwrote the
> wrong slot, corrupting the effective order. For purge_effective_progs(),
> it could dummy out a slot belonging to a different program and leave the
> detached program in the array while bpf_prog_put() drops its reference,
> i.e. a use-after-free.
>
> Fix both by replaying compute_effective_progs()'s placement (including
> the per-cgroup preorder reversal) in a shared effective_prog_pos()
> helper. Identify the entry by its struct bpf_prog_list pointer rather
> than by (prog, link) value, so the lookup resolves to exactly the
> attachment the syscall selected even when the same bpf_prog is attached
> to several cgroups in the hierarchy.
>
> Fixes: 4b82b181a26c ("bpf: Allow pre-ordering for bpf cgroup progs")
> Signed-off-by: Amery Hung <ameryhung@gmail.com>

LGTM with a nit below.

Acked-by: Yonghong Song <yonghong.song@linux.dev>

> ---
>   kernel/bpf/cgroup.c | 108 +++++++++++++++++++++++++-------------------
>   1 file changed, 62 insertions(+), 46 deletions(-)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 83ce66296ac1..4355ccb78a9c 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -939,19 +939,65 @@ static int cgroup_bpf_attach(struct cgroup *cgrp,
>   	return ret;
>   }
>   
> +static int effective_prog_pos(struct cgroup *cgrp,
> +			      enum cgroup_bpf_attach_type atype,
> +			      struct bpf_prog_list *target_pl)
> +{
> +	int cnt = 0, preorder_cnt = 0, fstart, bstart, init_bstart, pos = -1;
> +	struct bpf_prog_list *pl;
> +	struct cgroup *p = cgrp;
> +
> +	/* count effective programs to find where the preorder region ends */
> +	do {
> +		if (cnt == 0 || (p->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
> +			cnt += prog_list_length(&p->bpf.progs[atype], &preorder_cnt);
> +		p = cgroup_parent(p);
> +	} while (p);
> +
> +	/* replay compute_effective_progs() placement and record target's slot */
> +	cnt = 0;
> +	p = cgrp;
> +	fstart = preorder_cnt;
> +	bstart = preorder_cnt - 1;
> +	do {
> +		if (cnt > 0 && !(p->bpf.flags[atype] & BPF_F_ALLOW_MULTI))
> +			continue;
> +
> +		init_bstart = bstart;
> +		hlist_for_each_entry(pl, &p->bpf.progs[atype], node) {
> +			if (!prog_list_prog(pl))
> +				continue;
> +
> +			if (pl->flags & BPF_F_PREORDER) {
> +				if (pl == target_pl)
> +					pos = bstart;
> +				bstart--;
> +			} else {
> +				if (pl == target_pl)
> +					pos = fstart;
> +				fstart++;
> +			}
> +			cnt++;
> +		}
> +
> +		/* reverse pre-ordering progs at this cgroup level */
> +		if (pos >= bstart + 1 && pos <= init_bstart)
> +			pos = bstart + 1 + init_bstart - pos;

Here, if pos is not -1, we could break out of loop since each pl is unique.
But since this is not in a performance critical path, probably the current
implementation is okay.

> +	} while ((p = cgroup_parent(p)));
> +
> +	return pos;
> +}
> +
>   /* Swap updated BPF program for given link in effective program arrays across
>    * all descendant cgroups. This function is guaranteed to succeed.
>    */
>   static void replace_effective_prog(struct cgroup *cgrp,
>   				   enum cgroup_bpf_attach_type atype,
> -				   struct bpf_cgroup_link *link)
> +				   struct bpf_prog_list *pl)
>   {
>   	struct bpf_prog_array_item *item;
>   	struct cgroup_subsys_state *css;
>   	struct bpf_prog_array *progs;
> -	struct bpf_prog_list *pl;
> -	struct hlist_head *head;
> -	struct cgroup *cg;
>   	int pos;
>   
>   	css_for_each_descendant_pre(css, &cgrp->self) {
> @@ -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;
> +		pos = effective_prog_pos(desc, atype, pl);
> +		if (WARN_ON_ONCE(pos < 0))
> +			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);
>   		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);
>   	}
>   }
>   
> @@ -1024,7 +1058,7 @@ static int __cgroup_bpf_replace(struct cgroup *cgrp,
>   
>   	cgrp->bpf.revisions[atype] += 1;
>   	old_prog = xchg(&link->link.prog, new_prog);
> -	replace_effective_prog(cgrp, atype, link);
> +	replace_effective_prog(cgrp, atype, pl);
>   	bpf_prog_put(old_prog);
>   	return 0;
>   }
> @@ -1091,19 +1125,14 @@ static struct bpf_prog_list *find_detach_entry(struct hlist_head *progs,
>    *                           recomputing the array in place.
>    *
>    * @cgrp: The cgroup which descendants to travers
> - * @prog: A program to detach or NULL
> - * @link: A link to detach or NULL
> + * @pl: The prog_list entry being detached
>    * @atype: Type of detach operation
>    */
> -static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog *prog,
> -				  struct bpf_cgroup_link *link,
> +static void purge_effective_progs(struct cgroup *cgrp, struct bpf_prog_list *pl,
>   				  enum cgroup_bpf_attach_type atype)
>   {
>   	struct cgroup_subsys_state *css;
>   	struct bpf_prog_array *progs;
> -	struct bpf_prog_list *pl;
> -	struct hlist_head *head;
> -	struct cgroup *cg;
>   	int pos;
>   
>   	/* recompute effective prog array in place */

[...]


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v2 2/2] selftests/bpf: Test cgroup link replace with BPF_F_PREORDER
  2026-06-19  6:35 ` [PATCH bpf v2 2/2] selftests/bpf: Test cgroup link replace " Amery Hung
@ 2026-06-20 20:46   ` Yonghong Song
  0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2026-06-20 20:46 UTC (permalink / raw)
  To: Amery Hung, bpf
  Cc: alexei.starovoitov, andrii, daniel, eddyz87, memxor, martin.lau,
	kernel-team



On 6/18/26 11:35 PM, Amery Hung wrote:
> Add a cgroup_preorder case that attaches a normal and a BPF_F_PREORDER
> program to a cgroup (effective order [2, 1]), then replaces the normal
> link's program via bpf_link_update() and checks the effective order
> becomes [2, 3] — i.e. only the non-preorder slot changes. Without the
> replace_effective_prog() fix the array is corrupted and the order is
> wrong.
>
> Signed-off-by: Amery Hung <ameryhung@gmail.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v2 1/2] bpf: Fix effective prog array index with BPF_F_PREORDER
  2026-06-19  6:46   ` sashiko-bot
@ 2026-06-20 20:54     ` Yonghong Song
  0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2026-06-20 20:54 UTC (permalink / raw)
  To: sashiko-reviews, Amery Hung; +Cc: bpf



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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-06-20 20:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-19  6:35 [PATCH bpf v2 0/2] Fix effective prog array indexing with BPF_F_PREORDER Amery Hung
2026-06-19  6:35 ` [PATCH bpf v2 1/2] bpf: Fix effective prog array index " Amery Hung
2026-06-19  6:46   ` sashiko-bot
2026-06-20 20:54     ` Yonghong Song
2026-06-20 20:45   ` Yonghong Song
2026-06-19  6:35 ` [PATCH bpf v2 2/2] selftests/bpf: Test cgroup link replace " Amery Hung
2026-06-20 20:46   ` Yonghong Song

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.