linux-kernel - Re: [PATCH 2/9] rcu: Fixup noinstr warnings

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: <20200604080512.GA2587@hirez.programming.kicks-ass.net>
Date:   Thu, 4 Jun 2020 10:05:12 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Paul E. McKenney" <paulmck@...nel.org>
Cc:     tglx@...utronix.de, x86@...nel.org, elver@...gle.com,
        kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org,
        will@...nel.org, dvyukov@...gle.com, glider@...gle.com,
        andreyknvl@...gle.com
Subject: Re: [PATCH 2/9] rcu: Fixup noinstr warnings

On Wed, Jun 03, 2020 at 08:34:09PM -0700, Paul E. McKenney wrote:
> On Wed, Jun 03, 2020 at 07:13:20PM +0200, Peter Zijlstra wrote:
> > On Wed, Jun 03, 2020 at 09:46:00AM -0700, Paul E. McKenney wrote:

> > > > @@ -313,7 +313,7 @@ static __always_inline bool rcu_dynticks
> > > >  {
> > > >  	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
> > > >  
> > > > -	return !(atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > > > +	return !(arch_atomic_read(&rdp->dynticks) & RCU_DYNTICK_CTRL_CTR);
> > 
> > The above is actually instrumented by KCSAN, due to arch_atomic_read()
> > being a READ_ONCE() and it now understanding volatile.
> > 
> > > Also instrument_atomic_write(&rdp->dynticks, sizeof(rdp->dynticks)) as
> 
> Right, this should instead be instrument_read(...).
> 
> Though if KCSAN is unconditionally instrumenting volatile, how does
> this help?  Or does KCSAN's instrumentation of volatile somehow avoid
> causing trouble?

As Marco already explained, when used inside noinstr no instrumentation
will be emitted, when used outside noinstr it will emit the right
instrumentation.

> > > o	In theory in rcu_irq_exit_preempt(), but as this generates code
> > > 	only in lockdep builds, it might not be worth worrying about.
> > > 
> > > o	Ditto for rcu_irq_exit_check_preempt().
> > > 
> > > o	Ditto for __rcu_irq_enter_check_tick().
> > 
> > Not these, afaict they're all the above arch_atomic_read(), which is
> > instrumented due to volatile in these cases.

I this case, the above call-sites are all not noinstr (double negative!)
and will thus cause instrumentation to be emitted.

This is all a 'special' case for arch_atomic_read() (and _set()),
because they're basically READ_ONCE() (and WRITE_ONCE() resp.). The
normal atomics are asm() and it doesn't do anything for those (although
I suppose clang could, since it has this internal assembler to parse the
inline asm, but afaiu that's not something GCC ever wants to do).