Re: [PATCH] sched/fair: Clear throttle state when leaving the fair class

[Benjamin Segall]

Daehyeon Ko <4ncienth@xxxxxxxxx> writes:

> Task-based CFS bandwidth throttling removes a task's fair sched_entity
> while the scheduler core still considers the task queued. p->throttled
> carries the fair-specific state across dequeue/enqueue operations.
>
> DEQUEUE_CLASS removes the task from cfs_rq->throttled_limbo_list
> but retains the bit. If the last finite CFS quota is disabled while
> the task runs in another class, it is absent from the unthrottle walk and
> the bit survives.
> Returning to fair while cfs_bandwidth_used() is false then performs a real
> fair enqueue with p->throttled still set.
>
> After bandwidth is re-enabled, a class-change dequeue treats the
> stale bit as limbo state and skips __dequeue_task(). p->se.on_rq therefore
> remains set, and put_prev_task_fair() places the entity in the EEVDF tree
> while the task moves to RT. On exit, the fair picker can select the
> TASK_DEAD task. __schedule() then returns to do_task_dead(),
> which hits BUG().
>
> Keep p->throttled across migrations and group moves, but clear it on
> DEQUEUE_CLASS because fair-specific state must not outlive fair-class
> membership. A later return to fair uses the normal enqueue path. That path
> still respects a throttled cfs_rq.
>
> A KASAN, LOCKDEP and DEBUG_LIST v7.3 kernel reproduced the do_task_dead()
> BUG in 3/3 root and 3/3 delegated uid-1000 runs with CapPrm=CapEff=0 and
> RLIMIT_RTPRIO=1. The fix passed both matrices plus a 3/3 class-switch
> control with bandwidth continuously enabled, without a scheduler warning,
> KASAN report, lockdep splat or crash.
>
> An AI coding assistant analyzed the state mismatch and drafted the initial
> reproducer, fix candidate and this changelog. The source paths, trigger,
> controls and results were rechecked against the cited revisions.
>
> Fixes: e1fad12dcb66 ("sched/fair: Switch to task based throttle model")
> Signed-off-by: Daehyeon Ko <4ncienth@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---

Yep, sched_class switch lets you smuggle ->throttled into
!cfs_bandwidth_used(), and then that check on enqueue_throttled lets you
smuggle it onto actually being enqueued, which leads to corruption.
That's fun.

I think we need to use switched_from_fair though - if we are currently
TASK_ON_RQ_MIGRATING, sched_class changes will skip going into
dequeue_task_fair at all, so we will not see the DEQUEUE_CLASS.
Triggering a migration on a throttled task can still happen I'm fairly
sure.

It's too bad, since it would be nice to just add to this existing
touchpoint, but it's at least it's trivial:

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index cfc982205b98c..db57ac52eb7f7 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -15500,10 +15500,14 @@ static void switching_from_fair(struct rq *rq, struct task_struct *p)
 }
 
 static void switched_from_fair(struct rq *rq, struct task_struct *p)
 {
 	detach_task_cfs_rq(p);
+
+#ifdef CONFIG_CFS_BANDWIDTH
+	p->throttled = false;
+#endif
 }
 
 static void switched_to_fair(struct rq *rq, struct task_struct *p)
 {
 	WARN_ON_ONCE(p->se.sched_delayed);


(Or we could pull it out into its own function for cleanliness if it
seems worthwhile)