From f9d20ea4d2c5e5e6f173ddacbab08b6e252af79f Mon Sep 17 00:00:00 2001 From: ligd Date: Wed, 20 Jan 2021 21:26:28 +0800 Subject: [PATCH] sigdeliver: fix system block when kill signal to idle in SMP Bug description: CONFIG_SMP=y Suppose we have 2 cores in SMP, here is the ps return: PID GROUP CPU PRI POLICY TYPE NPX STATE STACK USED FILLED COMMAND 0 0 0 0 FIFO Kthread N-- Assigned 004076 000748 18.3% CPU0 IDLE 1 0 1 0 FIFO Kthread N-- Running 004096 000540 13.1% CPU1 IDLE nsh> kill -4 0 or: nsh> kill -4 1 system blocked. Reason: In func xx_sigdeliver() restore stage, when saved_irqcount == 0, that means rtcb NOT in critical_section before switch to xx_sigdeliver(), then we need reset the critical_section state before swith back. Fix: Add condition to cover saved_irqcount == 0. Change-Id: I4af7f95e47f6d78a4094c3757d39b01ac9d533b3 Signed-off-by: ligd --- arch/arm/src/armv6-m/arm_sigdeliver.c | 78 +++++++--------------- arch/arm/src/armv7-a/arm_sigdeliver.c | 39 +++-------- arch/arm/src/armv7-m/arm_sigdeliver.c | 39 +++-------- arch/arm/src/armv8-m/arm_sigdeliver.c | 39 +++-------- arch/risc-v/src/rv64gc/riscv_sigdeliver.c | 39 +++-------- arch/xtensa/src/common/xtensa_sigdeliver.c | 39 +++-------- 6 files changed, 78 insertions(+), 195 deletions(-) diff --git a/arch/arm/src/armv6-m/arm_sigdeliver.c b/arch/arm/src/armv6-m/arm_sigdeliver.c index 186b843adb..28b73eb0bb 100644 --- a/arch/arm/src/armv6-m/arm_sigdeliver.c +++ b/arch/arm/src/armv6-m/arm_sigdeliver.c @@ -1,35 +1,20 @@ /**************************************************************************** * arch/arm/src/armv6-m/arm_sigdeliver.c * - * Copyright (C) 2013-2015, 2018-2019 Gregory Nutt. All rights reserved. - * Author: Gregory Nutt + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions - * are met: + * http://www.apache.org/licenses/LICENSE-2.0 * - * 1. Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * 2. Redistributions in binary form must reproduce the above copyright - * notice, this list of conditions and the following disclaimer in - * the documentation and/or other materials provided with the - * distribution. - * 3. Neither the name NuttX nor the names of its contributors may be - * used to endorse or promote products derived from this software - * without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS - * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE - * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, - * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, - * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS - * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED - * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN - * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE - * POSSIBILITY OF SUCH DAMAGE. + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. * ****************************************************************************/ @@ -144,20 +129,19 @@ void arm_sigdeliver(void) sinfo("Resuming\n"); - /* Call enter_critical_section() to disable local interrupts before - * restoring local context. - * - * Here, we should not use up_irq_save() in SMP mode. - * For example, if we call up_irq_save() here and another CPU might - * have called up_cpu_pause() to this cpu, hence g_cpu_irqlock has - * been locked by the cpu, in this case, we would see a deadlock in - * later call of enter_critical_section() to restore irqcount. - * To avoid this situation, we need to call enter_critical_section(). +#ifdef CONFIG_SMP + /* Restore the saved 'irqcount' and recover the critical section + * spinlocks. */ -#ifdef CONFIG_SMP - enter_critical_section(); -#else + DEBUGASSERT(rtcb->irqcount == 0); + while (rtcb->irqcount < saved_irqcount) + { + enter_critical_section(); + } +#endif + +#ifndef CONFIG_SUPPRESS_INTERRUPTS up_irq_save(); #endif @@ -183,22 +167,6 @@ void arm_sigdeliver(void) #endif rtcb->xcp.sigdeliver = NULL; /* Allows next handler to be scheduled */ -#ifdef CONFIG_SMP - /* Restore the saved 'irqcount' and recover the critical section - * spinlocks. - * - * REVISIT: irqcount should be one from the above call to - * enter_critical_section(). Could the saved_irqcount be zero? That - * would be a problem. - */ - - DEBUGASSERT(rtcb->irqcount == 1); - while (rtcb->irqcount < saved_irqcount) - { - enter_critical_section(); - } -#endif - /* Then restore the correct state for this thread of * execution. */ diff --git a/arch/arm/src/armv7-a/arm_sigdeliver.c b/arch/arm/src/armv7-a/arm_sigdeliver.c index 8ceef66fd0..20baf6a4df 100644 --- a/arch/arm/src/armv7-a/arm_sigdeliver.c +++ b/arch/arm/src/armv7-a/arm_sigdeliver.c @@ -124,20 +124,19 @@ void arm_sigdeliver(void) sinfo("Resuming\n"); - /* Call enter_critical_section() to disable local interrupts before - * restoring local context. - * - * Here, we should not use up_irq_save() in SMP mode. - * For example, if we call up_irq_save() here and another CPU might - * have called up_cpu_pause() to this cpu, hence g_cpu_irqlock has - * been locked by the cpu, in this case, we would see a deadlock in - * later call of enter_critical_section() to restore irqcount. - * To avoid this situation, we need to call enter_critical_section(). +#ifdef CONFIG_SMP + /* Restore the saved 'irqcount' and recover the critical section + * spinlocks. */ -#ifdef CONFIG_SMP - enter_critical_section(); -#else + DEBUGASSERT(rtcb->irqcount == 0); + while (rtcb->irqcount < saved_irqcount) + { + enter_critical_section(); + } +#endif + +#ifndef CONFIG_SUPPRESS_INTERRUPTS up_irq_save(); #endif @@ -159,22 +158,6 @@ void arm_sigdeliver(void) regs[REG_CPSR] = rtcb->xcp.saved_cpsr; rtcb->xcp.sigdeliver = NULL; /* Allows next handler to be scheduled */ -#ifdef CONFIG_SMP - /* Restore the saved 'irqcount' and recover the critical section - * spinlocks. - * - * REVISIT: irqcount should be one from the above call to - * enter_critical_section(). Could the saved_irqcount be zero? That - * would be a problem. - */ - - DEBUGASSERT(rtcb->irqcount == 1); - while (rtcb->irqcount < saved_irqcount) - { - enter_critical_section(); - } -#endif - /* Then restore the correct state for this thread of execution. */ board_autoled_off(LED_SIGNAL); diff --git a/arch/arm/src/armv7-m/arm_sigdeliver.c b/arch/arm/src/armv7-m/arm_sigdeliver.c index d62743dae5..50dbec3aba 100644 --- a/arch/arm/src/armv7-m/arm_sigdeliver.c +++ b/arch/arm/src/armv7-m/arm_sigdeliver.c @@ -128,20 +128,19 @@ void arm_sigdeliver(void) sinfo("Resuming\n"); - /* Call enter_critical_section() to disable local interrupts before - * restoring local context. - * - * Here, we should not use up_irq_save() in SMP mode. - * For example, if we call up_irq_save() here and another CPU might - * have called up_cpu_pause() to this cpu, hence g_cpu_irqlock has - * been locked by the cpu, in this case, we would see a deadlock in - * later call of enter_critical_section() to restore irqcount. - * To avoid this situation, we need to call enter_critical_section(). +#ifdef CONFIG_SMP + /* Restore the saved 'irqcount' and recover the critical section + * spinlocks. */ -#ifdef CONFIG_SMP - enter_critical_section(); -#else + DEBUGASSERT(rtcb->irqcount == 0); + while (rtcb->irqcount < saved_irqcount) + { + enter_critical_section(); + } +#endif + +#ifndef CONFIG_SUPPRESS_INTERRUPTS up_irq_save(); #endif @@ -171,22 +170,6 @@ void arm_sigdeliver(void) #endif rtcb->xcp.sigdeliver = NULL; /* Allows next handler to be scheduled */ -#ifdef CONFIG_SMP - /* Restore the saved 'irqcount' and recover the critical section - * spinlocks. - * - * REVISIT: irqcount should be one from the above call to - * enter_critical_section(). Could the saved_irqcount be zero? That - * would be a problem. - */ - - DEBUGASSERT(rtcb->irqcount == 1); - while (rtcb->irqcount < saved_irqcount) - { - enter_critical_section(); - } -#endif - /* Then restore the correct state for this thread of * execution. */ diff --git a/arch/arm/src/armv8-m/arm_sigdeliver.c b/arch/arm/src/armv8-m/arm_sigdeliver.c index 88b7f71712..cb5a2de72d 100644 --- a/arch/arm/src/armv8-m/arm_sigdeliver.c +++ b/arch/arm/src/armv8-m/arm_sigdeliver.c @@ -128,20 +128,19 @@ void arm_sigdeliver(void) sinfo("Resuming\n"); - /* Call enter_critical_section() to disable local interrupts before - * restoring local context. - * - * Here, we should not use up_irq_save() in SMP mode. - * For example, if we call up_irq_save() here and another CPU might - * have called up_cpu_pause() to this cpu, hence g_cpu_irqlock has - * been locked by the cpu, in this case, we would see a deadlock in - * later call of enter_critical_section() to restore irqcount. - * To avoid this situation, we need to call enter_critical_section(). +#ifdef CONFIG_SMP + /* Restore the saved 'irqcount' and recover the critical section + * spinlocks. */ -#ifdef CONFIG_SMP - enter_critical_section(); -#else + DEBUGASSERT(rtcb->irqcount == 0); + while (rtcb->irqcount < saved_irqcount) + { + enter_critical_section(); + } +#endif + +#ifndef CONFIG_SUPPRESS_INTERRUPTS up_irq_save(); #endif @@ -171,22 +170,6 @@ void arm_sigdeliver(void) #endif rtcb->xcp.sigdeliver = NULL; /* Allows next handler to be scheduled */ -#ifdef CONFIG_SMP - /* Restore the saved 'irqcount' and recover the critical section - * spinlocks. - * - * REVISIT: irqcount should be one from the above call to - * enter_critical_section(). Could the saved_irqcount be zero? That - * would be a problem. - */ - - DEBUGASSERT(rtcb->irqcount == 1); - while (rtcb->irqcount < saved_irqcount) - { - enter_critical_section(); - } -#endif - /* Then restore the correct state for this thread of * execution. */ diff --git a/arch/risc-v/src/rv64gc/riscv_sigdeliver.c b/arch/risc-v/src/rv64gc/riscv_sigdeliver.c index dc0cc4352e..4fd3cf6d72 100644 --- a/arch/risc-v/src/rv64gc/riscv_sigdeliver.c +++ b/arch/risc-v/src/rv64gc/riscv_sigdeliver.c @@ -144,20 +144,19 @@ void up_sigdeliver(void) sinfo("Resuming EPC: %016" PRIx64 " INT_CTX: %016" PRIx64 "\n", regs[REG_EPC], regs[REG_INT_CTX]); - /* Call enter_critical_section() to disable local interrupts before - * restoring local context. - * - * Here, we should not use up_irq_save() in SMP mode. - * For example, if we call up_irq_save() here and another CPU might - * have called up_cpu_pause() to this cpu, hence g_cpu_irqlock has - * been locked by the cpu, in this case, we would see a deadlock in - * later call of enter_critical_section() to restore irqcount. - * To avoid this situation, we need to call enter_critical_section(). +#ifdef CONFIG_SMP + /* Restore the saved 'irqcount' and recover the critical section + * spinlocks. */ -#ifdef CONFIG_SMP - enter_critical_section(); -#else + DEBUGASSERT(rtcb->irqcount == 0); + while (rtcb->irqcount < saved_irqcount) + { + (void)enter_critical_section(); + } +#endif + +#ifndef CONFIG_SUPPRESS_INTERRUPTS up_irq_save(); #endif @@ -179,22 +178,6 @@ void up_sigdeliver(void) regs[REG_INT_CTX] = rtcb->xcp.saved_int_ctx; rtcb->xcp.sigdeliver = NULL; /* Allows next handler to be scheduled */ -#ifdef CONFIG_SMP - /* Restore the saved 'irqcount' and recover the critical section - * spinlocks. - * - * REVISIT: irqcount should be one from the above call to - * enter_critical_section(). Could the saved_irqcount be zero? That - * would be a problem. - */ - - DEBUGASSERT(rtcb->irqcount == 1); - while (rtcb->irqcount < saved_irqcount) - { - (void)enter_critical_section(); - } -#endif - /* Then restore the correct state for this thread of * execution. */ diff --git a/arch/xtensa/src/common/xtensa_sigdeliver.c b/arch/xtensa/src/common/xtensa_sigdeliver.c index 92879f731b..94a248455b 100644 --- a/arch/xtensa/src/common/xtensa_sigdeliver.c +++ b/arch/xtensa/src/common/xtensa_sigdeliver.c @@ -121,20 +121,19 @@ void xtensa_sig_deliver(void) sinfo("Resuming\n"); - /* Call enter_critical_section() to disable local interrupts before - * restoring local context. - * - * Here, we should not use up_irq_save() in SMP mode. - * For example, if we call up_irq_save() here and another CPU might - * have called up_cpu_pause() to this cpu, hence g_cpu_irqlock has - * been locked by the cpu, in this case, we would see a deadlock in - * later call of enter_critical_section() to restore irqcount. - * To avoid this situation, we need to call enter_critical_section(). +#ifdef CONFIG_SMP + /* Restore the saved 'irqcount' and recover the critical section + * spinlocks. */ -#ifdef CONFIG_SMP - enter_critical_section(); -#else + DEBUGASSERT(rtcb->irqcount == 0); + while (rtcb->irqcount < saved_irqcount) + { + enter_critical_section(); + } +#endif + +#ifndef CONFIG_SUPPRESS_INTERRUPTS up_irq_save(); #endif @@ -190,22 +189,6 @@ void xtensa_sig_deliver(void) rtcb->xcp.regs[REG_A0] = regs[REG_A0]; -#ifdef CONFIG_SMP - /* Restore the saved 'irqcount' and recover the critical section - * spinlocks. - * - * REVISIT: irqcount should be one from the above call to - * enter_critical_section(). Could the saved_irqcount be zero? That - * would be a problem. - */ - - DEBUGASSERT(rtcb->irqcount == 1); - while (rtcb->irqcount < saved_irqcount) - { - enter_critical_section(); - } -#endif - /* Then restore the correct state for this thread of execution. * NOTE: The co-processor state should already be correct. */