Re: [PATCH v3 2/4] selftests/cgroup: add memcg_stat_cross_cpu correctness test for flush

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

New issues:
- [Low] Multi-line comment does not follow the correct BPF/kernel style.

Pre-existing issues:
- [High] The kfuncs bpf_mem_cgroup_memory_events and bpf_mem_cgroup_vm_events lack negative bounds checks for their enum parameters, leading to out-of-bounds array reads.
--

--- Patch [2]: [PATCH v3 2/4] selftests/cgroup: add memcg_stat_cross_cpu correctness test for flush ---
commit aa1f4cd8dd58a883c661755d56a15bc86e70795f
Author: Ziyang Men <[email protected]>

selftests/cgroup: add memcg_stat_cross_cpu correctness test for flush

This patch adds a new selftest, test_memcg_stat_cross_cpu, to verify
that cgroup flushes happen correctly by ensuring BPF read values match
cgroupfs file readings. It also strengthens test coverage by scattering
charges across multiple CPUs.

> diff --git a/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c b/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c
> new file mode 100644
> index 0000000000000..3c2acd49c16ba
> --- /dev/null
> +++ b/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
> +#define BPF_NO_KFUNC_PROTOTYPES
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_core_read.h>
> +#include "memcg_stat_cross_cpu.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +/*
> + * Declared here rather than taken from vmlinux.h, which only has them if the
> + * build host's own kernel does.  A kernel without them is caught at run time.
> + */
> +struct mem_cgroup *bpf_get_mem_cgroup(struct cgroup_subsys_state *css) __ksym;
> +void bpf_put_mem_cgroup(struct mem_cgroup *memcg) __ksym;
> +void bpf_mem_cgroup_flush_stats(struct mem_cgroup *memcg) __ksym;
> +unsigned long bpf_mem_cgroup_page_state(struct mem_cgroup *memcg, int idx) __ksym;
> +unsigned long bpf_mem_cgroup_vm_events(struct mem_cgroup *memcg,
> +				       enum vm_event_item event) __ksym;

[Severity: High]
This is a pre-existing issue, but does this kfunc allow an out-of-bounds array
read?

The kfuncs bpf_mem_cgroup_memory_events() and bpf_mem_cgroup_vm_events() lack
negative bounds checks for their enum parameters. A BPF program can pass a
negative integer as the enum parameter to bpf_mem_cgroup_vm_events().

This passes the negative enum to memcg_vm_event_item_valid() in mm/memcontrol.c:

    if (idx >= NR_VM_EVENT_ITEMS)

This validation function omits a negative bounds check (e.g., missing an
unsigned cast). Because the BPF verifier evaluates enum parameters as signed
integers without an explicit lower bound check, negative values bypass the
check. This reaches memcg_events_index() and allows arbitrary memory offsets
to be read relative to the memcg arrays.

[ ... ]

> diff --git a/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c b/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c
> new file mode 100644
> index 0000000000000..9946c41c7a908
> --- /dev/null
> +++ b/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c

[ ... ]

> +static int add_node(const char *path, bool is_leaf)
> +{
> +	struct cg_node *n = &nodes[n_nodes];
> +
> +	if (cg_create(path))
> +		return -1;
> +
> +	strncpy(n->path, path, sizeof(n->path) - 1);
> +	n->id = cg_get_id(path);
> +	n->is_leaf = is_leaf;
> +	if (is_leaf)
> +		n_leaves++;
> +	n_nodes++;
> +	return 0;
> +}
> +
> +/* Create # @levels more levels below @path.
> + * Only the last level is charged.
> + */

[Severity: Low]
Does this multi-line comment style follow the BPF and kernel guidelines?
The preferred style requires the opening /* to be on its own line:

/*
 * Create # @levels more levels below @path.
 * Only the last level is charged.
 */

> +static int build_children(const char *path, int fanout, int levels)
> +{
> +	char child[PATH_MAX];
> +	int i;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2