linux-kernel - Re: [PATCH] sched/fair: Add null pointer check to pick_next

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: <20250422133620.GF14170@noisy.programming.kicks-ass.net>
Date: Tue, 22 Apr 2025 15:36:20 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Omar Sandoval <osandov@...ndov.com>
Cc: Rik van Riel <riel@...riel.com>, Chris Mason <clm@...a.com>,
	Pat Cody <pat@...cody.io>, mingo@...hat.com, juri.lelli@...hat.com,
	vincent.guittot@...aro.org, dietmar.eggemann@....com,
	rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
	vschneid@...hat.com, linux-kernel@...r.kernel.org, patcody@...a.com,
	kernel-team@...a.com, Breno Leitao <leitao@...ian.org>
Subject: Re: [PATCH] sched/fair: Add null pointer check to pick_next_entity()

On Fri, Apr 18, 2025 at 04:49:08PM -0700, Omar Sandoval wrote:
> On Fri, Apr 18, 2025 at 05:44:38PM +0200, Peter Zijlstra wrote:

> > @@ -850,6 +811,7 @@ RB_DECLARE_CALLBACKS(static, min_vruntim
> >  static void __enqueue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  {
> >  	avg_vruntime_add(cfs_rq, se);
> > +	update_zero_vruntime(cfs_rq);
> 
> Won't this double-count cfs_rq->curr in the avg_vruntime() calculation
> in update_zero_vruntime()? When cfs_rq->curr->on_rq, put_prev_entity()
> calls this with se == cfs_rq->curr _before_ setting cfs_rq->curr to
> NULL.

Yep, right you are. I note that set_next_entity() sets cfs_rq->curr
late, and does not have this issue. I'll look at making
put_prev_entity() clear cfs_rq->curr early.

(obviously I'll have to check all that it does after is not using curr)

> So the avg_vruntime_add() call will add entity_key(cfs_rq->curr)
> to cfs_rq->avg_vruntime and se_weight(cfs_rq->curr) to cfs_rq->avg_load.
> Then update_zero_vruntime() calls avg_vruntime(), which still sees
> curr->on_rq and will add curr's key and weight again. This throws
> zero_vruntime off, maybe by enough to bust zero_vruntime and/or
> avg_vruntime?
> 
> Should the call to update_zero_vruntime() go before avg_vruntime_add()?

The thing is that adding/removing from the tree makes avg_vruntime jump
around a bit, and I wanted to adjust for that jump (in the lazy way).

So it needs to be after enqueue/dequeue.



Meanwhile, I've done a custom module that does:

	preempt_disable();
	/* 'spin' for a minute */
	for (int i = 0; i < 60*1000; i++)
		__ndelay(1000000);
	preempt_enable();

just to emulate some ridiculous PREEMPT_NONE kernel section, and while
it trips RCU and Soft Lockup watchdogs, it does not seem to trip any of
the __builtin_overflow bits, even when ran at nice 19.

(this was with the zero_vruntime thing on, I've yet to try with the
upstream min_vruntime thing)

So unless you've disabled those watchdogs (or pushed their limits up),
I'm fairly confident that there's no overflow to be had, even with that
curr issue.