Re: [PATCH 04/10] sched_ext: Split curr|donor references properly

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing conversion from `task_current()` to `task_current_donor()` in sched_ext task state management skips BPF callbacks and corrupts task state.
- [High] BPF kfuncs expose the physical execution context instead of the scheduling context, violating the commit's design principle.
- [High] `dequeue_task_scx` unconditionally calls `update_curr_scx`, which steals execution time from non-EXT proxy donors and corrupts their accounting.
- [High] scx_can_stop_tick() stops the tick for FAIR donors executing via EXT owners, bypassing CFS bandwidth constraints.
--

--- Patch [4]: [PATCH 04/10] sched_ext: Split curr|donor references properly ---
commit ed60d9105c70ea6983b5253a3067b0440935ec2f
Author: Andrea Righi <[email protected]>

sched_ext: Split curr|donor references properly

With proxy execution, the task selected by the scheduler and the task
physically executing can differ. A blocked mutex waiter donates its
scheduling context to the lock owner.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 1412239d37f52..755ea5cc6169f 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -1170,17 +1170,24 @@ static void touch_core_sched_dispatch(struct rq *rq, struct task_struct *p)
>  
>  static void update_curr_scx(struct rq *rq)
>  {
> -	struct task_struct *curr = rq->curr;
> +	struct task_struct *donor;
>  	s64 delta_exec;
>  
> +	/*
> +	 * update_curr_scx() is selected through rq->donor->sched_class, not
> +	 * rq->curr->sched_class, so @donor is always an EXT task here. If an EXT
> +	 * owner executes for a FAIR donor, FAIR's update_curr() runs instead.
> +	 */
> +	donor = rq->donor;
> +
>  	delta_exec = update_curr_common(rq);
>  	if (unlikely(delta_exec <= 0))
>  		return;
>  
> -	if (curr->scx.slice != SCX_SLICE_INF) {
> -		curr->scx.slice -= min_t(u64, curr->scx.slice, delta_exec);
> -		if (!curr->scx.slice)
> -			touch_core_sched(rq, curr);
> +	if (donor->scx.slice != SCX_SLICE_INF) {
> +		donor->scx.slice -= min_t(u64, donor->scx.slice, delta_exec);
> +		if (!donor->scx.slice)
> +			touch_core_sched(rq, donor);
>  	}

[Severity: High]
Does this steal execution time from non-EXT proxy donors?

If an EXT task executes on behalf of a FAIR donor, dequeuing the EXT owner
calls update_curr_scx() from dequeue_task_scx() because it evaluates
task_current(rq, p) which matches the EXT owner.

This code advances rq->donor->se.exec_start via update_curr_common() and
mutates the non-EXT donor's slice. Will this bypass CFS vruntime and
bandwidth (throttling) accounting, starving the FAIR task of its tracked
CPU time?

[ ... ]

> @@ -4081,14 +4092,14 @@ static void run_deferred(struct rq *rq)
>  #ifdef CONFIG_NO_HZ_FULL
>  bool scx_can_stop_tick(struct rq *rq)
>  {
> -	struct task_struct *p = rq->curr;
> +	struct task_struct *p = rq->donor;
>  	struct scx_sched *sch = scx_task_sched(p);
>  
>  	if (p->sched_class != &ext_sched_class)
>  		return true;

[Severity: High]
Does this incorrectly bypass CFS bandwidth constraints for FAIR donors?

If a CPU is running a bandwidth-constrained FAIR donor through an EXT owner
on a NOHZ CPU, this returns true.

This delegates tick responsibility to core generic checks, which mistakenly
evaluate the EXT owner (rq->curr) instead of the FAIR donor in
__need_bw_check. Can this allow the FAIR task to bypass CFS quota limits?

[ ... ]

[Severity: High]
Are we missing conversions from task_current() to checking the donor in the
enqueue and dequeue paths?

In dequeue_task_scx(), if we have a blocked EXT proxy donor task (where
rq->donor == p but rq->curr != p) that is dequeued, task_current(rq, p)
evaluates to false:

    if (task_current(rq, p) &&
        (p->scx.flags & SCX_TASK_RUN_TRACKED)) {

Does this incorrectly skip the BPF stopping() callback and leak the
SCX_TASK_RUN_TRACKED state?

Similarly, in enqueue_task_scx():

    if (unlikely(enq_flags & ENQUEUE_RESTORE) && task_current(rq, p))
        sticky_cpu = cpu_of(rq);

Does this fail to preserve sticky_cpu for an actively running donor because
task_current(rq, p) is false, breaking the restore invariants?

[Severity: High]
Do the BPF kfuncs expose the physical execution context instead of the
scheduling context?

In scx_bpf_task_running():

    return task_rq(p)->curr == p;

This returns false for an active EXT donor because it checks the owner
(rq->curr) instead of the donor.

In scx_bpf_cpu_curr():

    return rcu_dereference(cpu_rq(cpu)->curr);

And in scx_bpf_cid_curr():

    return rcu_dereference(cpu_rq(cpu)->curr);

Do these expose the physical non-EXT owner to the BPF scheduler instead
of the expected EXT donor, causing the BPF scheduler to make incorrect
load-balancing decisions based on invalid task states?

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