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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `bpf_cgroup_stall` kfunc reads the 64-bit statistic `group->total` locklessly, exposing a torn read vulnerability on 32-bit architectures.
- [Low] The patch introduces a build failure when `CONFIG_CGROUPS` is enabled but `CONFIG_PSI` is disabled.
--

commit 6a28dc9991d77ff5f1bc78ed809531c8806dd33a
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 2bb60200c2665..1c353e0ff14f4 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -2881,6 +2882,37 @@ bpf_task_get_cgroup1(struct task_struct *task, int hierarchy_id)
>  		return NULL;
>  	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 u64 bpf_cgroup_stall(struct cgroup *cgrp, enum psi_states states)
> +{
> +	struct psi_group *group = cgroup_psi(cgrp);
> +
> +	if (unlikely(!group || (u32)states >= NR_PSI_STATES - 1))
> +		return (u64)-1;
> +
> +	return div_u64(group->total[PSI_AVGS][states], NSEC_PER_MSEC);

Will this cause a torn read on 32-bit architectures?

The 64-bit cumulative counter group->total[PSI_AVGS][states] is updated using
standard non-atomic addition in collect_percpu_times(), which runs in process
context under group->avgs_lock.

Since bpf_cgroup_stall() reads this value directly without holding the lock,
and non-sleepable BPF programs can execute in NMI or hardirq contexts, they
can preempt the update process.

On 32-bit architectures, a 64-bit update and read are split into two 32-bit
operations. If the BPF program preempts the CPU between the lower and upper
half updates, it could read a torn 64-bit value, leading to an incorrect,
artificial spike or drop in the reported stall time.

> +}
> +
> +/**
> + * 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);
> +
> +	if (unlikely(!group))
> +		return;
> +
> +	psi_group_flush_stats(group);

Can this lead to a build failure when CONFIG_CGROUPS is enabled but CONFIG_PSI
is disabled?

Both bpf_cgroup_stall() and bpf_cgroup_flush_stats() are guarded by
#ifdef CONFIG_CGROUPS, but heavily depend on PSI-specific constructs like
enum psi_states, NR_PSI_STATES, PSI_AVGS, group->total, and
psi_group_flush_stats().

When CONFIG_PSI is disabled, include/linux/psi_types.h defines
struct psi_group as an empty struct and omits definitions for the enums and
macros, and psi_group_flush_stats() is undeclared.

> +}
>  #endif /* CONFIG_CGROUPS */

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