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: <20241115142658.GA837020@pauld.westford.csb>
Date: Fri, 15 Nov 2024 09:26:58 -0500
From: Phil Auld <pauld@...hat.com>
To: Peter Zijlstra <peterz@...radead.org>
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 Fri, Nov 15, 2024 at 10:58:47AM +0100 Peter Zijlstra wrote:
> 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... :-)
>

I'm not entirely either :)


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

Subtle. That kind of thing (someone else acting on the write) was my
hesitation but I couldn't quite wrap my head around it.

That's why we have other people weigh in ...

Thanks!

> 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.

Presumably until some barrier or release at least, right?

I've got to dig into the kcsan stuff a bit more. It looks really
interesting.


Cheers,
Phil

> 
> 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.
> 
> 

--