signal: fix deadlock when sigdeliver call enter_critical_section

cpu0 cpu1: user_main signest_test sched_unlock nxsched_merge_pending nxsched_add_readytorun up_cpu_pause arm_sigdeliver enter_critical_section Reason: In the SMP, cpu0 is already in the critical section and waiting for cpu1 to enter the suspended state. However, when cpu1 executes arm_sigdeliver, it is in the irq-disabled state but not in the critical section. At this point, cpu1 is unable to respond to interrupts and is continuously attempting to enter the critical section, resulting in a deadlock. Resolve: adjust the logic, do not entering the critical section when interrupt-disabled. test: We can use qemu for testing. compiling make distclean -j20; ./tools/configure.sh -l qemu-armv8a:nsh_smp ;make -j20 running qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic -machine virt,virtualization=on,gic-version=3 -net none -chardev stdio,id=con,mux=on -serial chardev:con -mon chardev=con,mode=readline -kernel ./nuttx Signed-off-by: hujun5 <[email protected]>
hujun260 · May 20, 2024 · cb183d1 · cb183d1
1 parent 7044e38
commit cb183d1
Show file tree

Hide file tree

Showing 10 changed files with 69 additions and 56 deletions.
diff --git a/arch/arm/src/armv6-m/arm_sigdeliver.c b/arch/arm/src/armv6-m/arm_sigdeliver.c
@@ -63,8 +63,6 @@ void arm_sigdeliver(void)
    */
 
   int16_t saved_irqcount;
-
-  enter_critical_section();
 #endif
 
   board_autoled_on(LED_SIGNAL);
@@ -81,17 +79,16 @@ void arm_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
       leave_critical_section((uint16_t)regs[REG_PRIMASK]);
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -122,7 +119,7 @@ void arm_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -135,6 +132,9 @@ void arm_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+      leave_critical_section((uint16_t)regs[REG_PRIMASK]);
+#endif
       goto retry;
     }
 

diff --git a/arch/arm/src/armv7-a/arm_sigdeliver.c b/arch/arm/src/armv7-a/arm_sigdeliver.c
@@ -63,8 +63,6 @@ void arm_sigdeliver(void)
    */
 
   int16_t saved_irqcount;
-
-  enter_critical_section();
 #endif
 
   board_autoled_on(LED_SIGNAL);
@@ -81,17 +79,16 @@ void arm_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
       leave_critical_section(regs[REG_CPSR]);
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -122,7 +119,7 @@ void arm_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -135,6 +132,9 @@ void arm_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+      leave_critical_section(regs[REG_CPSR]);
+#endif
       goto retry;
     }
 

diff --git a/arch/arm/src/armv7-m/arm_sigdeliver.c b/arch/arm/src/armv7-m/arm_sigdeliver.c
@@ -63,8 +63,6 @@ void arm_sigdeliver(void)
    */
 
   int16_t saved_irqcount;
-
-  enter_critical_section();
 #endif
 
   board_autoled_on(LED_SIGNAL);
@@ -81,21 +79,20 @@ void arm_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
 #ifdef CONFIG_ARMV7M_USEBASEPRI
       leave_critical_section((uint8_t)regs[REG_BASEPRI]);
 #else
       leave_critical_section((uint16_t)regs[REG_PRIMASK]);
 #endif
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -126,7 +123,7 @@ void arm_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -139,6 +136,13 @@ void arm_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+#  ifdef CONFIG_ARMV7M_USEBASEPRI
+      leave_critical_section((uint8_t)regs[REG_BASEPRI]);
+#  else
+      leave_critical_section((uint16_t)regs[REG_PRIMASK]);
+#  endif
+#endif
       goto retry;
     }
 

diff --git a/arch/arm/src/armv7-r/arm_sigdeliver.c b/arch/arm/src/armv7-r/arm_sigdeliver.c
@@ -63,8 +63,6 @@ void arm_sigdeliver(void)
    */
 
   int16_t saved_irqcount;
-
-  enter_critical_section();
 #endif
 
   board_autoled_on(LED_SIGNAL);
@@ -81,17 +79,16 @@ void arm_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
       leave_critical_section(regs[REG_CPSR]);
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -119,7 +116,7 @@ void arm_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -132,6 +129,9 @@ void arm_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+      leave_critical_section(regs[REG_CPSR]);
+#endif
       goto retry;
     }
 

diff --git a/arch/arm/src/armv8-m/arm_sigdeliver.c b/arch/arm/src/armv8-m/arm_sigdeliver.c
@@ -63,8 +63,6 @@ void arm_sigdeliver(void)
    */
 
   int16_t saved_irqcount;
-
-  enter_critical_section();
 #endif
 
   board_autoled_on(LED_SIGNAL);
@@ -81,21 +79,20 @@ void arm_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
 #ifdef CONFIG_ARMV8M_USEBASEPRI
       leave_critical_section((uint8_t)regs[REG_BASEPRI]);
 #else
       leave_critical_section((uint16_t)regs[REG_PRIMASK]);
 #endif
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -126,7 +123,7 @@ void arm_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -139,6 +136,13 @@ void arm_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+#  ifdef CONFIG_ARMV8M_USEBASEPRI
+      leave_critical_section((uint8_t)regs[REG_BASEPRI]);
+#  else
+      leave_critical_section((uint16_t)regs[REG_PRIMASK]);
+#  endif
+#endif
       goto retry;
     }
 

diff --git a/arch/arm/src/armv8-r/arm_sigdeliver.c b/arch/arm/src/armv8-r/arm_sigdeliver.c
@@ -79,17 +79,16 @@ void arm_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
       leave_critical_section(regs[REG_CPSR]);
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -117,7 +116,7 @@ void arm_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -130,6 +129,9 @@ void arm_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+      leave_critical_section(regs[REG_CPSR]);
+#endif
       goto retry;
     }
 
@@ -149,6 +151,8 @@ void arm_sigdeliver(void)
 
   board_autoled_off(LED_SIGNAL);
 #ifdef CONFIG_SMP
+  rtcb->irqcount++;
+  leave_critical_section(regs[REG_CPSR]);
   rtcb->irqcount--;
 #endif
   arm_fullcontextrestore(regs);

diff --git a/arch/arm64/src/common/arm64_sigdeliver.c b/arch/arm64/src/common/arm64_sigdeliver.c
@@ -71,7 +71,6 @@ void arm64_sigdeliver(void)
   struct regs_context  *pctx =
                 (struct regs_context *)rtcb->xcp.saved_reg;
   flags = (pctx->spsr & SPSR_DAIF_MASK);
-  enter_critical_section();
 #endif
 
   sinfo("rtcb=%p sigdeliver=%p sigpendactionq.head=%p\n",
@@ -86,17 +85,16 @@ void arm64_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
       leave_critical_section(flags);
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -127,7 +125,7 @@ void arm64_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -140,6 +138,9 @@ void arm64_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+      leave_critical_section(flags);
+#endif
       goto retry;
     }
 

diff --git a/arch/risc-v/src/common/riscv_sigdeliver.c b/arch/risc-v/src/common/riscv_sigdeliver.c
@@ -64,8 +64,6 @@ void riscv_sigdeliver(void)
    */
 
   int16_t saved_irqcount;
-
-  enter_critical_section();
 #endif
 
   board_autoled_on(LED_SIGNAL);
@@ -82,17 +80,16 @@ void riscv_sigdeliver(void)
    */
 
   saved_irqcount = rtcb->irqcount;
-  DEBUGASSERT(saved_irqcount >= 1);
+  DEBUGASSERT(saved_irqcount >= 0);
 
   /* Now we need call leave_critical_section() repeatedly to get the irqcount
    * to zero, freeing all global spinlocks that enforce the critical section.
    */
 
-  do
+  while (rtcb->irqcount > 0)
     {
       leave_critical_section(regs[REG_INT_CTX]);
     }
-  while (rtcb->irqcount > 0);
 #endif /* CONFIG_SMP */
 
 #ifndef CONFIG_SUPPRESS_INTERRUPTS
@@ -121,7 +118,7 @@ void riscv_sigdeliver(void)
    */
 
   DEBUGASSERT(rtcb->irqcount == 0);
-  while (rtcb->irqcount < saved_irqcount)
+  while (rtcb->irqcount < saved_irqcount + 1)
     {
       enter_critical_section();
     }
@@ -134,6 +131,9 @@ void riscv_sigdeliver(void)
   if (!sq_empty(&rtcb->sigpendactionq) &&
       (rtcb->flags & TCB_FLAG_SIGNAL_ACTION) == 0)
     {
+#ifdef CONFIG_SMP
+      leave_critical_section(regs[REG_INT_CTX]);
+#endif
       goto retry;
     }