xen: fix two issues in Xen pv qspinlock handling

In the following situation a vcpu waiting for a lock might not be woken up from xen_poll_irq():

CPU 1: CPU 2: CPU 3:
takes a spinlock

                      tries to get lock
                      -> xen_qlock_wait()
                        -> xen_clear_irq_pending()

frees the lock
-> xen_qlock_kick(cpu2)

takes lock again

                                                  tries to get lock
                                                  -> *lock = _Q_SLOW_VAL
                        -> *lock == _Q_SLOW_VAL ?
                        -> xen_poll_irq()

frees the lock
-> xen_qlock_kick(cpu3)

And cpu 2 will sleep forever.

This can be avoided easily by modifying xen_qlock_wait() to call xen_poll_irq() only if the related irq was not pending and to call xen_clear_irq_pending() only if it was pending.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Juergen Gross <[email protected]>

---
 arch/x86/xen/spinlock.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

arch/x86/xen/spinlock.c +5 -3

index 973f10e05211..cd210a4ba7b1 100644
@@ -45,17 +45,12 @@ static void xen_qlock_wait(u8 *byte, u8 val)
 	if (irq == -1)
 		return;
 
-	/* clear pending */
-	xen_clear_irq_pending(irq);
-	barrier();
+	/* If irq pending already clear it and return. */
+	if (xen_test_irq_pending(irq)) {
+		xen_clear_irq_pending(irq);
+		return;
+	}
 

In the following situation a vcpu waiting for a lock might not be woken up from xen_poll_irq():

CPU 1: CPU 2: CPU 3:
takes a spinlock

                      tries to get lock
                      -> xen_qlock_wait()
                        -> xen_clear_irq_pending()

frees the lock
-> xen_qlock_kick(cpu2)

takes lock again

                                                  tries to get lock
                                                  -> *lock = _Q_SLOW_VAL
                        -> *lock == _Q_SLOW_VAL ?
                        -> xen_poll_irq()

frees the lock
-> xen_qlock_kick(cpu3)

And cpu 2 will sleep forever.

This can be avoided easily by modifying xen_qlock_wait() to call xen_poll_irq() only if the related irq was not pending and to call xen_clear_irq_pending() only if it was pending.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Juergen Gross <[email protected]>

---
 arch/x86/xen/spinlock.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

arch/x86/xen/spinlock.c +5 -10

index 973f10e05211..cd210a4ba7b1 100644
@@ -45,17 +45,12 @@ static void xen_qlock_wait(u8 *byte, u8 val)
 	if (irq == -1)
 		return;
 
-	/* clear pending */
-	xen_clear_irq_pending(irq);
-	barrier();
+	/* If irq pending already clear it and return. */
+	if (xen_test_irq_pending(irq)) {
+		xen_clear_irq_pending(irq);
+		return;
+	}
 
-	/*
-	 * We check the byte value after clearing pending IRQ to make sure
-	 * that we won't miss a wakeup event because of the clearing.
-	 *
-	 * The sync_clear_bit() call in xen_clear_irq_pending() is atomic.
-	 * So it is effectively a memory barrier for x86.
-	 */
 	if (READ_ONCE(*byte) != val)
 		return;
 

Quoted 1 line

Quoted 1 line · On 01.10.18 at 09:16, <[email protected]> wrote:

In the following situation a vcpu waiting for a lock might not be woken up from xen_poll_irq():

CPU 1: CPU 2: CPU 3:
takes a spinlock

                      tries to get lock
                      -> xen_qlock_wait()
                        -> xen_clear_irq_pending()

frees the lock
-> xen_qlock_kick(cpu2)

takes lock again

                                                  tries to get lock
                                                  -> *lock = _Q_SLOW_VAL
                        -> *lock == _Q_SLOW_VAL ?
                        -> xen_poll_irq()

frees the lock
-> xen_qlock_kick(cpu3)

And cpu 2 will sleep forever.

This can be avoided easily by modifying xen_qlock_wait() to call xen_poll_irq() only if the related irq was not pending and to call xen_clear_irq_pending() only if it was pending.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Juergen Gross <[email protected]>

Quoted 1 line

Quoted 1 line · On 01.10.18 at 09:16, <[email protected]> wrote:

In the following situation a vcpu waiting for a lock might not be woken up from xen_poll_irq():

CPU 1: CPU 2: CPU 3:
takes a spinlock

                      tries to get lock
                      -> xen_qlock_wait()
                        -> xen_clear_irq_pending()

frees the lock
-> xen_qlock_kick(cpu2)

takes lock again

                                                  tries to get lock
                                                  -> *lock = _Q_SLOW_VAL
                        -> *lock == _Q_SLOW_VAL ?
                        -> xen_poll_irq()

frees the lock
-> xen_qlock_kick(cpu3)

And cpu 2 will sleep forever.

This can be avoided easily by modifying xen_qlock_wait() to call xen_poll_irq() only if the related irq was not pending and to call xen_clear_irq_pending() only if it was pending.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Juergen Gross <[email protected]>

xen_qlock_wait() isn't safe for nested calls due to interrupts. A call of xen_qlock_kick() might be ignored in case a deeper nesting level was active right before the call of xen_poll_irq():

CPU 1: CPU 2:
spin_lock(lock1)

                                         spin_lock(lock1)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                            Interrupt happens
spin_unlock(lock1)
-> xen_qlock_kick(CPU 2)
spin_lock_irqsave(lock2)
                                         spin_lock_irqsave(lock2)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                               clears kick for lock1
                                            -> xen_poll_irq()

spin_unlock_irq_restore(lock2)
-> xen_qlock_kick(CPU 2)
wakes up
spin_unlock_irq_restore(lock2) IRET
resumes in xen_qlock_wait() -> xen_poll_irq() never wakes up

The solution is to disable interrupts in xen_qlock_wait() and not to poll for the irq in case xen_qlock_wait() is called in nmi context.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Juergen Gross <[email protected]>

---
 arch/x86/xen/spinlock.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

arch/x86/xen/spinlock.c +9 -8

index cd210a4ba7b1..e8d880e98057 100644
@@ -39,29 +39,25 @@ static void xen_qlock_kick(int cpu)
  */
 static void xen_qlock_wait(u8 *byte, u8 val)
 {
+	unsigned long flags;
 	int irq = __this_cpu_read(lock_kicker_irq);
 
 	/* If kicker interrupts not initialized yet, just spin */
-	if (irq == -1)
+	if (irq == -1 || in_nmi())
 		return;
 
-	/* If irq pending already clear it and return. */
+	/* Guard against reentry. */
+	local_irq_save(flags);
+
+	/* If irq pending already clear it. */
 	if (xen_test_irq_pending(irq)) {
 		xen_clear_irq_pending(irq);
-		return;
+	} else if (READ_ONCE(*byte) == val) {
+		/* Block until irq becomes pending (or a spurious wakeup) */
+		xen_poll_irq(irq);
 	}
 
-	if (READ_ONCE(*byte) != val)
-		return;
-
-	/*
-	 * If an interrupt happens here, it will leave the wakeup irq

xen_qlock_wait() isn't safe for nested calls due to interrupts. A call of xen_qlock_kick() might be ignored in case a deeper nesting level was active right before the call of xen_poll_irq():

CPU 1: CPU 2:
spin_lock(lock1)

                                         spin_lock(lock1)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                            Interrupt happens
spin_unlock(lock1)
-> xen_qlock_kick(CPU 2)
spin_lock_irqsave(lock2)
                                         spin_lock_irqsave(lock2)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                               clears kick for lock1
                                            -> xen_poll_irq()

spin_unlock_irq_restore(lock2)
-> xen_qlock_kick(CPU 2)
wakes up
spin_unlock_irq_restore(lock2) IRET
resumes in xen_qlock_wait() -> xen_poll_irq() never wakes up

The solution is to disable interrupts in xen_qlock_wait() and not to poll for the irq in case xen_qlock_wait() is called in nmi context.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Juergen Gross <[email protected]>

---
 arch/x86/xen/spinlock.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

arch/x86/xen/spinlock.c +10 -14

index cd210a4ba7b1..e8d880e98057 100644
@@ -39,29 +39,25 @@ static void xen_qlock_kick(int cpu)
  */
 static void xen_qlock_wait(u8 *byte, u8 val)
 {
+	unsigned long flags;
 	int irq = __this_cpu_read(lock_kicker_irq);
 
 	/* If kicker interrupts not initialized yet, just spin */
-	if (irq == -1)
+	if (irq == -1 || in_nmi())
 		return;
 
-	/* If irq pending already clear it and return. */
+	/* Guard against reentry. */
+	local_irq_save(flags);
+
+	/* If irq pending already clear it. */
 	if (xen_test_irq_pending(irq)) {
 		xen_clear_irq_pending(irq);
-		return;
+	} else if (READ_ONCE(*byte) == val) {
+		/* Block until irq becomes pending (or a spurious wakeup) */
+		xen_poll_irq(irq);
 	}
 
-	if (READ_ONCE(*byte) != val)
-		return;
-
-	/*
-	 * If an interrupt happens here, it will leave the wakeup irq
-	 * pending, which will cause xen_poll_irq() to return
-	 * immediately.
-	 */
-
-	/* Block until irq becomes pending (or perhaps a spurious wakeup) */
-	xen_poll_irq(irq);
+	local_irq_restore(flags);
 }
 
 static irqreturn_t dummy_handler(int irq, void *dev_id)

Quoted 1 line

Quoted 1 line · On 01.10.18 at 09:16, <[email protected]> wrote:

xen_qlock_wait() isn't safe for nested calls due to interrupts. A call of xen_qlock_kick() might be ignored in case a deeper nesting level was active right before the call of xen_poll_irq():

CPU 1: CPU 2:
spin_lock(lock1)

                                         spin_lock(lock1)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                            Interrupt happens
spin_unlock(lock1)
-> xen_qlock_kick(CPU 2)
spin_lock_irqsave(lock2)
                                         spin_lock_irqsave(lock2)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                               clears kick for lock1
                                            -> xen_poll_irq()

spin_unlock_irq_restore(lock2)
-> xen_qlock_kick(CPU 2)
wakes up
spin_unlock_irq_restore(lock2) IRET
resumes in xen_qlock_wait() -> xen_poll_irq() never wakes up

The solution is to disable interrupts in xen_qlock_wait() and not to poll for the irq in case xen_qlock_wait() is called in nmi context.

Quoted 1 line

Quoted 1 line · On 01.10.18 at 09:16, <[email protected]> wrote:

xen_qlock_wait() isn't safe for nested calls due to interrupts. A call of xen_qlock_kick() might be ignored in case a deeper nesting level was active right before the call of xen_poll_irq():

CPU 1: CPU 2:
spin_lock(lock1)

                                         spin_lock(lock1)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                            Interrupt happens
spin_unlock(lock1)
-> xen_qlock_kick(CPU 2)
spin_lock_irqsave(lock2)
                                         spin_lock_irqsave(lock2)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                               clears kick for lock1
                                            -> xen_poll_irq()

spin_unlock_irq_restore(lock2)
-> xen_qlock_kick(CPU 2)
wakes up
spin_unlock_irq_restore(lock2) IRET
resumes in xen_qlock_wait() -> xen_poll_irq() never wakes up

The solution is to disable interrupts in xen_qlock_wait() and not to poll for the irq in case xen_qlock_wait() is called in nmi context.

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

Quoted 15 lines · David Woodhouse

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Quoted 5 lines

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

Yes. Look at it like idle HLT or WFI. You have to disable interrupt before checking the condition and then the hardware or in this case the hypervisor has to bring you back when an interrupt is raised.

If that would not work then the check would be racy, because the interrupt could hit and be handled after the check and before going into HLT/WFI/hypercall and then the thing is out until the next interrupt comes along, which might be never.

Quoted 23 lines · Thomas Gleixner

Quoted 15 lines · David Woodhouse

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Quoted 5 lines

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

Quoted 8 lines

Yes. Look at it like idle HLT or WFI. You have to disable interrupt before checking the condition and then the hardware or in this case the hypervisor has to bring you back when an interrupt is raised.

If that would not work then the check would be racy, because the interrupt could hit and be handled after the check and before going into HLT/WFI/hypercall and then the thing is out until the next interrupt comes along, which might be never.

Right, but in this case we're calling into the hypervisor to poll for one *specific* IRQ. Everything you say is true for that specific IRQ.

My question is what happens to *other* IRQs. We want them, but are they masked? I'm staring at the Xen do_poll() code and haven't quite worked that out...

Quoted 23 lines · Thomas Gleixner

Quoted 15 lines · David Woodhouse

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Quoted 5 lines

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

Quoted 8 lines

Yes. Look at it like idle HLT or WFI. You have to disable interrupt before checking the condition and then the hardware or in this case the hypervisor has to bring you back when an interrupt is raised.

If that would not work then the check would be racy, because the interrupt could hit and be handled after the check and before going into HLT/WFI/hypercall and then the thing is out until the next interrupt comes along, which might be never.

Right, but in this case we're calling into the hypervisor to poll for one *specific* IRQ. Everything you say is true for that specific IRQ.

My question is what happens to *other* IRQs. We want them, but are they masked? I'm staring at the Xen do_poll() code and haven't quite worked that out...

Quoted 36 lines · David Woodhouse

Quoted 23 lines · Thomas Gleixner

Quoted 15 lines · David Woodhouse

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Quoted 5 lines

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

Quoted 8 lines

Yes. Look at it like idle HLT or WFI. You have to disable interrupt before checking the condition and then the hardware or in this case the hypervisor has to bring you back when an interrupt is raised.

If that would not work then the check would be racy, because the interrupt could hit and be handled after the check and before going into HLT/WFI/hypercall and then the thing is out until the next interrupt comes along, which might be never.

Right, but in this case we're calling into the hypervisor to poll for one *specific* IRQ. Everything you say is true for that specific IRQ.

Quoted 3 lines

My question is what happens to *other* IRQs. We want them, but are they masked? I'm staring at the Xen do_poll() code and haven't quite worked that out...

Ah, sorry. That of course has to come back like HLT/WFI for any interrupt, but I have no idea what the Xen HV is doing there.

Quoted 36 lines · David Woodhouse

Quoted 23 lines · Thomas Gleixner

Quoted 15 lines · David Woodhouse

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Quoted 5 lines

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

Quoted 8 lines

Yes. Look at it like idle HLT or WFI. You have to disable interrupt before checking the condition and then the hardware or in this case the hypervisor has to bring you back when an interrupt is raised.

If that would not work then the check would be racy, because the interrupt could hit and be handled after the check and before going into HLT/WFI/hypercall and then the thing is out until the next interrupt comes along, which might be never.

Right, but in this case we're calling into the hypervisor to poll for one *specific* IRQ. Everything you say is true for that specific IRQ.

Quoted 3 lines

My question is what happens to *other* IRQs. We want them, but are they masked? I'm staring at the Xen do_poll() code and haven't quite worked that out...

Ah, sorry. That of course has to come back like HLT/WFI for any interrupt, but I have no idea what the Xen HV is doing there.

The Xen HV is doing it right. It is blocking the vcpu in do_poll() and any interrupt will unblock it.

The Xen HV is doing it right. It is blocking the vcpu in do_poll() and any interrupt will unblock it.

Quoted 15 lines · David Woodhouse

arch/x86/xen/spinlock.c static void xen_qlock_kick(int cpu) · line 58

Quoted 12 lines · Juergen Gross

arch/x86/xen/spinlock.c +7 -2

-       /* If irq pending already clear it and return. */
+       /* Guard against reentry. */
+       local_irq_save(flags);
+
+       /* If irq pending already clear it. */
        if (xen_test_irq_pending(irq)) {
                xen_clear_irq_pending(irq);
-               return;
+       } else if (READ_ONCE(*byte) == val) {
+               /* Block until irq becomes pending (or a spurious wakeup) */
+               xen_poll_irq(irq);
        }

Quoted 5 lines

Does this still allow other IRQs to wake it from xen_poll_irq()?

In the case where process-context code is spinning for a lock without disabling interrupts, we *should* allow interrupts to occur still... does this?

Yes. Look at it like idle HLT or WFI. You have to disable interrupt before checking the condition and then the hardware or in this case the hypervisor has to bring you back when an interrupt is raised.

If that would not work then the check would be racy, because the interrupt could hit and be handled after the check and before going into HLT/WFI/hypercall and then the thing is out until the next interrupt comes along, which might be never.

The Xen specific queue spinlock wait function has two issues which could result in a hanging system.

They have a similar root cause of clearing a pending wakeup of a waiting vcpu and later going to sleep waiting for the just cleared wakeup event, which of course won't ever happen.

Juergen Gross (2):
xen: fix race in xen_qlock_wait()
xen: make xen_qlock_wait() nestable

 arch/x86/xen/spinlock.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

Cc [email protected]

Cc [email protected]

The Xen specific queue spinlock wait function has two issues which could result in a hanging system.

They have a similar root cause of clearing a pending wakeup of a waiting vcpu and later going to sleep waiting for the just cleared wakeup event, which of course won't ever happen.

Juergen Gross (2):
xen: fix race in xen_qlock_wait()
xen: make xen_qlock_wait() nestable

 arch/x86/xen/spinlock.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

Cc [email protected]

Cc [email protected]

The Xen specific queue spinlock wait function has two issues which could result in a hanging system.

They have a similar root cause of clearing a pending wakeup of a waiting vcpu and later going to sleep waiting for the just cleared wakeup event, which of course won't ever happen.

Juergen Gross (2):
xen: fix race in xen_qlock_wait()
xen: make xen_qlock_wait() nestable

 arch/x86/xen/spinlock.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

Cc [email protected]

Cc [email protected]

Quoted 11 lines · Juergen Gross

The Xen specific queue spinlock wait function has two issues which could result in a hanging system.

They have a similar root cause of clearing a pending wakeup of a waiting vcpu and later going to sleep waiting for the just cleared wakeup event, which of course won't ever happen.

Juergen Gross (2):
xen: fix race in xen_qlock_wait()
xen: make xen_qlock_wait() nestable

Quoted 5 lines

 arch/x86/xen/spinlock.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

Cc [email protected]

Cc [email protected]

LGTM. Both these should be Cc:[email protected], yes?

Quoted 11 lines · Juergen Gross

The Xen specific queue spinlock wait function has two issues which could result in a hanging system.

They have a similar root cause of clearing a pending wakeup of a waiting vcpu and later going to sleep waiting for the just cleared wakeup event, which of course won't ever happen.

Juergen Gross (2):
xen: fix race in xen_qlock_wait()
xen: make xen_qlock_wait() nestable

Quoted 5 lines

 arch/x86/xen/spinlock.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

Cc [email protected]

Cc [email protected]

LGTM. Both these should be Cc:[email protected], yes?

The Xen specific queue spinlock wait function has two issues which could result in a hanging system.

They have a similar root cause of clearing a pending wakeup of a waiting vcpu and later going to sleep waiting for the just cleared wakeup event, which of course won't ever happen.

Juergen Gross (2):
xen: fix race in xen_qlock_wait()
xen: make xen_qlock_wait() nestable

 arch/x86/xen/spinlock.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

Cc [email protected]

Cc [email protected]

Quoted 28 lines

Quoted 1 line

Quoted 1 line · On 01.10.18 at 09:16, <[email protected]> wrote:

xen_qlock_wait() isn't safe for nested calls due to interrupts. A call of xen_qlock_kick() might be ignored in case a deeper nesting level was active right before the call of xen_poll_irq():

CPU 1: CPU 2:
spin_lock(lock1)

                                         spin_lock(lock1)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                            Interrupt happens
spin_unlock(lock1)
-> xen_qlock_kick(CPU 2)
spin_lock_irqsave(lock2)
                                         spin_lock_irqsave(lock2)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                               clears kick for lock1
                                            -> xen_poll_irq()

spin_unlock_irq_restore(lock2)
-> xen_qlock_kick(CPU 2)
wakes up
spin_unlock_irq_restore(lock2) IRET
resumes in xen_qlock_wait() -> xen_poll_irq() never wakes up

Quoted 2 lines

The solution is to disable interrupts in xen_qlock_wait() and not to poll for the irq in case xen_qlock_wait() is called in nmi context.

Are precautions against NMI really worthwhile? Locks acquired both in NMI context as well as outside of it are liable to deadlock anyway, aren't they?

Quoted 1 line

Quoted 1 line · On 01.10.18 at 11:03, <[email protected]> wrote:

Quoted 31 lines · Jan Beulich

Quoted 30 lines

Quoted 1 line

Quoted 1 line · On 01.10.18 at 09:16, <[email protected]> wrote:

xen_qlock_wait() isn't safe for nested calls due to interrupts. A call of xen_qlock_kick() might be ignored in case a deeper nesting level was active right before the call of xen_poll_irq():

CPU 1: CPU 2:
spin_lock(lock1)

                                         spin_lock(lock1)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                            Interrupt happens
spin_unlock(lock1)
-> xen_qlock_kick(CPU 2)
spin_lock_irqsave(lock2)
                                         spin_lock_irqsave(lock2)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                               clears kick for lock1
                                            -> xen_poll_irq()

spin_unlock_irq_restore(lock2)
-> xen_qlock_kick(CPU 2)
wakes up
spin_unlock_irq_restore(lock2) IRET
resumes in xen_qlock_wait() -> xen_poll_irq() never wakes up

The solution is to disable interrupts in xen_qlock_wait() and not to poll for the irq in case xen_qlock_wait() is called in nmi context.

Quoted 3 lines

Are precautions against NMI really worthwhile? Locks acquired both in NMI context as well as outside of it are liable to deadlock anyway, aren't they?

The locks don't need to be the same. A NMI-only lock tried to be acquired with xen_qlock_wait() for another lock having been interrupted by the NMI will be enough to risk the issue.

Quoted 1 line

Quoted 1 line · On 01.10.18 at 11:03, <[email protected]> wrote:

Quoted 31 lines · Jan Beulich

Quoted 30 lines

Quoted 1 line

Quoted 1 line · On 01.10.18 at 09:16, <[email protected]> wrote:

xen_qlock_wait() isn't safe for nested calls due to interrupts. A call of xen_qlock_kick() might be ignored in case a deeper nesting level was active right before the call of xen_poll_irq():

CPU 1: CPU 2:
spin_lock(lock1)

                                         spin_lock(lock1)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                            Interrupt happens
spin_unlock(lock1)
-> xen_qlock_kick(CPU 2)
spin_lock_irqsave(lock2)
                                         spin_lock_irqsave(lock2)
                                         -> xen_qlock_wait()
                                            -> xen_clear_irq_pending()
                                               clears kick for lock1
                                            -> xen_poll_irq()

spin_unlock_irq_restore(lock2)
-> xen_qlock_kick(CPU 2)
wakes up
spin_unlock_irq_restore(lock2) IRET
resumes in xen_qlock_wait() -> xen_poll_irq() never wakes up

The solution is to disable interrupts in xen_qlock_wait() and not to poll for the irq in case xen_qlock_wait() is called in nmi context.

Quoted 3 lines

Are precautions against NMI really worthwhile? Locks acquired both in NMI context as well as outside of it are liable to deadlock anyway, aren't they?

The locks don't need to be the same. A NMI-only lock tried to be acquired with xen_qlock_wait() for another lock having been interrupted by the NMI will be enough to risk the issue.