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

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

New issues:
- [Medium] Cross-CPU charge scattering fails to trigger page faults on all K CPUs when resident_bytes / k < PAGE_SIZE.
- [Medium] Commit message contains inaccurate claims regarding the test framework and utility helpers.
- [Low] Multi-line comment does not follow the required BPF subsystem style.

Pre-existing issues:
- [High] Missing bounds checks for negative enum values in kfuncs bpf_mem_cgroup_vm_events and bpf_mem_cgroup_memory_events allow out-of-bounds array access.
--

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

selftests/cgroup: add memcg_stat_cross_cpu correctness test for flush

This commit adds a memory-cgroup BPF selftest that verifies if the
memory-cgroup BPF kfuncs return values that match what userspace reads
from cgroupfs. It builds a cgroup tree, charges memory across multiple
CPUs, and compares the flushed kfunc stats to the traditional memory.stat
reads.

> Add a test_progs selftest that verifies the memory-cgroup BPF kfuncs
[ ... ]
> The traditional path reads memory.stat / memory.current through a new
> read_cgroup_file() helper added to cgroup_helpers (the read counterpart
> of write_cgroup_file).

[Severity: Medium]
The commit message states that a test_progs selftest was added and a new
read_cgroup_file() helper was introduced. However, the patch implements a
standalone kselftest via TEST_GEN_PROGS and relies on the pre-existing
cg_read() helper instead. Could the commit message be updated to accurately
reflect the implemented code?

[ ... ]
> diff --git a/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c b/tools/testing/selftests/cgroup/memcg_stat_cross_cpu.bpf.c
[ ... ]
> +	snap.file = bpf_mem_cgroup_page_state(memcg, idx_file);
> +	snap.shmem = bpf_mem_cgroup_page_state(memcg, idx_shmem);
> +	snap.file_mapped = bpf_mem_cgroup_page_state(memcg, idx_fmapped);
> +	snap.pgfault = bpf_mem_cgroup_vm_events(memcg, idx_pgfault);

[Severity: High]
This is a pre-existing issue, but the called kfuncs
bpf_mem_cgroup_vm_events() and bpf_mem_cgroup_memory_events() in
mm/bpf_memcontrol.c lack negative bounds checks for their enum arguments.

Since the BPF verifier does not enforce enum ranges, a BPF program could
pass a negative enum value which bypasses the upper-bound check (e.g.,
idx >= NR_VM_EVENT_ITEMS). This happens because the C code evaluates the
signed integer, leading to an out-of-bounds array access.

Can we add the required negative bounds checks to these kfuncs?

> +
> +	/* page_counter fields need no kfunc; read them off the trusted ptr. */
[ ... ]
> diff --git a/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c b/tools/testing/selftests/cgroup/test_memcg_stat_cross_cpu.c
[ ... ]
> +	/*
> +	 * Fault the region in K slices, each on a different CPU, so the charge
> +	 * for this leaf is scattered across K per-cpu rstat trees.  A correct
> +	 * flush must gather all K slices.
> +	 */
> +	per = resident_bytes / k;
> +	for (j = 0; j < k; j++) {
> +		off = (size_t)j * per;
> +		if (pin_cpu(cpu_list[(base + j) % n_cpu]))
> +			_exit(3);
> +		memset(region + off, 1,
> +		       (j == k - 1) ? resident_bytes - off : per);
> +	}

[Severity: Medium]
If the system has a large number of CPUs, can per (resident_bytes / k)
become smaller than the page size?

For example, in the cross_cpu_large_tree test case with 256KB
resident_bytes and >64 CPUs, multiple CPUs would be assigned offsets
within the same 4KB page. The first CPU writing to the page causes a page
fault, but subsequent CPUs writing to the same page will find it already
mapped and won't trigger faults, defeating the cross-CPU scattering
mechanism.

> +
> +	/* Ready: the charge is in place and spread across K CPUs. */
> +	if (write(charge_ready[1], "x", 1) != 1)
> +		_exit(4);
> +	close(charge_ready[1]);
> +
> +	/* Hold the charge (region stays mapped) until the parent tells
> +	 * us to exit by closing the control pipe.
> +	 */

[Severity: Low]
Does this multi-line comment format match the BPF subsystem style? The BPF
subsystem requires multi-line comments to have the opening /* on its own
line, even in selftests.

> +	while (read(charge_ctrl[0], &c, 1) > 0)
> +		;

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