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: <20241114192056.GI471026@pauld.westford.csb>
Date: Thu, 14 Nov 2024 14:20:56 -0500
From: Phil Auld <pauld@...hat.com>
To: Jon Kohler <jon@...anix.com>
Cc: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
	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 07:01:13PM +0000 Jon Kohler wrote:
> 
> 
> > On Nov 14, 2024, at 1:57 PM, Phil Auld <pauld@...hat.com> wrote:
> > 
> > !-------------------------------------------------------------------|
> >  CAUTION: External Email
> > 
> > |-------------------------------------------------------------------!
> > 
> > On Thu, Nov 14, 2024 at 09:53:52AM -0700 Jon Kohler wrote:
> >> In {activate|deactivate}_task(), hoist ASSERT_EXCLUSIVE_WRITER() to be
> >> above WRITE_ONCE(p->on_rq), which matches the ordering listed in the
> >> KCSAN documentation, kcsan-checks.h code comments, and the usage
> >> pattern we already have in __block_task().
> >> 
> >> Signed-off-by: Jon Kohler <jon@...anix.com>
> >> ---
> >> kernel/sched/core.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index a1c353a62c56..80a04c36b495 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -2066,16 +2066,16 @@ void activate_task(struct rq *rq, struct task_struct *p, int flags)
> >> 
> >> enqueue_task(rq, p, flags);
> >> 
> >> - WRITE_ONCE(p->on_rq, TASK_ON_RQ_QUEUED);
> >> ASSERT_EXCLUSIVE_WRITER(p->on_rq);
> >> + WRITE_ONCE(p->on_rq, TASK_ON_RQ_QUEUED);
> >> }
> >> 
> >> void deactivate_task(struct rq *rq, struct task_struct *p, int flags)
> >> {
> >> SCHED_WARN_ON(flags & DEQUEUE_SLEEP);
> >> 
> >> - WRITE_ONCE(p->on_rq, TASK_ON_RQ_MIGRATING);
> >> ASSERT_EXCLUSIVE_WRITER(p->on_rq);
> >> + WRITE_ONCE(p->on_rq, TASK_ON_RQ_MIGRATING);
> >> 
> >> /*
> >> * Code explicitly relies on TASK_ON_RQ_MIGRATING begin set *before*
> >> -- 
> >> 2.43.0
> >> 
> >> 
> > 
> > This looks fine to me and it makes sense to have the assert before the
> > write.  A quick grep showed that this is by no means a universal pattern
> > at the moment.
> > 
> 
> I’d have to imaging having the assert before must be the right way to
> do this, just from a logic control flow perspective. I’m happy to fix ’the
> others', or do you think I should let them sit there?
>

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

Cheers,
Phil

> > 
> > Reviewed-by: Phil Auld <pauld@...hat.com>
> > 
> > 
> > Cheers,
> > Phil
> > 
> > -- 
> > 
> 

--