thr3ads.net - Linux Virtualization - [PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock [Feb 2015]

If this information is useful, please help other people find it:
Share via:

Ingo Molnar

2015-Feb-24 14:47 UTC

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

* Greg KH <gregkh at linuxfoundation.org> wrote:
> On Tue, Feb 24, 2015 at 02:54:59PM +0530, Raghavendra K T wrote:
> > Paravirt spinlock clears slowpath flag after doing unlock.
> > As explained by Linus currently it does:
> >                 prev = *lock;
> >                 add_smp(&lock->tickets.head, TICKET_LOCK_INC);
> > 
> >                 /* add_smp() is a full mb() */
> > 
> >                 if (unlikely(lock->tickets.tail &
TICKET_SLOWPATH_FLAG))
> >                         __ticket_unlock_slowpath(lock, prev);
> > 
> > which is *exactly* the kind of things you cannot do with spinlocks,
> > because after you've done the "add_smp()" and released
the spinlock
> > for the fast-path, you can't access the spinlock any more. 
Exactly
> > because a fast-path lock might come in, and release the whole data
> > structure.
> > 
> > Linus suggested that we should not do any writes to lock after
unlock(),
> > and we can move slowpath clearing to fastpath lock.
> > 
> > So this patch implements the fix with:
> > 1. Moving slowpath flag to head (Oleg):
> > Unlocked locks don't care about the slowpath flag; therefore we
can keep
> > it set after the last unlock, and clear it again on the first
(try)lock.
> > -- this removes the write after unlock. note that keeping slowpath
flag would
> > result in unnecessary kicks.
> > By moving the slowpath flag from the tail to the head ticket we also
avoid
> > the need to access both the head and tail tickets on unlock.
> > 
> > 2. use xadd to avoid read/write after unlock that checks the need for
> > unlock_kick (Linus):
> > We further avoid the need for a read-after-release by using xadd;
> > the prev head value will include the slowpath flag and indicate if we
> > need to do PV kicking of suspended spinners -- on modern chips xadd
> > isn't (much) more expensive than an add + load.
> > 
> > Result:
> >  setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
> >  benchmark overcommit %improve
> >  kernbench  1x           -0.13
> >  kernbench  2x            0.02
> >  dbench     1x           -1.77
> >  dbench     2x           -0.63
> > 
> > [Jeremy: hinted missing TICKET_LOCK_INC for kick]
> > [Oleg: Moving slowpath flag to head, ticket_equals idea]
> > [PeterZ: Detailed changelog]
> > 
> > Reported-by: Sasha Levin <sasha.levin at oracle.com>
> > Suggested-by: Linus Torvalds <torvalds at linux-foundation.org>
> > Signed-off-by: Raghavendra K T <raghavendra.kt at
linux.vnet.ibm.com>
> > Reviewed-by: Oleg Nesterov <oleg at redhat.com>
> > Acked-by: David Vrabel <david.vrabel at citrix.com>
> > ---
> >  arch/x86/include/asm/spinlock.h | 94
++++++++++++++++++++---------------------
> >  arch/x86/kernel/kvm.c           |  7 ++-
> >  arch/x86/xen/spinlock.c         |  7 ++-
> >  3 files changed, 58 insertions(+), 50 deletions(-)
> > 
> > Changes for stable:
> >   - Don't replace the ACCESS_ONCE to READ_ONCE which would cause
horraneous
> >     Compiler warnings (Linus, David Vbriel, PeterZ, Ingo)
> 
> What is the git commit id of this in Linus's tree?  What 
> stable tree(s) do you want this applied to?
It's:

 d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock

You'll also need this fix from Linus to avoid (harmless) 
build warnings:

 dd36929720f4 kernel: make READ_ONCE() valid on const arguments

Thanks,

	Ingo

Greg KH

2015-Feb-24 15:20 UTC

head link

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

On Tue, Feb 24, 2015 at 03:47:37PM +0100, Ingo Molnar
wrote:> 
> * Greg KH <gregkh at linuxfoundation.org> wrote:
> 
> > On Tue, Feb 24, 2015 at 02:54:59PM +0530, Raghavendra K T wrote:
> > > Paravirt spinlock clears slowpath flag after doing unlock.
> > > As explained by Linus currently it does:
> > >                 prev = *lock;
> > >                 add_smp(&lock->tickets.head,
TICKET_LOCK_INC);
> > > 
> > >                 /* add_smp() is a full mb() */
> > > 
> > >                 if (unlikely(lock->tickets.tail &
TICKET_SLOWPATH_FLAG))
> > >                         __ticket_unlock_slowpath(lock, prev);
> > > 
> > > which is *exactly* the kind of things you cannot do with
spinlocks,
> > > because after you've done the "add_smp()" and
released the spinlock
> > > for the fast-path, you can't access the spinlock any more. 
Exactly
> > > because a fast-path lock might come in, and release the whole
data
> > > structure.
> > > 
> > > Linus suggested that we should not do any writes to lock after
unlock(),
> > > and we can move slowpath clearing to fastpath lock.
> > > 
> > > So this patch implements the fix with:
> > > 1. Moving slowpath flag to head (Oleg):
> > > Unlocked locks don't care about the slowpath flag; therefore
we can keep
> > > it set after the last unlock, and clear it again on the first
(try)lock.
> > > -- this removes the write after unlock. note that keeping
slowpath flag would
> > > result in unnecessary kicks.
> > > By moving the slowpath flag from the tail to the head ticket we
also avoid
> > > the need to access both the head and tail tickets on unlock.
> > > 
> > > 2. use xadd to avoid read/write after unlock that checks the need
for
> > > unlock_kick (Linus):
> > > We further avoid the need for a read-after-release by using xadd;
> > > the prev head value will include the slowpath flag and indicate
if we
> > > need to do PV kicking of suspended spinners -- on modern chips
xadd
> > > isn't (much) more expensive than an add + load.
> > > 
> > > Result:
> > >  setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
> > >  benchmark overcommit %improve
> > >  kernbench  1x           -0.13
> > >  kernbench  2x            0.02
> > >  dbench     1x           -1.77
> > >  dbench     2x           -0.63
> > > 
> > > [Jeremy: hinted missing TICKET_LOCK_INC for kick]
> > > [Oleg: Moving slowpath flag to head, ticket_equals idea]
> > > [PeterZ: Detailed changelog]
> > > 
> > > Reported-by: Sasha Levin <sasha.levin at oracle.com>
> > > Suggested-by: Linus Torvalds <torvalds at
linux-foundation.org>
> > > Signed-off-by: Raghavendra K T <raghavendra.kt at
linux.vnet.ibm.com>
> > > Reviewed-by: Oleg Nesterov <oleg at redhat.com>
> > > Acked-by: David Vrabel <david.vrabel at citrix.com>
> > > ---
> > >  arch/x86/include/asm/spinlock.h | 94
++++++++++++++++++++---------------------
> > >  arch/x86/kernel/kvm.c           |  7 ++-
> > >  arch/x86/xen/spinlock.c         |  7 ++-
> > >  3 files changed, 58 insertions(+), 50 deletions(-)
> > > 
> > > Changes for stable:
> > >   - Don't replace the ACCESS_ONCE to READ_ONCE which would
cause horraneous
> > >     Compiler warnings (Linus, David Vbriel, PeterZ, Ingo)
> > 
> > What is the git commit id of this in Linus's tree?  What 
> > stable tree(s) do you want this applied to?
> 
> It's:
> 
>  d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock
> 
> You'll also need this fix from Linus to avoid (harmless) 
> build warnings:
> 
>  dd36929720f4 kernel: make READ_ONCE() valid on const arguments
Great.  But what stable kernel trees should it be backported to?  Just
3.19?  Or anything older?

thanks,

greg k-h

Raghavendra K T

2015-Feb-24 18:19 UTC

head link

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

On 02/24/2015 08:17 PM, Ingo Molnar wrote:>
> * Greg KH <gregkh at linuxfoundation.org> wrote:
>
>> On Tue, Feb 24, 2015 at 02:54:59PM +0530, Raghavendra K T wrote:
>>> Paravirt spinlock clears slowpath flag after doing unlock.
>>> As explained by Linus currently it does:
>>>                  prev = *lock;
>>>                  add_smp(&lock->tickets.head,
TICKET_LOCK_INC);
>>>
>>>                  /* add_smp() is a full mb() */
>>>
>>>                  if (unlikely(lock->tickets.tail &
TICKET_SLOWPATH_FLAG))
>>>                          __ticket_unlock_slowpath(lock, prev);
>>>
>>> which is *exactly* the kind of things you cannot do with spinlocks,
>>> because after you've done the "add_smp()" and
released the spinlock
>>> for the fast-path, you can't access the spinlock any more. 
Exactly
>>> because a fast-path lock might come in, and release the whole data
>>> structure.
>>>
>>> Linus suggested that we should not do any writes to lock after
unlock(),
>>> and we can move slowpath clearing to fastpath lock.
>>>
>>> So this patch implements the fix with:
>>> 1. Moving slowpath flag to head (Oleg):
>>> Unlocked locks don't care about the slowpath flag; therefore we
can keep
>>> it set after the last unlock, and clear it again on the first
(try)lock.
>>> -- this removes the write after unlock. note that keeping slowpath
flag would
>>> result in unnecessary kicks.
>>> By moving the slowpath flag from the tail to the head ticket we
also avoid
>>> the need to access both the head and tail tickets on unlock.
>>>
>>> 2. use xadd to avoid read/write after unlock that checks the need
for
>>> unlock_kick (Linus):
>>> We further avoid the need for a read-after-release by using xadd;
>>> the prev head value will include the slowpath flag and indicate if
we
>>> need to do PV kicking of suspended spinners -- on modern chips xadd
>>> isn't (much) more expensive than an add + load.
>>>
>>> Result:
>>>   setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
>>>   benchmark overcommit %improve
>>>   kernbench  1x           -0.13
>>>   kernbench  2x            0.02
>>>   dbench     1x           -1.77
>>>   dbench     2x           -0.63
>>>
>>> [Jeremy: hinted missing TICKET_LOCK_INC for kick]
>>> [Oleg: Moving slowpath flag to head, ticket_equals idea]
>>> [PeterZ: Detailed changelog]
>>>
>>> Reported-by: Sasha Levin <sasha.levin at oracle.com>
>>> Suggested-by: Linus Torvalds <torvalds at
linux-foundation.org>
>>> Signed-off-by: Raghavendra K T <raghavendra.kt at
linux.vnet.ibm.com>
>>> Reviewed-by: Oleg Nesterov <oleg at redhat.com>
>>> Acked-by: David Vrabel <david.vrabel at citrix.com>
>>> ---
>>>   arch/x86/include/asm/spinlock.h | 94
++++++++++++++++++++---------------------
>>>   arch/x86/kernel/kvm.c           |  7 ++-
>>>   arch/x86/xen/spinlock.c         |  7 ++-
>>>   3 files changed, 58 insertions(+), 50 deletions(-)
>>>
>>> Changes for stable:
>>>    - Don't replace the ACCESS_ONCE to READ_ONCE which would
cause horraneous
>>>      Compiler warnings (Linus, David Vbriel, PeterZ, Ingo)
>>
>> What is the git commit id of this in Linus's tree?  What
>> stable tree(s) do you want this applied to?
>
> It's:
>
>   d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock
Yes, This is the original patch. Please note I have taken out the
READ_ONCE changes from the original patch to avoid build warnings
mentioned below.
(Those READ_ONCE changes were cosmetic and was not present in the
previous versions)
>
> You'll also need this fix from Linus to avoid (harmless)
> build warnings:
>
>   dd36929720f4 kernel: make READ_ONCE() valid on const arguments
So this may not be absolutely necessary with the current patch.

Raghavendra K T

2015-Feb-24 18:29 UTC

head link

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

On 02/24/2015 08:50 PM, Greg KH wrote:> On Tue, Feb 24, 2015 at 03:47:37PM +0100, Ingo Molnar wrote:
>>
>> * Greg KH <gregkh at linuxfoundation.org> wrote:
>>
>>> On Tue, Feb 24, 2015 at 02:54:59PM +0530, Raghavendra K T wrote:
>>>> Paravirt spinlock clears slowpath flag after doing unlock.
>>>> As explained by Linus currently it does:
>>>>                  prev = *lock;
>>>>                  add_smp(&lock->tickets.head,
TICKET_LOCK_INC);
>>>>
>>>>                  /* add_smp() is a full mb() */
>>>>
>>>>                  if (unlikely(lock->tickets.tail &
TICKET_SLOWPATH_FLAG))
>>>>                          __ticket_unlock_slowpath(lock, prev);
>>>>
>>>> which is *exactly* the kind of things you cannot do with
spinlocks,
>>>> because after you've done the "add_smp()" and
released the spinlock
>>>> for the fast-path, you can't access the spinlock any more. 
Exactly
>>>> because a fast-path lock might come in, and release the whole
data
>>>> structure.
>>>>
>>>> Linus suggested that we should not do any writes to lock after
unlock(),
>>>> and we can move slowpath clearing to fastpath lock.
>>>>
>>>> So this patch implements the fix with:
>>>> 1. Moving slowpath flag to head (Oleg):
>>>> Unlocked locks don't care about the slowpath flag;
therefore we can keep
>>>> it set after the last unlock, and clear it again on the first
(try)lock.
>>>> -- this removes the write after unlock. note that keeping
slowpath flag would
>>>> result in unnecessary kicks.
>>>> By moving the slowpath flag from the tail to the head ticket we
also avoid
>>>> the need to access both the head and tail tickets on unlock.
>>>>
>>>> 2. use xadd to avoid read/write after unlock that checks the
need for
>>>> unlock_kick (Linus):
>>>> We further avoid the need for a read-after-release by using
xadd;
>>>> the prev head value will include the slowpath flag and indicate
if we
>>>> need to do PV kicking of suspended spinners -- on modern chips
xadd
>>>> isn't (much) more expensive than an add + load.
>>>>
>>>> Result:
>>>>   setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
>>>>   benchmark overcommit %improve
>>>>   kernbench  1x           -0.13
>>>>   kernbench  2x            0.02
>>>>   dbench     1x           -1.77
>>>>   dbench     2x           -0.63
>>>>
>>>> [Jeremy: hinted missing TICKET_LOCK_INC for kick]
>>>> [Oleg: Moving slowpath flag to head, ticket_equals idea]
>>>> [PeterZ: Detailed changelog]
>>>>
>>>> Reported-by: Sasha Levin <sasha.levin at oracle.com>
>>>> Suggested-by: Linus Torvalds <torvalds at
linux-foundation.org>
>>>> Signed-off-by: Raghavendra K T <raghavendra.kt at
linux.vnet.ibm.com>
>>>> Reviewed-by: Oleg Nesterov <oleg at redhat.com>
>>>> Acked-by: David Vrabel <david.vrabel at citrix.com>
>>>> ---
>>>>   arch/x86/include/asm/spinlock.h | 94
++++++++++++++++++++---------------------
>>>>   arch/x86/kernel/kvm.c           |  7 ++-
>>>>   arch/x86/xen/spinlock.c         |  7 ++-
>>>>   3 files changed, 58 insertions(+), 50 deletions(-)
>>>>
>>>> Changes for stable:
>>>>    - Don't replace the ACCESS_ONCE to READ_ONCE which would
cause horraneous
>>>>      Compiler warnings (Linus, David Vbriel, PeterZ, Ingo)
>>>
>>> What is the git commit id of this in Linus's tree?  What
>>> stable tree(s) do you want this applied to?
>>
>> It's:
>>
>>   d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock
>>
>> You'll also need this fix from Linus to avoid (harmless)
>> build warnings:
>>
>>   dd36929720f4 kernel: make READ_ONCE() valid on const arguments
>
> Great.  But what stable kernel trees should it be backported to?  Just
> 3.19?  Or anything older?
My patch was intended only for 3.19.

Though paravirt changes have gone in 3.12, the problem manifested
clearly after some of the completion related changes. but I leave that 
decision to experts here. (I 'll send necessary changes if patch is
needed for older versions because it may not apply cleanly).

Greg KH

2015-Feb-24 18:38 UTC

head link

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

On Tue, Feb 24, 2015 at 11:49:13PM +0530, Raghavendra K T
wrote:> On 02/24/2015 08:17 PM, Ingo Molnar wrote:
> >
> >* Greg KH <gregkh at linuxfoundation.org> wrote:
> >
> >>On Tue, Feb 24, 2015 at 02:54:59PM +0530, Raghavendra K T wrote:
> >>>Paravirt spinlock clears slowpath flag after doing unlock.
> >>>As explained by Linus currently it does:
> >>>                 prev = *lock;
> >>>                 add_smp(&lock->tickets.head,
TICKET_LOCK_INC);
> >>>
> >>>                 /* add_smp() is a full mb() */
> >>>
> >>>                 if (unlikely(lock->tickets.tail &
TICKET_SLOWPATH_FLAG))
> >>>                         __ticket_unlock_slowpath(lock, prev);
> >>>
> >>>which is *exactly* the kind of things you cannot do with
spinlocks,
> >>>because after you've done the "add_smp()" and
released the spinlock
> >>>for the fast-path, you can't access the spinlock any more. 
Exactly
> >>>because a fast-path lock might come in, and release the whole
data
> >>>structure.
> >>>
> >>>Linus suggested that we should not do any writes to lock after
unlock(),
> >>>and we can move slowpath clearing to fastpath lock.
> >>>
> >>>So this patch implements the fix with:
> >>>1. Moving slowpath flag to head (Oleg):
> >>>Unlocked locks don't care about the slowpath flag;
therefore we can keep
> >>>it set after the last unlock, and clear it again on the first
(try)lock.
> >>>-- this removes the write after unlock. note that keeping
slowpath flag would
> >>>result in unnecessary kicks.
> >>>By moving the slowpath flag from the tail to the head ticket we
also avoid
> >>>the need to access both the head and tail tickets on unlock.
> >>>
> >>>2. use xadd to avoid read/write after unlock that checks the
need for
> >>>unlock_kick (Linus):
> >>>We further avoid the need for a read-after-release by using
xadd;
> >>>the prev head value will include the slowpath flag and indicate
if we
> >>>need to do PV kicking of suspended spinners -- on modern chips
xadd
> >>>isn't (much) more expensive than an add + load.
> >>>
> >>>Result:
> >>>  setup: 16core (32 cpu +ht sandy bridge 8GB 16vcpu guest)
> >>>  benchmark overcommit %improve
> >>>  kernbench  1x           -0.13
> >>>  kernbench  2x            0.02
> >>>  dbench     1x           -1.77
> >>>  dbench     2x           -0.63
> >>>
> >>>[Jeremy: hinted missing TICKET_LOCK_INC for kick]
> >>>[Oleg: Moving slowpath flag to head, ticket_equals idea]
> >>>[PeterZ: Detailed changelog]
> >>>
> >>>Reported-by: Sasha Levin <sasha.levin at oracle.com>
> >>>Suggested-by: Linus Torvalds <torvalds at
linux-foundation.org>
> >>>Signed-off-by: Raghavendra K T <raghavendra.kt at
linux.vnet.ibm.com>
> >>>Reviewed-by: Oleg Nesterov <oleg at redhat.com>
> >>>Acked-by: David Vrabel <david.vrabel at citrix.com>
> >>>---
> >>>  arch/x86/include/asm/spinlock.h | 94
++++++++++++++++++++---------------------
> >>>  arch/x86/kernel/kvm.c           |  7 ++-
> >>>  arch/x86/xen/spinlock.c         |  7 ++-
> >>>  3 files changed, 58 insertions(+), 50 deletions(-)
> >>>
> >>>Changes for stable:
> >>>   - Don't replace the ACCESS_ONCE to READ_ONCE which would
cause horraneous
> >>>     Compiler warnings (Linus, David Vbriel, PeterZ, Ingo)
> >>
> >>What is the git commit id of this in Linus's tree?  What
> >>stable tree(s) do you want this applied to?
> >
> >It's:
> >
> >  d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock
> 
> Yes, This is the original patch. Please note I have taken out the
> READ_ONCE changes from the original patch to avoid build warnings
> mentioned below.
> (Those READ_ONCE changes were cosmetic and was not present in the
> previous versions)
> 
> >
> >You'll also need this fix from Linus to avoid (harmless)
> >build warnings:
> >
> >  dd36929720f4 kernel: make READ_ONCE() valid on const arguments
> 
> So this may not be absolutely necessary with the current patch.
I'd prefer to be as close as possible to the upstream patch.  So if
applying both of these patches will work, I'd much rather do that.
Changing patches when backporting them to stable for no good reason than
to clean things up, just confuses everyone involved.

Let's keep our messy history :)

thanks,

greg k-h

Reasonably Related Threads

Search for more possibly parallel threads

Linux Virtualization - Feb 2015 - [PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

[PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock

Reasonably Related Threads