Re: [PATCH 1/2] sched_ext: Make scx_locked_rq() return NULL from NMI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] scx_bpf_cpuperf_set() and scx_bpf_cidperf_set() deadlock in NMI when scx_locked_rq() returns NULL.

Pre-existing issues:
- [High] Tracing BPF programs executing in NMI context can deadlock the system by calling pre-existing 'any' category kfuncs that acquire raw spinlocks.
--

commit e3a4b62825e5b60c4d87fa4b8dd2f4b8dee65bbf
Author: Wanwu Li <liwanwu@kylinos.cn>

sched_ext: Make scx_locked_rq() return NULL from NMI

scx_locked_rq() reads the per-CPU scx_locked_rq_state, which tracks the
rq locked by the context running on the CPU. Tracing progs can call
kfuncs from NMI, and an NMI interrupts - rather than replaces - the
context that set scx_locked_rq_state, so a non-NULL read from NMI
falsely tells the caller that it holds the interrupted context's rq lock.

Link: https://lore.kernel.org/r/d84b31727f04e1ed0d40042ba1c09e61@kernel.org

> diff --git a/kernel/sched/ext/internal.h b/kernel/sched/ext/internal.h
> index 27bbf5e04d900..faa80ac5a653a 100644
> --- a/kernel/sched/ext/internal.h
> +++ b/kernel/sched/ext/internal.h
> @@ -2181,6 +2181,15 @@ static inline void scx_schedule_reenq_local(struct rq *rq, u64 reenq_flags)
>   */
>  static inline struct rq *scx_locked_rq(void)
>  {
> +	/*
> +	 * Tracing progs can call kfuncs from NMI. scx_locked_rq_state tracks
> +	 * the rq locked by the interrupted context, so a non-NULL read from
> +	 * NMI would falsely claim its lock. Return NULL from NMI so that
> +	 * callers take their unlocked paths.
> +	 */
> +	if (unlikely(in_nmi()))
> +		return NULL;
> +

[Severity: High]
Does returning NULL in NMI cause a deadlock in scx_cpuperf_set() and
scx_cidperf_set()?

When a tracing BPF program executing in NMI context calls
scx_bpf_cpuperf_set(), it now sees locked_rq as NULL:

kernel/sched/ext/ext.c:scx_cpuperf_set() {
    [ ... ]
    locked_rq = scx_locked_rq();

    /*
     * If no rq lock is held, allow to operate on any CPU by acquiring
     * the corresponding rq lock.
     */
    if (!locked_rq) {
        rq_lock_irqsave(rq, &rf);
        update_rq_clock(rq);
    }
    [ ... ]
}

If the interrupted kernel context was already holding this CPU's rq->lock,
wouldn't attempting to unconditionally acquire it here cause the NMI handler
to spin forever and deadlock the system?

[Severity: High]
This isn't a bug introduced by this patch, but several any category kfuncs
unconditionally acquire raw spinlocks. Can these cause similar deadlocks if
invoked from NMI tracing programs?

For example, scx_bpf_destroy_dsq() calls destroy_dsq():

kernel/sched/ext/ext.c:destroy_dsq() {
    [ ... ]
    raw_spin_lock_irqsave(&dsq->lock, flags);
    [ ... ]
}

Similarly, bpf_iter_scx_dsq_next() unconditionally takes the dsq lock:

kernel/sched/ext/ext.c:bpf_iter_scx_dsq_next() {
    [ ... ]
    guard(raw_spinlock_irqsave)(&kit->dsq->lock);
    [ ... ]
}

And scx_bpf_dsq_reenq() calls schedule_dsq_reenq() which also takes a raw
spinlock:

kernel/sched/ext/ext.c:schedule_dsq_reenq() {
    [ ... ]
    guard(raw_spinlock_irqsave)(&rq->scx.deferred_reenq_lock);
    [ ... ]
}

If an NMI interrupts a kernel path holding one of these locks and the tracing
program invokes the corresponding kfunc, will the NMI handler spin forever
trying to acquire the held lock?

>  	return __this_cpu_read(scx_locked_rq_state);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903032953.659847-1-liwanwu@kylinos.cn?part=1