Re: [PATCH sched_ext/for-7.3 07/32] sched_ext: Make the kick machinery per-sched

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] List corruption in `scx_kick_cpu()` and `kick_cpus_irq_workfn()` due to NMI context concurrency.
--

commit 9ac2a72a88974fa9fc65a32e794dca946c726004
Author: Tejun Heo <tj@kernel.org>

sched_ext: Make the kick machinery per-sched

This commit makes the preemption kick machinery per-scheduler instead of
sharing targets across every scheduler on the cpu. It moves the kick masks
into a per-cpu structure and updates the kick machinery to link the sched
onto a per-cpu list that is consumed by a local irq work function.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 13af43bca8507..8445e34e205fb 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -7543,15 +7574,24 @@ static void kick_cpus_irq_workfn(struct irq_work *irq_work)
>  
>  	ksyncs = rcu_dereference_bh(ksyncs_pcpu)->syncs;
>  
> -	for_each_cpu(cpu, this_scx->cpus_to_kick) {
> -		should_wait |= kick_one_cpu(cpu, this_rq, ksyncs);
> -		cpumask_clear_cpu(cpu, this_scx->cpus_to_kick);
> -		cpumask_clear_cpu(cpu, this_scx->cpus_to_kick_if_idle);
> -	}
> +	/*
> +	 * Walk scheds with pending kicks on this cpu. scx_kick_cpu() adds to
> +	 * the list under local_irq_save() and only this irq_work consumes it.
> +	 * A plain list without locking is sufficient.
> +	 */
> +	list_for_each_entry_safe(pcpu, tmp, &this_scx->sched_pcpus_to_kick, to_kick_node) {
> +		list_del_init(&pcpu->to_kick_node);

[Severity: High]
Does this code safely handle preemption by non-maskable interrupts?

The comment notes that a plain list without locking is sufficient because
scx_kick_cpu() adds to the list under local_irq_save(). However,
local_irq_save() does not mask nmi execution.

If an nmi fires while a cpu is in the middle of list_del_init() here in
kick_cpus_irq_workfn(), and the nmi handler invokes a bpf tracing program
that calls scx_bpf_kick_cpu(), could that concurrently execute
list_add_tail() on this same per-cpu list and corrupt the list pointers?

[ ... ]

> @@ -8499,16 +8541,18 @@ void scx_kick_cpu(struct scx_sched *sch, s32 cpu, u64 flags)
>  			}
>  			raw_spin_rq_unlock(target_rq);
>  		}
> -		cpumask_set_cpu(cpu, this_rq->scx.cpus_to_kick_if_idle);
> +		cpumask_set_cpu(cpu, pcpu->cpus_to_kick_if_idle);
>  	} else {
> -		cpumask_set_cpu(cpu, this_rq->scx.cpus_to_kick);
> +		cpumask_set_cpu(cpu, pcpu->cpus_to_kick);
>  
>  		if (flags & SCX_KICK_PREEMPT)
> -			cpumask_set_cpu(cpu, this_rq->scx.cpus_to_preempt);
> +			cpumask_set_cpu(cpu, pcpu->cpus_to_preempt);
>  		if (flags & SCX_KICK_WAIT)
> -			cpumask_set_cpu(cpu, this_rq->scx.cpus_to_wait);
> +			cpumask_set_cpu(cpu, pcpu->cpus_to_wait);
>  	}
>  
> +	if (list_empty(&pcpu->to_kick_node))
> +		list_add_tail(&pcpu->to_kick_node, &this_rq->scx.sched_pcpus_to_kick);

[Severity: High]
Can a similar race happen here during concurrent nmi execution?

Since scx_kick_cpu() protects list_add_tail() only with local_irq_save(),
an nmi can interrupt it. If the nmi executes a bpf program that also
triggers scx_kick_cpu(), won't the nested list_add_tail() operations
corrupt the standard doubly-linked list?

>  	irq_work_queue(&this_rq->scx.kick_cpus_irq_work);
>  out:
>  	local_irq_restore(irq_flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703080159.2314350-1-tj@kernel.org?part=7