linux-kernel - Re: [PATCH] sched/core: Fix forceidle balancing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ykbk4MSSNSxsQoMs@hirez.programming.kicks-ass.net>
Date:   Fri, 1 Apr 2022 13:41:20 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Ingo Molnar <mingo@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        vincent.guittot@...aro.org
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        joel@...lfernandes.org, dietmar.eggemann@....com,
        bsegall@...gle.com, mgorman@...e.de, bristot@...hat.com
Subject: Re: [PATCH] sched/core: Fix forceidle balancing

On Wed, Mar 30, 2022 at 06:05:35PM +0200, Peter Zijlstra wrote:
> 
> Steve reported that ChromeOS encounters the forceidle balancer being
> ran from rt_mutex_setprio()'s balance_callback() invocation and
> explodes.
> 
> Now, the forceidle balancer gets queued every time the idle task gets
> selected, set_next_task(), which is strictly too often.
> rt_mutex_setprio() also uses set_next_task() in the 'change' pattern:
> 
> 	queued = task_on_rq_queued(p); /* p->on_rq == TASK_ON_RQ_QUEUED */
> 	running = task_current(rq, p); /* rq->curr == p */
> 
> 	if (queued)
> 		dequeue_task(...);
> 	if (running)
> 		put_prev_task(...);
> 
> 	/* change task properties */
> 
> 	if (queued)
> 		enqueue_task(...);
> 	if (running)
> 		set_next_task(...);
> 
> However, rt_mutex_setprio() will explicitly not run this pattern on
> the idle task (since priority boosting the idle task is quite insane).
> Most other 'change' pattern users are pidhash based and would also not
> apply to idle.
> 
> Also, the change pattern doesn't contain a __balance_callback()
> invocation and hence we could have an out-of-band balance-callback,
> which *should* trigger the WARN in rq_pin_lock() (which guards against
> this exact anti-pattern).
> 
> So while none of that explains how this happens, it does indicate that
> having it in set_next_task() might not be the most robust option.
> 
> Instead, explicitly queue the forceidle balancer from pick_next_task()
> when it does indeed result in forceidle selection. Having it here,
> ensures it can only be triggered under the __schedule() rq->lock
> instance, and hence must be ran from that context.
> 
> This also happens to clean up the code a little, so win-win.

So I couldn't figure out how this could happen without triggering other
warnings, because as I mentioned elsewhere, commit 565790d28b1e ("sched:
Fix balance_callback()") should've caused a different splat.

But then Dietmar reminded me that ChromeOS is probably running some
ancient crud with backports on :/ and will most likely not have that
commit.