Re: [PATCH 2/4] bpf: add bpf_cgroup_{flush_stats,stall} function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing bounds check on the `states` enum parameter in `bpf_cgroup_stall` allows out-of-bounds memory access.
- [High] Both `bpf_cgroup_stall` and `bpf_cgroup_flush_stats` unconditionally dereference the pointer returned by `cgroup_psi()`, which can be NULL.
- [High] Compilation fails when `CONFIG_CGROUPS=y` but `CONFIG_PSI=n`.
- [Medium] The `bpf_cgroup_stall` function truncates its 64-bit return value on 32-bit architectures.
--

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

bpf: add bpf_cgroup_{flush_stats,stall} function

Add bpf_cgroup_{flush_stats,stall} function to prepare for the
subsequent mthp_ext ebpf program.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 2bb60200c266..87f3072adce3 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -2819,6 +2820,32 @@ __bpf_kfunc struct cgroup *bpf_cgroup_from_id(u64 cgid)
>  	return cgrp;
>  }
>  
> +/**
> + * bpf_cgroup_stall - acquire the total stall time of cgroup
> + * @cgrp: cgroup struct
> + * @states: psi states
> + *
> + * Return the total stall time.
> + */
> +__bpf_kfunc unsigned long bpf_cgroup_stall(struct cgroup *cgrp,
> +					   enum psi_states states)

Is it possible that div_u64() returns a 64-bit value that gets truncated
here on 32-bit architectures? Since unsigned long is 32 bits on those
systems, would changing the return type to u64 prevent the stall time
from wrapping around?

One more thing regarding the build, what happens when CONFIG_CGROUPS is
enabled but CONFIG_PSI is disabled? Since struct psi_group might be defined
as an empty struct without a total field in include/linux/psi_types.h, and
cgroup_psi() wouldn't be declared, would it be better to wrap these kfuncs
and their BTF_ID_FLAGS in an #ifdef CONFIG_PSI block?

> +{
> +	struct psi_group *group = cgroup_psi(cgrp);
> +
> +	return div_u64(group->total[PSI_AVGS][states], NSEC_PER_MSEC);

Could states be out of bounds here? Since the BPF verifier treats enum
parameters as unconstrained scalars, a BPF program might pass an arbitrary
or negative integer. Does this need a runtime bounds check like
states < 0 || states >= NR_PSI_STATES - 1 ?

Also, is it possible for group to be NULL? If cgroup PSI tracking is
disabled at runtime via the cgroup_disable=pressure boot parameter,
cgroup_psi(cgrp) can return NULL for non-root cgroups. Does this dereference
of group->total need a NULL check first?

> +}
> +
> +/**
> + * bpf_cgroup_flush_stats - Flush cgroup's statistics
> + * @cgrp: cgroup struct
> + */
> +__bpf_kfunc void bpf_cgroup_flush_stats(struct cgroup *cgrp)
> +{
> +	struct psi_group *group = cgroup_psi(cgrp);
> +
> +	psi_group_flush_stats(group);

Similar to bpf_cgroup_stall(), if PSI tracking is disabled, group could
be NULL here. Since psi_group_flush_stats() might unconditionally acquire
a mutex, does this code need a NULL check on group to prevent a panic?

> +}

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