Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493541 > unrolled thread

Re: [PATCH locking/Documentation 1/2] Add note of release-acquire store vulnerability

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-29 18:00 +0200
    Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Will Deacon <will.deacon@arm.com> - 2016-09-29 18:10 +0200
      Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-29 18:20 +0200
        Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-29 18:50 +0200
      Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-29 18:50 +0200
        Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Will Deacon <will.deacon@arm.com> - 2016-09-29 19:20 +0200
          Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-29 19:30 +0200
            Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-29 20:10 +0200
              Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-29 20:20 +0200
                Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-29 20:50 +0200
                  Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-29 21:20 +0200
                    Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Alan Stern <stern@rowland.harvard.edu> - 2016-09-29 21:40 +0200
                      Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-29 22:30 +0200
                      Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-30 11:00 +0200
                    Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-30 11:10 +0200
                    Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-30 12:00 +0200
                      Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-30 14:20 +0200
                        Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-30 15:00 +0200
                          Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-30 15:40 +0200
            Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Boqun Feng <boqun.feng@gmail.com> - 2016-09-30 08:00 +0200
              Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Will Deacon <will.deacon@arm.com> - 2016-09-30 11:30 +0200
                Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-30 13:40 +0200
        Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-30 12:30 +0200
          Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-30 14:20 +0200
            Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability Peter Zijlstra <peterz@infradead.org> - 2016-09-30 14:50 +0200
              Re: [PATCH locking/Documentation 1/2] Add note of release-acquire  store vulnerability "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-09-30 15:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1493985

On Fri, Sep 30, 2016 at 01:53:52PM +0800, Boqun Feng wrote:
> On Thu, Sep 29, 2016 at 10:23:22AM -0700, Paul E. McKenney wrote:
> > If two processes are related by a RELEASE+ACQUIRE pair, ordering can be
> > broken if a third process overwrites the value written by the RELEASE
> > operation before the ACQUIRE operation has a chance of reading it, for
> > example:
> > 
> > 	P0(int *x, int *y)
> > 	{
> > 		WRITE_ONCE(*x, 1);
> > 		smp_wmb();
>                ^^^^^^^^^^^
> 
> What is this smp_wmb() for?
> 
> > 		smp_store_release(y, 1);
> > 	}
> > 
> > 	P1(int *y)
> > 	{
> > 		WRITE_ONCE(*y, 2);
> 
> If we change this WRITE_ONCE to a relaxed atomic operation(e.g.
> xchg_relaxed(y, 2)), both herd and ppcmem said the exist-clause "y = 2
> /\ 2:r1 = 2 /\ 2:r2 = 0" wouldn't be triggered on PPC.
> 
> I guess we will get the same behavior on ARM/ARM64, Will?
> 
> If a normal store could break chain, while a RmW atomic won't, do we
> want to call it out in the document and build our memory model around
> this?

I think this is required to work by C11's definition of release sequences,
so any architecture that claims to support those with the same instructions
will need this to be forbidden.

Personally, I think that's a bug in C11, because I think it goes too far
in forbidding some hardware optimisations around relaxed xchg, but it is
what it is.

Will

[toc] | [prev] | [next] | [standalone]

#1494035

On Fri, Sep 30, 2016 at 10:20:09AM +0100, Will Deacon wrote:
> On Fri, Sep 30, 2016 at 01:53:52PM +0800, Boqun Feng wrote:
> > On Thu, Sep 29, 2016 at 10:23:22AM -0700, Paul E. McKenney wrote:
> > > If two processes are related by a RELEASE+ACQUIRE pair, ordering can be
> > > broken if a third process overwrites the value written by the RELEASE
> > > operation before the ACQUIRE operation has a chance of reading it, for
> > > example:
> > > 
> > > 	P0(int *x, int *y)
> > > 	{
> > > 		WRITE_ONCE(*x, 1);
> > > 		smp_wmb();
> >                ^^^^^^^^^^^
> > 
> > What is this smp_wmb() for?

Seems redundant to me, now that you mention it.  ;-)

But maybe this does something on ARM?

> > > 		smp_store_release(y, 1);
> > > 	}
> > > 
> > > 	P1(int *y)
> > > 	{
> > > 		WRITE_ONCE(*y, 2);
> > 
> > If we change this WRITE_ONCE to a relaxed atomic operation(e.g.
> > xchg_relaxed(y, 2)), both herd and ppcmem said the exist-clause "y = 2
> > /\ 2:r1 = 2 /\ 2:r2 = 0" wouldn't be triggered on PPC.
> > 
> > I guess we will get the same behavior on ARM/ARM64, Will?
> > 
> > If a normal store could break chain, while a RmW atomic won't, do we
> > want to call it out in the document and build our memory model around
> > this?
> 
> I think this is required to work by C11's definition of release sequences,
> so any architecture that claims to support those with the same instructions
> will need this to be forbidden.

Yes.

> Personally, I think that's a bug in C11, because I think it goes too far
> in forbidding some hardware optimisations around relaxed xchg, but it is
> what it is.

The idea at the time (2007 or thereabouts) was that the atomic operation
would have a hard time breaking the causal chain.  To your point, atomic
xchg could presumably update the value and figure out what the previous
value was after the fact.  Maybe we should try to get the committee to
relax the requirement for relaxed xchg, though backwards compatibility
will make that a tough sell.  Might need a new xchg API.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1494016

On Thu, Sep 29, 2016 at 09:43:53AM -0700, Paul E. McKenney wrote:
> If two processes are related by a RELEASE+ACQUIRE pair, ordering can be
> broken if a third process overwrites the value written by the RELEASE
> operation before the ACQUIRE operation has a chance of reading it, for
> example:
> 
> 	P0(int *x, int *y)
> 	{
> 		WRITE_ONCE(*x, 1);
> 		smp_store_release(y, 1);
> 	}
> 
> 	P1(int *y)
> 	{
> 		smp_store_release(y, 2);
> 	}
> 
> 	P2(int *x, int *y)
> 	{
> 		r1 = smp_load_acquire(y);
> 		r2 = READ_ONCE(*x);
> 	}
> 
> Both ARM and powerpc allow the "after the dust settles" outcome (r1=2 &&
> r2=0), as does the current version of the early prototype Linux-kernel
> memory model.
> 
> This commit therefore updates the documentation to call this vulnerability
> out explicitly.

So its a pretty dumb thing to do in any case (and yes the kernel does
this). Its also entirely expected in my book, that if you generate
conflicting writes on a release, ordering is out the window.

Why do we need to call this out? Who in his right mind would want to do
this and expect anything other than wreckage?

Not that we're not having too much 'fun' discussing this,.. but I do
wonder why we need to call this out.

[toc] | [prev] | [next] | [standalone]

#1494063

On Fri, Sep 30, 2016 at 12:25:16PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 29, 2016 at 09:43:53AM -0700, Paul E. McKenney wrote:
> > If two processes are related by a RELEASE+ACQUIRE pair, ordering can be
> > broken if a third process overwrites the value written by the RELEASE
> > operation before the ACQUIRE operation has a chance of reading it, for
> > example:
> > 
> > 	P0(int *x, int *y)
> > 	{
> > 		WRITE_ONCE(*x, 1);
> > 		smp_store_release(y, 1);
> > 	}
> > 
> > 	P1(int *y)
> > 	{
> > 		smp_store_release(y, 2);
> > 	}
> > 
> > 	P2(int *x, int *y)
> > 	{
> > 		r1 = smp_load_acquire(y);
> > 		r2 = READ_ONCE(*x);
> > 	}
> > 
> > Both ARM and powerpc allow the "after the dust settles" outcome (r1=2 &&
> > r2=0), as does the current version of the early prototype Linux-kernel
> > memory model.
> > 
> > This commit therefore updates the documentation to call this vulnerability
> > out explicitly.
> 
> So its a pretty dumb thing to do in any case (and yes the kernel does
> this). Its also entirely expected in my book, that if you generate
> conflicting writes on a release, ordering is out the window.
> 
> Why do we need to call this out? Who in his right mind would want to do
> this and expect anything other than wreckage?
> 
> Not that we're not having too much 'fun' discussing this,.. but I do
> wonder why we need to call this out.

You lost me on this one...  If no one does this, sure, we can leave it out.
But if some part of the kernel does rely on this, we should call it out as
forbidden.  And fix the kernel, of course.

Or am I missing your point?

								Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1494079

On Fri, Sep 30, 2016 at 05:17:21AM -0700, Paul E. McKenney wrote:
> On Fri, Sep 30, 2016 at 12:25:16PM +0200, Peter Zijlstra wrote:

> > So its a pretty dumb thing to do in any case (and yes the kernel does
> > this). Its also entirely expected in my book, that if you generate
> > conflicting writes on a release, ordering is out the window.
> > 
> > Why do we need to call this out? Who in his right mind would want to do
> > this and expect anything other than wreckage?
> > 
> > Not that we're not having too much 'fun' discussing this,.. but I do
> > wonder why we need to call this out.
> 
> You lost me on this one...  If no one does this, sure, we can leave it out.
> But if some part of the kernel does rely on this, we should call it out as
> forbidden.  And fix the kernel, of course.

Well, the kernel does this, but doesn't rely on ordering. Do "git grep
zap_locks". Its disgusting, can (and does) fail and generally is a sign
of badly broken code (printk is all that).

> Or am I missing your point?

My point was, its obvious crack, anybody doing this needs to have his
head examined. Then again, maybe we should just say that :-)

[toc] | [prev] | [next] | [standalone]

#1494095

On Fri, Sep 30, 2016 at 02:45:42PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 30, 2016 at 05:17:21AM -0700, Paul E. McKenney wrote:
> > On Fri, Sep 30, 2016 at 12:25:16PM +0200, Peter Zijlstra wrote:
> 
> > > So its a pretty dumb thing to do in any case (and yes the kernel does
> > > this). Its also entirely expected in my book, that if you generate
> > > conflicting writes on a release, ordering is out the window.
> > > 
> > > Why do we need to call this out? Who in his right mind would want to do
> > > this and expect anything other than wreckage?
> > > 
> > > Not that we're not having too much 'fun' discussing this,.. but I do
> > > wonder why we need to call this out.
> > 
> > You lost me on this one...  If no one does this, sure, we can leave it out.
> > But if some part of the kernel does rely on this, we should call it out as
> > forbidden.  And fix the kernel, of course.
> 
> Well, the kernel does this, but doesn't rely on ordering. Do "git grep
> zap_locks". Its disgusting, can (and does) fail and generally is a sign
> of badly broken code (printk is all that).

;-) ;-) ;-)

> > Or am I missing your point?
> 
> My point was, its obvious crack, anybody doing this needs to have his
> head examined. Then again, maybe we should just say that :-)

I do find that as I get older, emphatically stating the obvious becomes
an increasingly large fraction of my role, so agreed.  ;-)

							Thanx, Paul

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web