linux-kernel - Re: [PATCH] sched: hoist ASSERT_EXCLUSIVE_WRITER(p->on_rq) above WRITE

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: <20241115095847.GV22801@noisy.programming.kicks-ass.net>
Date: Fri, 15 Nov 2024 10:58:47 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Phil Auld <pauld@...hat.com>
Cc: Jon Kohler <jon@...anix.com>, Ingo Molnar <mingo@...hat.com>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Valentin Schneider <vschneid@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sched: hoist ASSERT_EXCLUSIVE_WRITER(p->on_rq) above
 WRITE_ONCE

On Thu, Nov 14, 2024 at 02:20:56PM -0500, Phil Auld wrote:

> I don't know. I don't think it matters much since the assert is really
> independent of the actual write. Like I said it makes sense to have it
> first to me but others may see it as just moving code around for no strong
> reason.  Peter may or may not decide to pick this one up. Other "mis-ordered"
> uses are in code maintained by different folks.
> 
> You can see if anyone else weighs in...

So I'm not entirely about this patch... :-)

Per commit b55945c500c5 ("sched: Fix pick_next_task_fair() vs try_to_wake_up() race")
we can see that this placement is not equivalent.

Placing it before the store means that nobody else will store to it
until you've done your store.

Placing it after means that nobody else will attempt the store,
irrespective of the store you just did.

That is, the ASSERT after the store is a stronger assert.

In case of activate_task(), we mark the task as on_rq, and this state is
protected by rq->lock (which we hold), so there must not be any stores
for as long as we hold that lock. So after is the right place.

In case of deactivate_task(), this is a hand-off, but the handoff
doesn't happen until after dequeue_task() and set_task_cpu(). So at this
point, again, nobody should be modifying it.