Re: [PATCH v2 4/4] samples: bpf: add mthp_ext

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Small memory processes bypass the 4KB fallback rule due to a faulty condition that excludes zero memory usage.
- [High] The cgroup memory check is indefinitely bypassed for cgroups that do not experience memory pressure because a stall value of `0` is treated as uninitialized.
- [Medium] The BPF program submits uninitialized memory to the ring buffer if reading the cgroup name fails, which can crash the userspace handler.
- [Medium] The global `exiting` flag is accessed locklessly without the `volatile sig_atomic_t` qualifier, potentially leading to an infinite loop.
--

commit d747c2eece6f13195b37f744a5f3e9fd6d9a2c95
Author: Vernon Yang <yanglincheng@kylinos.cn>

samples: bpf: add mthp_ext

Design mthp_ext case to address real workload issues by adjusting mTHP size
via a cgroup BPF program.

> diff --git a/samples/bpf/mthp_ext.bpf.c b/samples/bpf/mthp_ext.bpf.c
> new file mode 100644
> --- /dev/null
> +++ b/samples/bpf/mthp_ext.bpf.c
[ ... ]
> +SEC("iter.s/cgroup")
> +int cgroup_scan(struct bpf_iter__cgroup *ctx)
> +{
[ ... ]
> +	bpf_cgroup_flush_stats(cgrp);
> +	curr_stall = bpf_cgroup_stall(cgrp, PSI_MEM_FULL);
> +	if (!info->stall) {

Does this logic unintentionally bypass the memory check for cgroups without
memory pressure?

bpf_cgroup_stall() can return exactly 0 for cgroups that have not experienced
any memory pressure. If curr_stall is 0, info->stall will be set to 0.

On subsequent scans, !info->stall will evaluate to true again, continuously
resetting the order to PMD_ORDER and skipping the memory size evaluation
below.

> +		info->order = configs.init_order;
> +		goto UPDATE;
> +	}
> +	delta = curr_stall - info->stall;
> +	bpf_mem_cgroup_flush_stats(memcg);
> +	curr_mem = bpf_mem_cgroup_page_state(memcg, NR_ANON_MAPPED) +
> +		   bpf_mem_cgroup_page_state(memcg, NR_SHMEM);
> +	if ((curr_mem && curr_mem < FROM_MB(configs.min_mem)) ||

Could this condition allow new processes to bypass the 4KB fallback rule?

When a process starts and has not faulted in any anonymous or shmem memory,
its memory usage is 0. The curr_mem && check causes this to evaluate to false,
falling through to the else branch and assigning PMD_ORDER.

This seems to allow small or starting processes to allocate 2MB huge pages
for their first allocations instead of falling back to 4KB.

> +	     delta >= configs.threshold)
> +		info->order = 0;
> +	else
> +		info->order = PMD_ORDER;
> +
> +	if (configs.debug) {
> +		e = bpf_ringbuf_reserve(&events, sizeof(*e), 0);
> +		if (e) {
> +			e->prev_stall = info->stall;
> +			e->curr_stall = curr_stall;
> +			e->delta = delta;
> +			e->mem = curr_mem;
> +			e->order = info->order;
> +			bpf_probe_read_kernel_str(e->name, sizeof(e->name),
> +						  cgrp->kn->name);

Is it possible for e->name to contain uninitialized garbage if
bpf_probe_read_kernel_str() fails?

bpf_ringbuf_reserve() does not zero-initialize the memory. If the string
read fails, e->name might lack a null terminator. This could cause a
segmentation fault in the userspace daemon when it processes the event using
printf.

> +			bpf_ringbuf_submit(e, 0);
> +		}
> +	}
[ ... ]
> diff --git a/samples/bpf/mthp_ext.c b/samples/bpf/mthp_ext.c
> new file mode 100644
> --- /dev/null
> +++ b/samples/bpf/mthp_ext.c
[ ... ]
> +static bool exiting;

Should this be declared as volatile sig_atomic_t?

Since exiting is modified asynchronously in the signal handler and read in the
while (!exiting) loops, an optimizing compiler might hoist the read out of
the loop. This could potentially cause the program to hang indefinitely
instead of exiting on a signal.

> +
> +static void usage(const char *name)
> +{
[ ... ]

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