linux-kernel - Re: linux-next: build warnings after merge of the access

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]

Date:	Thu, 26 Mar 2015 09:15:21 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Will Deacon <will.deacon@....com>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Christian Borntraeger <borntraeger@...ibm.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
	"linux-next@...r.kernel.org" <linux-next@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Davidlohr Bueso <dave@...olabs.net>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>
Subject: Re: linux-next: build warnings after merge of the access_once tree

On Thu, Mar 26, 2015 at 7:22 AM, Peter Zijlstra <peterz@...radead.org> wrote:
>
> So we can either just remove the READ_ONCE(), or replace it with a
> leading barrier() call just to be on the paranoid side of things.

NOOO!

> Any preferences?

Not a preference: a _requirement_.

Get rid of the f*cking size checks etc on READ_ONCE() and friends.

They are about - wait for it - "reading a value once".

Note how it doesn't say ANYTHING about "atomic" or anything like that.
It's about reading *ONCE*.

> Something like so, but with a sensible comment I suppose.

Hell f*cking no. The "something like so" is huge and utter crap,
because the barrier is on the wrong side.

> -       old.lock_count = READ_ONCE(lockref->lock_count);                        \
> +       barrier();                                                              \
> +       old.lock_count = lockref->lock_count;                                   \
>         while (likely(arch_spin_value_unlocked(old.lock.rlock.raw_lock))) {     \
>                 struct lockref new = old, prev = old;                           \

The WHOLE point of the READ_ONCE (formerly ACCESS_ONCE) is that it
tells the compiler that it cannot reload the value.

Notice how it is *not* about atomicitiy. The compiler can read the
value in fifteen pieces, randomly mixing one bit or five. Nobody
cares.

But the important thing is that ONCE IT IS READ, it is never read
again. That's the "once" part.

Why is that important? It's important because we have to absolutely
guartantee that the value we *test* is the same value we use later.
That's a common concern with mutable variables, and is the only reason
for READ_ONCE() in the first place.

The whole atomicity etc crap was just that - crap. It was never about
atomicitiy. It was about the compiler not reloading values.

So no. No barriers. No "removal of READ_ONCE". Just get rid of the
broken "sanity" checks in the READ_ONCE implementation that are just
pure garbage.

The checks in ACCESS_ONCE() were because apparently gcc got things
wrong - dropping the volatile - for aggregate types. They were never
supposed to be about atomicity, even if there clearly was some
confusion about that.

Really. Just get rid of the checks - they were wrong. They were
clearly very close to *introducing* a bug, rather than fixing anything
at all.

                             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/