Re: [PATCH 1/3] selftests/bpf: add memcg_stat_reader BPF-vs-memory.stat benchmark

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Multi-line comments in the BPF selftest header `memcg_stat_reader.h` violate the BPF subsystem comment style guidelines by not having the opening `/*` on its own line.

Pre-existing issues:
- [High] The kfunc `bpf_mem_cgroup_vm_events` allows an out-of-bounds array read due to a missing negative bounds check on its signed enum argument.
--

commit 4ae0facb76303e67faffbd1e910d6587123d09d6
Author: Ziyang Men <ziyang.meme@gmail.com>

selftests/bpf: add memcg_stat_reader BPF-vs-memory.stat benchmark

This commit adds a BPF selftest to benchmark reading memory cgroup statistics
by walking the cgroup subtree using a BPF program and comparing the cost
against traditional userspace file reads.

> diff --git a/tools/testing/selftests/bpf/memcg_stat_reader.h b/tools/testing/selftests/bpf/memcg_stat_reader.h
> new file mode 100644
> index 0000000000000..72afebe95ccb8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/memcg_stat_reader.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
> +#ifndef __MEMCG_STAT_READER_H
> +#define __MEMCG_STAT_READER_H
> +
> +/*
> + * One per-cgroup snapshot, produced by the BPF cgroup iterator and read back
> + * from a BPF hash map keyed by cgroup id.  The "matched" subset is always
> + * populated so it can be compared field-by-field against what userspace parses
> + * out of memory.stat / memory.current / memory.max.  The "full" fold is only
> + * populated when collect_full is set and exists to (a) force the extra kfunc
> + * reads to happen (so the full-vs-matched timing is honest) and (b) give a
> + * coarse, informational signal of how many fields the full path touched.
> + */
> +struct memcg_stat_snapshot {
> +	__u64 cgroup_id;
> +
> +	/* Matched subset. Page-state values are in bytes (already unit-scaled
> +	 * by the kernel), so they compare directly against memory.stat.
> +	 */

[Severity: Low]
Does this multi-line comment follow the modern kernel formatting rules?
The BPF subsystem requires the opening /* to be on its own line, rather than
sharing a line with the comment text.

> +	__u64 anon;		/* NR_ANON_MAPPED, bytes */
> +	__u64 file;		/* NR_FILE_PAGES, bytes */
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/memcg_stat_reader.c b/tools/testing/selftests/bpf/progs/memcg_stat_reader.c
> new file mode 100644
> index 0000000000000..a2c1b1b483646
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/memcg_stat_reader.c
[ ... ]
> +	/* Matched subset: always collected so correctness holds in both modes. */
> +	snap.anon = bpf_mem_cgroup_page_state(memcg,
> +			bpf_core_enum_value(enum node_stat_item, NR_ANON_MAPPED));
> +	snap.file = bpf_mem_cgroup_page_state(memcg,
> +			bpf_core_enum_value(enum node_stat_item, NR_FILE_PAGES));
> +	snap.shmem = bpf_mem_cgroup_page_state(memcg,
> +			bpf_core_enum_value(enum node_stat_item, NR_SHMEM));
> +	snap.file_mapped = bpf_mem_cgroup_page_state(memcg,
> +			bpf_core_enum_value(enum node_stat_item, NR_FILE_MAPPED));
> +	snap.pgfault = bpf_mem_cgroup_vm_events(memcg,
> +			bpf_core_enum_value(enum vm_event_item, PGFAULT));

[Severity: High]
This is a pre-existing issue, but does bpf_mem_cgroup_vm_events() prevent an
out-of-bounds read when a BPF program passes a negative event index?

Since the verifier does not check enum ranges, a negative value can reach
memcg_vm_event_item_valid() via this kfunc:

mm/bpf_memcontrol.c:bpf_mem_cgroup_vm_events() {
    ...
    if (unlikely(!memcg_vm_event_item_valid(event)))
        return (unsigned long)-1;
    ...
}

In memcg_vm_event_item_valid(), the index is compared against
NR_VM_EVENT_ITEMS using a signed comparison:

mm/memcontrol.c:memcg_vm_event_item_valid() {
    if (idx >= NR_VM_EVENT_ITEMS)
        return false;
    ...
}

Because both idx and NR_VM_EVENT_ITEMS are signed, a negative index bypasses
the check. This could allow the negative index to be used when accessing
the events array, resulting in an out-of-bounds read of kernel memory. Should
memcg_vm_event_item_valid() include a check for negative values, or cast the
value to an unsigned integer?

> +
> +	/* page_counter fields need no kfunc; read them off the trusted ptr. */
> +	snap.usage_pages = BPF_CORE_READ(memcg, memory.usage.counter);
> +	snap.max_pages = BPF_CORE_READ(memcg, memory.max);

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