ARM: Fix deadlock scenario with smp_send_stop()

PATCH Superseded Stephen Boyd 2 participants 6 messages Jul 9, 2013 – Jul 30, 2013 Arm (32-bit)

Share & tools

Stephen Boyd

author

2013-07-09 15:36 UTC #0 lore raw

PATCH 1/1 ARM: Fix deadlock scenario with smp_send_stop()

If one process calls sys_reboot and that process then stops other CPUs while those CPUs are within a spin_lock() region we can potentially encounter a deadlock scenario like below.

CPU 0 CPU 1
----- -----
spin_lock(my_lock)
smp_send_stop()
<send IPI> handle_IPI()

                         disable_preemption/irqs
                          while(1);

<PREEMPT>
spin_lock(my_lock) <--- Waits forever

We shouldn't attempt to run any other tasks after we send a stop IPI to a CPU so disable preemption so that this task runs to completion.

Reported-by Sundarajan Srinivasan <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

Resending this patch now that the context has changed.

 arch/arm/kernel/process.c | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kernel/process.c +3 -0

index 7f1efcd..8bc12d7 100644
@@ -206,6 +206,7 @@ void machine_shutdown(void)
  */
 void machine_halt(void)
 {
+	preempt_disable();
 	smp_send_stop();
 
 	local_irq_disable();

@@ -220,6 +221,7 @@ void machine_halt(void)
  */
 void machine_power_off(void)
 {
+	preempt_disable();
 	smp_send_stop();
 
 	if (pm_power_off)

@@ -239,6 +241,7 @@ void machine_power_off(void)
  */
 void machine_restart(char *cmd)
 {
+	preempt_disable();
 	smp_send_stop();
 
 	arm_pm_restart(reboot_mode, cmd);

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2013-07-24 18:56 UTC #1 lore raw

Quoted 18 lines · Stephen Boyd

If one process calls sys_reboot and that process then stops other CPUs while those CPUs are within a spin_lock() region we can potentially encounter a deadlock scenario like below.
CPU 0 CPU 1
----- -----
spin_lock(my_lock)
smp_send_stop()
<send IPI> handle_IPI()
                         disable_preemption/irqs
                          while(1);
<PREEMPT>
spin_lock(my_lock) <--- Waits forever
We shouldn't attempt to run any other tasks after we send a stop IPI to a CPU so disable preemption so that this task runs to completion.

Quoted 5 lines

Reported-by Sundarajan Srinivasan <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
Resending this patch now that the context has changed.

Ping? Shall I put this in the patch tracker?

Quoted 32 lines

 arch/arm/kernel/process.c | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kernel/process.c +3 -0

index 7f1efcd..8bc12d7 100644
@@ -206,6 +206,7 @@ void machine_shutdown(void)
  */
 void machine_halt(void)
 {
+	preempt_disable();
 	smp_send_stop();
 
 	local_irq_disable();

@@ -220,6 +221,7 @@ void machine_halt(void)
  */
 void machine_power_off(void)
 {
+	preempt_disable();
 	smp_send_stop();
 
 	if (pm_power_off)

@@ -239,6 +241,7 @@ void machine_power_off(void)
  */
 void machine_restart(char *cmd)
 {
+	preempt_disable();
 	smp_send_stop();
 
 	arm_pm_restart(reboot_mode, cmd);

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Russell King - ARM Linux

2013-07-24 20:21 UTC #2 lore raw

Quoted 19 lines · Stephen Boyd

Quoted 18 lines · Stephen Boyd
If one process calls sys_reboot and that process then stops other CPUs while those CPUs are within a spin_lock() region we can potentially encounter a deadlock scenario like below.
CPU 0 CPU 1
----- -----
spin_lock(my_lock)
smp_send_stop()
<send IPI> handle_IPI()
                         disable_preemption/irqs
                          while(1);
<PREEMPT>
spin_lock(my_lock) <--- Waits forever
We shouldn't attempt to run any other tasks after we send a stop IPI to a CPU so disable preemption so that this task runs to completion.

Quoted 7 lines

Quoted 5 lines
Reported-by Sundarajan Srinivasan <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
Resending this patch now that the context has changed.
Ping? Shall I put this in the patch tracker?

Well, looking at x86, they use local_irq_disable() before sending the stop, so I think we should do the same for cross-arch consistency.

Stephen Boyd

author

2013-07-24 20:29 UTC #3 lore raw

Quoted 20 lines · Russell King - ARM Linux

Quoted 19 lines · Stephen Boyd
Quoted 18 lines · Stephen Boyd
If one process calls sys_reboot and that process then stops other CPUs while those CPUs are within a spin_lock() region we can potentially encounter a deadlock scenario like below.
CPU 0 CPU 1
----- -----
spin_lock(my_lock)
smp_send_stop()
<send IPI> handle_IPI()
                         disable_preemption/irqs
                          while(1);
<PREEMPT>
spin_lock(my_lock) <--- Waits forever
We shouldn't attempt to run any other tasks after we send a stop IPI to a CPU so disable preemption so that this task runs to completion.

Quoted 8 lines

Quoted 6 lines
Quoted 5 lines
Reported-by Sundarajan Srinivasan <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---
Resending this patch now that the context has changed.
Ping? Shall I put this in the patch tracker?
Well, looking at x86, they use local_irq_disable() before sending the stop, so I think we should do the same for cross-arch consistency.

Fair enough. I'll send v2 with that.

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2013-07-24 20:36 UTC #4 lore raw

PATCH v2 1/1 ARM: Fix deadlock scenario with smp_send_stop()

In reply to Russell King - ARM Linux

If one process calls sys_reboot and that process then stops other CPUs while those CPUs are within a spin_lock() region we can potentially encounter a deadlock scenario like below.

CPU 0 CPU 1
----- -----
spin_lock(my_lock)
smp_send_stop()
<send IPI> handle_IPI()

                         disable_preemption/irqs
                          while(1);

<PREEMPT>
spin_lock(my_lock) <--- Waits forever

We shouldn't attempt to run any other tasks after we send a stop IPI to a CPU so disable preemption so that this task runs to completion. We use local_irq_disable() here for cross-arch consistency with x86.

Reported-by Sundarajan Srinivasan <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---

Changes since v1:
- Use local_irq_disable() instead of preempt_disable()

 arch/arm/kernel/process.c | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kernel/process.c +3 -0

index d3ca4f6..08b47eb 100644
@@ -197,6 +197,7 @@ void machine_shutdown(void)
  */
 void machine_halt(void)
 {
+	local_irq_disable();
 	smp_send_stop();
 
 	local_irq_disable();

@@ -211,6 +212,7 @@ void machine_halt(void)
  */
 void machine_power_off(void)
 {
+	local_irq_disable();
 	smp_send_stop();
 
 	if (pm_power_off)

@@ -230,6 +232,7 @@ void machine_power_off(void)
  */
 void machine_restart(char *cmd)
 {
+	local_irq_disable();
 	smp_send_stop();
 
 	arm_pm_restart(reboot_mode, cmd);

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2013-07-30 22:12 UTC #5 lore raw

Quoted 14 lines · Stephen Boyd

If one process calls sys_reboot and that process then stops other CPUs while those CPUs are within a spin_lock() region we can potentially encounter a deadlock scenario like below.
CPU 0 CPU 1
----- -----
spin_lock(my_lock)
smp_send_stop()
<send IPI> handle_IPI()
                         disable_preemption/irqs
                          while(1);
<PREEMPT>
spin_lock(my_lock) <--- Waits forever

Quoted 8 lines

We shouldn't attempt to run any other tasks after we send a stop IPI to a CPU so disable preemption so that this task runs to completion. We use local_irq_disable() here for cross-arch consistency with x86.
Reported-by Sundarajan Srinivasan <[email protected]>
Signed-off-by Stephen Boyd <[email protected]>
---

Ok I threw this into the patch tracker because there were no more comments.

Quoted 34 lines

Changes since v1:
- Use local_irq_disable() instead of preempt_disable()

 arch/arm/kernel/process.c | 3 +++
 1 file changed, 3 insertions(+)

arch/arm/kernel/process.c +3 -0

index d3ca4f6..08b47eb 100644
@@ -197,6 +197,7 @@ void machine_shutdown(void)
  */
 void machine_halt(void)
 {
+	local_irq_disable();
 	smp_send_stop();
 
 	local_irq_disable();

@@ -211,6 +212,7 @@ void machine_halt(void)
  */
 void machine_power_off(void)
 {
+	local_irq_disable();
 	smp_send_stop();
 
 	if (pm_power_off)

@@ -230,6 +232,7 @@ void machine_power_off(void)
  */
 void machine_restart(char *cmd)
 {
+	local_irq_disable();
 	smp_send_stop();
 
 	arm_pm_restart(reboot_mode, cmd);

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation