linux-kernel - Re: [PATCH 1/5] KVM: x86: avoid atomic operations on APICv vmentry

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, 27 Oct 2016 00:50:46 +0300
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Nadav Amit <nadav.amit@...il.com>,
        LKML <linux-kernel@...r.kernel.org>, KVM <kvm@...r.kernel.org>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Yang Zhang <yang.zhang.wz@...il.com>,
        feng wu <feng.wu@...el.com>, Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH 1/5] KVM: x86: avoid atomic operations on APICv vmentry

On Wed, Oct 19, 2016 at 04:45:48AM -0700, Paul E. McKenney wrote:
> On Sun, Oct 16, 2016 at 05:29:24AM +0300, Michael S. Tsirkin wrote:
> > On Sat, Oct 15, 2016 at 03:47:45AM -0400, Paolo Bonzini wrote:
> > > 
> > > > > On Oct 14, 2016, at 11:56 AM, Paolo Bonzini <pbonzini@...hat.com> wrote:
> > > > >>> 
> > > > >>> 	for (i = 0; i <= 7; i++) {
> > > > >>> -		pir_val = xchg(&pir[i], 0);
> > > > >>> -		if (pir_val)
> > > > >>> +		pir_val = READ_ONCE(pir[i]);
> > > > >> 
> > > > >> Out of curiosity, do you really need this READ_ONCE?
> > > > > 
> > > > > The answer can only be "depends on the compiler's whims". :)
> > > > > If you think of READ_ONCE as a C11 relaxed atomic load, then yes.
> > > > 
> > > > Hm.. So the idea is to make the code "race-free” in the sense
> > > > that every concurrent memory access is done using READ_ONCE/WRITE_ONCE?
> > > > 
> > > > If that is the case, I think there are many other cases that need to be
> > > > changed, for example apic->irr_pending and vcpu->arch.pv.pv_unhalted.
> > > 
> > > There is no documentation for this in the kernel tree unfortunately.
> > > But yes, I think we should do that.  Using READ_ONCE/WRITE_ONCE around
> > > memory barriers is a start.
> > > 
> > > Paolo
> > 
> > I'm beginning to think that if a value is always (maybe except for init
> > where we don't much care about the code size anyway) accessed through
> > *_ONCE macros, we should just mark it volatile and be done with it. The
> > code will look cleaner, and there will be less space for errors
> > like forgetting *_ONCE macros.
> > 
> > Would such code (where all accesses are done through
> > READ_ONCE/WRITE_ONCE otherwise) be an exception to
> > volatile-considered-harmful.txt rules?
> > 
> > Cc Paul and Jonathan (for volatile-considered-harmful.txt).
> 
> One concern would be the guy reading the code, to whom it might not
> be obvious that the underlying access was volatile, especially if
> the reference was a few levels down.
> 
> 							Thanx, Paul

So the guy reading the code will think access is unsafe, check it up and
realise it's fine.  Is this a big deal?  Isn't it better than the
reverse where one forgets to use READ_ONCE and gets a runtime bug?

My point is that this text:

	The key point to understand with regard to volatile is that its purpose is
	to suppress optimization, which is almost never what one really wants to do.

doesn't seem to apply anymore since we have hundreds of users of
READ_ONCE the point of which is exactly to suppress optimization.

I'm guessing this was written when we only had barrier() which is quite
unlike READ_ONCE in that it's not tied to a specific memory reference.

-- 
MST