Re: [PATCH 11/15] sched_ext: Split curr|donor references properly

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] rq_is_open() incorrectly inspects rq->curr instead of rq->donor when evaluating SCX_ENQ_PREEMPT constraints.
- [High] scx_rescue_charge() misattributes rescue execution time by dereferencing scx_task_sched(rq->curr) instead of rq->donor.
- [Medium] dequeue_task_scx() and scx_bypass() incorrectly evaluate task_current() instead of task_current_donor().
- [Medium] Blocked proxy donors incorrectly enter ops.running() and ops.stopping() sessions, directly contradicting documentation.
--

--- Patch [11]: [PATCH 11/15] sched_ext: Split curr|donor references properly ---
commit 85eb7af3f37babba254fc28b03cd51fb5e7deb18
Author: Andrea Righi <arighi@nvidia.com>
sched_ext: Split curr|donor references properly

This commit updates sched_ext to correctly differentiate between the physical 
execution task (rq->curr) and the scheduling context donor (rq->donor) during 
proxy execution. It modifies scheduling policy and accounting paths to rely on 
rq->donor, resolving discrepancies when the two tasks differ. It also extends 
scx_dump_state() to explicitly report both contexts.

> diff --git a/Documentation/scheduler/sched-ext.rst b/Documentation/scheduler/sched-ext.rst
> index ad2fff3c05937..db1ef89ed8f2a 100644
> --- a/Documentation/scheduler/sched-ext.rst
> +++ b/Documentation/scheduler/sched-ext.rst
> @@ -487,6 +487,12 @@ and edge cases, to name a few examples:
>    class, in which case it will exit the tick-dispatch loop even though it is runnable
>    and has a non-zero slice.
>  
> +* Under proxy execution, sched_ext continues to observe the donor as the current
> +  scheduling context. A blocked donor does not enter an ``ops.running()`` /
> +  ``ops.stopping()`` session because it does not execute itself, and the lock
> +  owner executing on its behalf is intentionally not reported through these
> +  callbacks.
> +

[Severity: Medium]
Will blocked proxy donors incorrectly enter ops.running() and ops.stopping() 
sessions, contradicting this documentation?

In set_next_task_scx(), when restoring a previously resolved context (!first):

    if ((p->scx.flags & SCX_TASK_QUEUED) && (!p->is_blocked || !first))
        scx_start_task_running(rq, p);

And in scx_proxy_donor_start():

    if (donor->sched_class == &ext_sched_class &&
        (donor->scx.flags & SCX_TASK_QUEUED))
        scx_start_task_running(rq, donor);

Does calling scx_start_task_running() unconditionally for queued donors here 
mark the task with SCX_TASK_RUN_TRACKED and invoke the BPF running callbacks, 
violating the API contract that blocked donors do not execute themselves?

>  See the "Scheduling Cycle" section for a more detailed description of how
>  a freshly woken up task gets on a CPU.
>  
> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 7b467b666212b..39d3e3af05f90 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -1438,23 +1438,30 @@ static void apply_slice_vtime(struct task_struct *p, u64 slice, u64 vtime, u64 e
>  
>  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;
> +
>  	/* apply even on 0 delta_exec, callers may still act on the slice */
> -	apply_task_slice_oob(rq, curr);
> +	apply_task_slice_oob(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);
>  	}
>  
> -	if (unlikely(curr == scx_rescuee(rq)))
> +	if (unlikely(donor == scx_rescuee(rq)))
>  		scx_rescue_charge(rq, delta_exec);

[Severity: High]
Does scx_rescue_charge() misattribute rescue execution time by dereferencing 
scx_task_sched(rq->curr) instead of rq->donor?

When a proxy donor is placed in the rescue DSQ and consumes time, 
scx_rescue_charge() is called here, but internally it does:

    pcpu = per_cpu_ptr(scx_task_sched(rq->curr)->pcpu, cpu_of(rq));
    pcpu->rescue_avg = scx_rescue_decay_avg(pcpu) + delta_exec;

If rq->curr is a non-ext task (like fair), scx_task_sched() could return 
incorrect scheduler contexts or default structures. Can this lead to accounting 
corruption or potential pointer dereferences when it charges the consumed 
budget to the physical execution task's scheduler instead of the ext donor?

>  
>  	dl_server_update(&rq->ext_server, delta_exec);

[ ... ]

> @@ -1634,9 +1641,9 @@ static void rq_owned_post_enq(struct scx_sched *sch, struct rq *rq,
>  	if (rq->scx.flags & SCX_RQ_IN_BALANCE)
>  		return;
>  
> -	if ((enq_flags & SCX_ENQ_PREEMPT) && p != rq->curr &&
> -	    rq->curr->sched_class == &ext_sched_class) {
> -		if (likely(scx_set_task_slice(rq->curr, 0)))
> +	if ((enq_flags & SCX_ENQ_PREEMPT) && p != rq->donor &&
> +	    rq->donor->sched_class == &ext_sched_class) {
> +		if (likely(scx_set_task_slice(rq->donor, 0)))
>  			resched_curr(rq);
>  		else
>  			__scx_add_event(sch, SCX_EV_SLICE_DENIED, 1);

[Severity: High]
Does rq_is_open() also need to check rq->donor instead of rq->curr when 
evaluating SCX_ENQ_PREEMPT constraints?

Currently, rq_is_open() evaluates the physical execution task (rq->curr):

    struct task_struct *curr = rq->curr;
    return curr->sched_class != &ext_sched_class ||
        likely(!(curr->scx.flags & SCX_TASK_PROTECTED));

Under proxy execution, if rq->curr is a fair task while the donor (rq->donor) 
is an ext task with a protected slice, rq_is_open() would return true, causing 
dsq_inc_nr() to queue an immed task assuming immediate preemption.

However, since rq_owned_post_enq() here correctly checks rq->donor and refuses 
preemption, could this leave the newly queued task stranded on the local DSQ 
without a reschedule kick, causing starvation?

[ ... ]

> @@ -2212,13 +2219,14 @@ static void enqueue_task_scx(struct rq *rq, struct task_struct *p, int core_enq_
>  		rq->scx.flags |= SCX_RQ_IN_WAKEUP;
>  
>  	/*
> -	 * Restoring a running task will be immediately followed by
> -	 * set_next_task_scx() which expects the task to not be on the BPF
> +	 * Restoring the current scheduling context will be immediately followed
> +	 * by set_next_task_scx() which expects the task to not be on the BPF
>  	 * scheduler as tasks can only start running through local DSQs. Force
>  	 * direct-dispatch into the local DSQ by setting the sticky_cpu. Mark
>  	 * IGNORE_CAPS to force entry into the local DSQ.
>  	 */
> -	if (unlikely(enq_flags & ENQUEUE_RESTORE) && task_current(rq, p)) {
> +	if (unlikely(enq_flags & ENQUEUE_RESTORE) &&
> +	    task_current_donor(rq, p)) {
>  		sticky_cpu = cpu_of(rq);
>  		enq_flags |= SCX_ENQ_IGNORE_CAPS;
>  	}

[Severity: Medium]
Are there other places that should use task_current_donor() instead of 
task_current() to avoid stripping the slice from the active proxy donor 
prematurely?

For example, in dequeue_task_scx():

    if (!((deq_flags & DEQUEUE_SAVE) && task_current(rq, p)))
        scx_task_slice_ended(rq, p);

If a proxy donor is dequeued with DEQUEUE_SAVE (e.g. for a priority change), 
the task_current(rq, p) check will fail because the physical execution task is 
not the donor, causing the donor to lose its slice protection.

Similarly in scx_bypass():

    if (bypass && task_current(rq, p))
        scx_task_slice_ended(rq, p);

Could this fail to clear the slice for the active proxy donor because 
task_current() evaluates to false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810151523.86994-1-arighi@nvidia.com?part=11