[Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

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_

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/06/2015 09:49 AM, Raghavendra K T wrote: > static __always_inline void arch_spin_unlock(arch_spinlock_t *lock) > { > if (TICKET_SLOWPATH_FLAG && > - static_key_false(¶virt_ticketlocks_enabled)) { > - arch_spinlock_t prev; > + static_key_false(¶virt_tick

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Fri, Feb 6, 2015 at 7:20 AM, Sasha Levin wrote: > > Can we modify it slightly to avoid potentially accessing invalid memory: So I think there's a race with that. And I'll warn you: the kernel does do speculative reads of memory that might be invalid, not just in places like this. See the comm

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T wrote: > Paravirt spinlock clears slowpath flag after doing unlock. [ fix edited out ] So I'm not going to be applying this for 3.19, because it's much too late and the patch is too scary. Plus the bug probably effectively never shows up in real lif

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/06/2015 07:15 PM, Linus Torvalds wrote: > On Fri, Feb 6, 2015 at 7:20 AM, Sasha Levin wrote: >> >> Can we modify it slightly to avoid potentially accessing invalid memory: > > So I think there's a race with that. > > And I'll warn you: the kernel does do speculative reads of memory that >

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/06/2015 09:49 AM, 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() */

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Fri, 2015-02-06 at 08:25 -0800, Linus Torvalds wrote: > On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T > wrote: > > Paravirt spinlock clears slowpath flag after doing unlock. > [ fix edited out ] > > So I'm not going to be applying this for 3.19, because it's much too > late and the patch is

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/06/2015 02:42 PM, Davidlohr Bueso wrote: > On Fri, 2015-02-06 at 08:25 -0800, Linus Torvalds wrote: >> On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T >> wrote: >>> Paravirt spinlock clears slowpath flag after doing unlock. >> [ fix edited out ] >> >> So I'm not going to be applying this for

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Fri, 2015-02-06 at 16:15 -0500, Sasha Levin wrote: > On 02/06/2015 02:42 PM, Davidlohr Bueso wrote: > > On Fri, 2015-02-06 at 08:25 -0800, Linus Torvalds wrote: > >> On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T > >> wrote: > >>> Paravirt spinlock clears slowpath flag after doing unlock. > >>

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/06, Sasha Levin wrote: > > Can we modify it slightly to avoid potentially accessing invalid memory: > > diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h > index 5315887..cd22d73 100644 > --- a/arch/x86/include/asm/spinlock.h > +++ b/arch/x86/include/asm/spinlock

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/06/2015 09:55 PM, Linus Torvalds wrote: On Fri, Feb 6, 2015 at 6:49 AM, Raghavendra K T wrote: Paravirt spinlock clears slowpath flag after doing unlock. [ fix edited out ] So I'm not going to be applying this for 3.19, because it's much too late and the patch is too scary. Plus the bug

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/07/2015 12:27 AM, Sasha Levin wrote: On 02/06/2015 09:49 AM, 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);

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/06/2015 06:49 AM, 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() */

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/09/2015 02:44 AM, Jeremy Fitzhardinge wrote: On 02/06/2015 06:49 AM, Raghavendra K T wrote: [...] Linus suggested that we should not do any writes to lock after unlock(), and we can move slowpath clearing to fastpath lock. Yep, that seems like a sound approach. Current approach seem

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: > So we have 3 choices, > 1. xadd > 2. continue with current approach. > 3. a read before unlock and also after that. For the truly paranoid we have probe_kernel_address(), suppose the lock was in module space and the module just got

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/09/2015 05:32 PM, Peter Zijlstra wrote: On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: So we have 3 choices, 1. xadd 2. continue with current approach. 3. a read before unlock and also after that. For the truly paranoid we have probe_kernel_address(), suppose the lock w

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Mon, Feb 9, 2015 at 4:02 AM, Peter Zijlstra wrote: > On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: >> So we have 3 choices, >> 1. xadd >> 2. continue with current approach. >> 3. a read before unlock and also after that. > > For the truly paranoid we have probe_kernel_address

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/10/2015 06:23 AM, Linus Torvalds wrote: On Mon, Feb 9, 2015 at 4:02 AM, Peter Zijlstra wrote: On Mon, Feb 09, 2015 at 03:04:22PM +0530, Raghavendra K T wrote: So we have 3 choices, 1. xadd 2. continue with current approach. 3. a read before unlock and also after that. For the truly par

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/10/2015 04:30 AM, Raghavendra K T wrote: >> >> So I think Raghavendra's last version (which hopefully fixes the >> lockup problem that Sasha reported) together with changing that > > V2 did pass the stress, but getting confirmation Sasha would help. I've been running it for the last two day

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/10, Raghavendra K T wrote: > > On 02/10/2015 06:23 AM, Linus Torvalds wrote: > >> add_smp(&lock->tickets.head, TICKET_LOCK_INC); >> if (READ_ONCE(lock->tickets.tail) & TICKET_SLOWPATH_FLAG) .. >> >> into something like >> >> val = xadd((&lock->ticket.head_tail, TICK

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Tue, Feb 10, 2015 at 2:18 PM, Denys Vlasenko wrote: > while (1) { > if (READ_ONCE(lock->tickets.head) != TICKET_TAIL(val)) > cpu_relax(); > } Doh should be while (READ_ONCE(lock->tickets.head) != TICKET_TAIL(val) cpu_relax(

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Tue, Feb 10, 2015 at 10:30 AM, Raghavendra K T wrote: > On 02/10/2015 06:23 AM, Linus Torvalds wrote: >> add_smp(&lock->tickets.head, TICKET_LOCK_INC); >> if (READ_ONCE(lock->tickets.tail) & TICKET_SLOWPATH_FLAG) .. >> >> into something like >> >> val = xadd((&lock->t

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/10, Denys Vlasenko wrote: > > # define HEAD_MASK (TICKET_SLOWPATH_FLAG-1) > > ... > unlock_again: > > val = xadd((&lock->ticket.head_tail, TICKET_LOCK_INC); > if (unlikely(!(val & HEAD_MASK))) { > /* overflow. we inadvertently incremented the tail word. > * tail's lsb is TICKET_SLOWP

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/10/2015 05:26 AM, Oleg Nesterov wrote: > On 02/10, Raghavendra K T wrote: >> On 02/10/2015 06:23 AM, Linus Torvalds wrote: >> >>> add_smp(&lock->tickets.head, TICKET_LOCK_INC); >>> if (READ_ONCE(lock->tickets.tail) & TICKET_SLOWPATH_FLAG) .. >>> >>> into something like >>>

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/10/2015 06:56 PM, Oleg Nesterov wrote: On 02/10, Raghavendra K T wrote: On 02/10/2015 06:23 AM, Linus Torvalds wrote: add_smp(&lock->tickets.head, TICKET_LOCK_INC); if (READ_ONCE(lock->tickets.tail) & TICKET_SLOWPATH_FLAG) .. into something like val = xad

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/10, Jeremy Fitzhardinge wrote: > > On 02/10/2015 05:26 AM, Oleg Nesterov wrote: > > On 02/10, Raghavendra K T wrote: > >> Unfortunately xadd could result in head overflow as tail is high. > >> > >> The other option was repeated cmpxchg which is bad I believe. > >> Any suggestions? > > Stupid

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/11, Raghavendra K T wrote: > > On 02/10/2015 06:56 PM, Oleg Nesterov wrote: > >> In this case __ticket_check_and_clear_slowpath() really needs to cmpxchg >> the whole .head_tail. Plus obviously more boring changes. This needs a >> separate patch even _if_ this can work. > > Correct, but apart

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/11/2015 11:08 PM, Oleg Nesterov wrote: On 02/11, Raghavendra K T wrote: On 02/10/2015 06:56 PM, Oleg Nesterov wrote: In this case __ticket_check_and_clear_slowpath() really needs to cmpxchg the whole .head_tail. Plus obviously more boring changes. This needs a separate patch even _if_ t

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/11/2015 09:24 AM, Oleg Nesterov wrote: > I agree, and I have to admit I am not sure I fully understand why > unlock uses the locked add. Except we need a barrier to avoid the race > with the enter_slowpath() users, of course. Perhaps this is the only > reason? Right now it needs to be a loc

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On Feb 11, 2015 3:15 PM, "Jeremy Fitzhardinge" wrote: > > Right now it needs to be a locked operation to prevent read-reordering. > x86 memory ordering rules state that all writes are seen in a globally > consistent order, and are globally ordered wrt reads *on the same > addresses*, but reads to

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/11/2015 03:28 PM, Linus Torvalds wrote: > > > On Feb 11, 2015 3:15 PM, "Jeremy Fitzhardinge" > wrote: > > > > Right now it needs to be a locked operation to prevent read-reordering. > > x86 memory ordering rules state that all writes are seen in a globally > > consis

Re: [Xen-devel] [PATCH] x86 spinlock: Fix memory corruption on completing completions

On 02/11, Jeremy Fitzhardinge wrote: > > On 02/11/2015 09:24 AM, Oleg Nesterov wrote: > > I agree, and I have to admit I am not sure I fully understand why > > unlock uses the locked add. Except we need a barrier to avoid the race > > with the enter_slowpath() users, of course. Perhaps this is the