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

Forwarding sashiko review

On Mon, May 4, 2026 at 1:25 AM <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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?

Yes, I will fix that in the next version. Thanks!

> 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?

Indeed, this is a problem. Compared to adding “#ifdef CONFIG_PSI”, I
prefer adding follow, as it is clearer, Thanks!

diff --git a/include/linux/psi.h b/include/linux/psi.h
index 7b4fd8190810..243dcf97bea4 100644
--- a/include/linux/psi.h
+++ b/include/linux/psi.h
@@ -52,6 +52,10 @@ static inline void psi_memstall_enter(unsigned long
*flags) {}
 static inline void psi_memstall_leave(unsigned long *flags) {}

 #ifdef CONFIG_CGROUPS
+static inline struct psi_group *cgroup_psi(struct cgroup *cgrp)
+{
+       return NULL;
+}
 static inline int psi_cgroup_alloc(struct cgroup *cgrp)

> > +{
> > +     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 ?

Good catch, I will fix that in the next version. Thanks!

> 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?

I will fix that in the next version. Thanks!

> > +}
> > +
> > +/**
> > + * 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?

I will fix that in the next version. Thanks!

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