linux-kernel - Re: [PATCH for-4.17 2/2] powerpc: Remove smp_mb() from arch_spin_is

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: <1522150386.7364.53.camel@kernel.crashing.org>
Date:   Tue, 27 Mar 2018 22:33:06 +1100
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     Andrea Parri <andrea.parri@...rulasolutions.com>
Cc:     Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH for-4.17 2/2] powerpc: Remove smp_mb() from
 arch_spin_is_locked()

On Tue, 2018-03-27 at 12:25 +0200, Andrea Parri wrote:
> > I would rather wait until it is properly documented. Debugging that IPC
> > problem took a *LOT* of time and energy, I wouldn't want these issues
> > to come and bite us again.
> 
> I understand. And I'm grateful for this debugging as well as for the (IMO)
> excellent account of it you provided in 51d7d5205d338.
> 
> Said this ;) I cannot except myself from saying that I would probably have
> resisted that solution (adding an smp_mb() in my arch_spin_is_locked), and
> instead "blamed"/suggested that caller to fix his memory ordering...

This is debatable. I tend go for the conservative approach assuming
most people using fairly high level APIs such as spin_is_locked() are
not fully cognisant of the subtleties of our memory model.

After all, it works on x86 ...

The fact that the load can leak into the internals of spin_lock()
implementation and re-order with the lock word load itself is quite
hard to fathom for somebody who doesn't have years of experience
dealing with that sort of issue.

So people will get it wrong.

So unless it's very performance sensitive, I'd rather have things like
spin_is_locked be conservative by default and provide simpler ordering
semantics.

Cheers,
Ben.