Re: [PATCH 07/12] sched_ext: Split curr|donor references properly

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] task_tick_scx() unconditionally invokes ops.tick() for blocked proxy donors, violating the BPF state machine contract.
- [Medium] Missing task_current_donor() check in enqueue_task_scx() violates DSQ invariants for proxy donors.
--

--- Patch [7]: [PATCH 07/12] sched_ext: Split curr|donor references properly ---
commit 0a1e9539e4e14034ca89a7b3b4988f413e2e416b
Author: Andrea Righi <[email protected]>

sched_ext: Split curr|donor references properly

This commit updates the sched_ext scheduler class to distinguish between the
physical execution context and the scheduling context during proxy execution.
It ensures that sched_ext policy, accounting, and various BPF kfuncs observe
the donor task instead of the physically executing task.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index b5fd91d9bca4f..a466adf8e1548 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -3566,9 +3577,9 @@ void scx_tick(struct rq *rq)
>  	update_other_load_avgs(rq);
>  }
>  
> -static void task_tick_scx(struct rq *rq, struct task_struct *curr, int queued)
> +static void task_tick_scx(struct rq *rq, struct task_struct *donor, int queued)
>  {
> -	struct scx_sched *sch = scx_task_sched(curr);
> +	struct scx_sched *sch = scx_task_sched(donor);
>  
>  	update_curr_scx(rq);
>  
> @@ -3577,13 +3588,13 @@ static void task_tick_scx(struct rq *rq, struct task_struct *curr, int queued)
>  	 * we can't trust the slice management or ops.core_sched_before().
>  	 */
>  	if (scx_bypassing(sch, cpu_of(rq))) {
> -		set_task_slice(curr, 0);
> -		touch_core_sched(rq, curr);
> +		set_task_slice(donor, 0);
> +		touch_core_sched(rq, donor);
>  	} else if (SCX_HAS_OP(sch, tick)) {
> -		SCX_CALL_OP_TASK(sch, tick, rq, curr);
> +		SCX_CALL_OP_TASK(sch, tick, rq, donor);
>  	}

[Severity: High]
Does this code incorrectly call ops.tick() for a blocked donor?

The commit message explicitly states that a blocked donor does not enter an
ops.running() or ops.stopping() session. However, this unconditionally invokes
ops.tick() for the donor without verifying if it is blocked or tracked as
running.

Since BPF schedulers may allocate per-CPU state upon ops.running(), could this
crash or leak state when ops.tick() fires for a task the scheduler never
tracked as running?

[ ... ]

> @@ -9172,13 +9195,15 @@ __bpf_kfunc bool scx_bpf_task_set_slice(struct task_struct *p, u64 slice,
>  		return false;
>  
>  	/*
> -	 * Directly write only when we hold the lock of the rq @p is queued or
> -	 * running on. See the slice write rules above.
> +	 * Directly write only when we hold the lock of the rq @p is queued on or
> +	 * provides the current scheduling context for. Under proxy execution,
> +	 * rq->donor owns and consumes the slice while rq->curr executes on its
> +	 * behalf. See the slice write rules above.
>  	 */
>  	locked_rq = scx_locked_rq();
>  	if (!locked_rq ||
>  	    (READ_ONCE(p->scx.runnable_cpu) != cpu_of(locked_rq) &&
> -	     !task_current(locked_rq, p))) {
> +	     !task_current_donor(locked_rq, p))) {
>  		set_task_slice_oob(sch, p, slice);
>  		return true;
>  	}

This patch updates several task_current() checks to task_current_donor(),
but it appears one was missed in enqueue_task_scx():

kernel/sched/ext/ext.c:enqueue_task_scx() {
    ...
	/*
	 * Restoring a running task 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)) {
		sticky_cpu = cpu_of(rq);
		enq_flags |= SCX_ENQ_IGNORE_CAPS;
	}
    ...
}

[Severity: Medium]
Should this use task_current_donor(rq, p) instead of task_current(rq, p)?

If a blocked proxy donor undergoes priority inheritance, it could be
dequeued and re-enqueued with ENQUEUE_RESTORE. Since the donor is not
physically rq->curr, task_current(rq, p) evaluates to false.

Would this bypass the local DSQ assignment and erroneously push the active
proxy donor into the BPF scheduler instead of the local DSQ?

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