Re: [PATCH bpf-next v3 09/15] bpf: Add infrastructure to support attaching struct_ops to cgroups

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] replace_effective_prog() computes a logical array index but uses it as a physical index, overwriting the wrong program.
--

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

bpf: Add infrastructure to support attaching struct_ops to cgroups

This patch adds the necessary infrastructure to attach a struct_ops map to a
cgroup. It creates a consistent expectation for attaching struct_ops to
cgroups across subsystems by reusing the existing bpf_cgroup_link and
handling struct_ops maps similarly to traditional program links.

> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 081d81de18163..745e826c9f615 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
[ ... ]
> @@ -2820,6 +2897,270 @@ const struct bpf_verifier_ops cg_sockopt_verifier_ops = {
[ ... ]
> +static int cgroup_struct_ops_link_update(struct bpf_link *link, struct bpf_map *new_map,
> +					 struct bpf_map *expected_old_map)
> +{
[ ... ]
> +	bpf_map_inc(new_map);
> +	WRITE_ONCE(cg_link->map, new_map);
> +	replace_effective_prog(cgrp, atype, pl);

[Severity: High]
This is a pre-existing issue, but does replace_effective_prog() risk
overwriting the wrong program in the physical items array?

In replace_effective_prog(), effective_prog_pos() returns a logical array
index based on the current hlist length, which is then used directly as a
physical index into the progs->items[] array:

kernel/bpf/cgroup.c:replace_effective_prog() {
    ...
    pos = effective_prog_pos(desc, atype, pl);
    ...
    item = &progs->items[pos];
    prog_list_replace_item(pl, item);
}

If a detach operation falls back to purge_effective_progs() (e.g., due to a
memory allocation failure in update_effective_progs()), it marks the detached
program's entry as a dummy without shifting elements, and removes it from
the hlist.

If another program is subsequently updated via BPF_PROG_ATTACH with
BPF_F_REPLACE, effective_prog_pos() calculates the target's logical index
based on the new, shorter hlist.

Using this logical index on the physical array could point to the dummy
element instead of the target program's actual slot. 

Would this cause the update to overwrite the dummy, leaving both the new and
original programs to execute?

Should replace_effective_prog() skip dummy elements when indexing the
physical array to align the logical index with the physical position?

> +	bpf_map_put(old_map);
> +	cgrp->bpf.revisions[atype]++;
> +
> +out:
> +	cgroup_unlock();
> +	return err;
> +}

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