Re: [PATCH v3 1/4] cgroup: add BPF kfuncs to read a cpu cgroup's stats

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `bpf_cgroup_base_stat` kfunc is exposed to all BPF program types but unsafely acquires a non-NMI-safe lock and unconditionally enables hardware interrupts, leading to IRQ state corruption, NMI deadlocks, and PREEMPT_RT panics.
--

--- Patch [1]: [PATCH v3 1/4] cgroup: add BPF kfuncs to read a cpu cgroup's stats ---
commit 41558082c4ece37f910c5109f3537477a7ba8820
Author: Ziyang Men <[email protected]>

cgroup: add BPF kfuncs to read a cpu cgroup's stats

This commit adds BPF kfuncs for reading CPU cgroup statistics. It registers
css_rstat_flush() and bpf_cgroup_base_stat(), allowing BPF programs to read
CFS bandwidth counters directly.

> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> index de816a43db9f0..5db72504a8a5a 100644
> --- a/kernel/cgroup/rstat.c
> +++ b/kernel/cgroup/rstat.c

[ ... ]

> @@ -752,10 +752,49 @@ void cgroup_base_stat_cputime_show(struct seq_file *seq)
>  	cgroup_force_idle_show(seq, &bstat);
>  }
>  
> -/* Add bpf kfuncs for css_rstat_updated() and css_rstat_flush() */
> +#ifdef CONFIG_BPF_SYSCALL
> +
> +__bpf_kfunc_start_defs();
> +
> +/**
> + * bpf_cgroup_base_stat - Read a cgroup's base statistics
> + * @cgrp: cgroup to read from
> + * @out: zero-initialized output in nanoseconds
> + *
> + * CPU time is adjusted as for cpu.stat.
> + */
> +__bpf_kfunc void bpf_cgroup_base_stat(struct cgroup *cgrp,
> +				      struct cgroup_base_stat *out)
> +{
> +	if (cgroup_parent(cgrp)) {
> +		__css_rstat_lock(&cgrp->self, -1);
> +		*out = cgrp->bstat;
> +		cputime_adjust(&cgrp->bstat.cputime, &cgrp->prev_cputime,
> +			       &out->cputime.utime, &out->cputime.stime);
> +		__css_rstat_unlock(&cgrp->self, -1);

[Severity: High]
Does this code unconditionally enable hardware interrupts when called from
arbitrary BPF contexts?

Since this kfunc is registered under BPF_PROG_TYPE_UNSPEC without context
checks, it can be called from tracing programs (like kprobes or perf_events)
that may run in NMI context or with interrupts already disabled.

If it executes in an interrupt-disabled context, __css_rstat_unlock() will
unconditionally re-enable hardware interrupts and violate the caller's locking
assumptions:

kernel/cgroup/rstat.c:__css_rstat_unlock() {
	...
	spin_unlock_irq(lock);
}

Also, could calling this from an atomic tracing context cause a panic on
PREEMPT_RT, since spinlock_t is a sleeping lock there?

> +	} else {
> +		root_cgroup_cputime(out);
> +	}
> +}

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