All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pingfan Liu <kernelfans@gmail.com>
To: linux-arm-kernel@lists.infradead.org
Cc: Pingfan Liu <kernelfans@gmail.com>,
	Mark Rutland <mark.rutland@arm.com>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Joey Gouly <joey.gouly@arm.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	Julien Thierry <julien.thierry@arm.com>,
	Yuichi Ito <ito-yuichi@fujitsu.com>,
	rcu@vger.kernel.org
Subject: [PATCHv3 1/4] arm64: entry: judge nmi ealier to avoid deadlock in RCU
Date: Tue, 16 Nov 2021 16:24:47 +0800	[thread overview]
Message-ID: <20211116082450.10357-2-kernelfans@gmail.com> (raw)
In-Reply-To: <20211116082450.10357-1-kernelfans@gmail.com>

Linux kernel places strict semantics between NMI and maskable interrupt.
So does the RCU component, else deadlock may happen. But the current
arm64 entry code can partially breach this rule through calling
rcu_nmi_enter().

***  how a deadlock can happen if NMI mistaken as IRQ  ***

    rcu_nmi_enter()
    {

        if (rcu_dynticks_curr_cpu_in_eqs()) {

        	if (!in_nmi())
        		rcu_dynticks_task_exit();
                ...
        	if (!in_nmi()) {
        		instrumentation_begin();
        		rcu_cleanup_after_idle();
        		instrumentation_end();
        	}
                ...
        } else if (!in_nmi()) {
        	instrumentation_begin();
        	rcu_irq_enter_check_tick();
        }

    }

If a NMI is mistaken as a maskable interrupt, rcu_irq_enter_check_tick()
can hit a deadlock, which is demonstrated by the following scenario:

    note_gp_changes() runs in a task context
    {
       local_irq_save(flags); // this protects against irq, but not NMI
       rnp = rdp->mynode;
       ...
       raw_spin_trylock_rcu_node(rnp)
                                  -------> broken in by (p)NMI, without taking __nmi_enter()
                                           rcu_nmi_enter()
                                            ->__rcu_irq_enter_check_tick()
                                               ->raw_spin_lock_rcu_node(rdp->mynode);
                                                   deadlock happens!!!

    }

*** On arm64, how pNMI mistaken as IRQ ***

On arm64, pNMI is an analogue to NMI. In essence, it is a higher
priority interrupt but not disabled by local_irq_disable().

In current implementation
1) If a pNMI from a context where IRQs were masked, it can be recognized
   as nmi, and calls __nmi_enter() immediately. This is no problem.

2) But it causes trouble if a pNMI from a context where IRQs were
   unmasked, and temporarily regarded as maskable interrupt.  It is not
   treated as NMI, i.e. calling nmi_enter() until reading from GIC.

    __el1_irq()
    {
      irq_enter_rcu()   ----> hit the deadlock bug
        gic_handle_nmi()
          nmi_enter()
          nmi_exit()
      irq_exit_rcu()
    }

*** Remedy ***

If the irqchip level exposes an interface for detecting pNMI to arch level
code, it can meet the requirement at this early stage. That is the
interface (*interrupt_is_nmi)() in this patch.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: rcu@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm64/include/asm/irq.h     |  1 +
 arch/arm64/kernel/entry-common.c | 10 +++++++++-
 arch/arm64/kernel/irq.c          | 18 ++++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index fac08e18bcd5..f3eb13bfa65e 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -11,6 +11,7 @@ struct pt_regs;
 int set_handle_irq(void (*handle_irq)(struct pt_regs *));
 #define set_handle_irq	set_handle_irq
 int set_handle_fiq(void (*handle_fiq)(struct pt_regs *));
+int set_nmi_discriminator(bool (*discriminator)(void));
 
 static inline int nr_legacy_irqs(void)
 {
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index f7408edf8571..5a1a5dd66d04 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -260,6 +260,14 @@ static void do_interrupt_handler(struct pt_regs *regs,
 
 extern void (*handle_arch_irq)(struct pt_regs *);
 extern void (*handle_arch_fiq)(struct pt_regs *);
+extern bool (*interrupt_is_nmi)(void);
+
+static inline bool is_in_pnmi(struct pt_regs *regs)
+{
+	if (!interrupts_enabled(regs) || (*interrupt_is_nmi)())
+		return true;
+	return false;
+}
 
 static void noinstr __panic_unhandled(struct pt_regs *regs, const char *vector,
 				      unsigned int esr)
@@ -454,7 +462,7 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
 {
 	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
 
-	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
+	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && is_in_pnmi(regs))
 		__el1_pnmi(regs, handler);
 	else
 		__el1_irq(regs, handler);
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index bda49430c9ea..fabed09ed966 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -81,8 +81,14 @@ static void default_handle_fiq(struct pt_regs *regs)
 	panic("FIQ taken without a root FIQ handler\n");
 }
 
+static bool default_nmi_discriminator(void)
+{
+	return false;
+}
+
 void (*handle_arch_irq)(struct pt_regs *) __ro_after_init = default_handle_irq;
 void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init = default_handle_fiq;
+bool (*interrupt_is_nmi)(void) __ro_after_init = default_nmi_discriminator;
 
 int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 {
@@ -104,6 +110,18 @@ int __init set_handle_fiq(void (*handle_fiq)(struct pt_regs *))
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI)
+
+int __init set_nmi_discriminator(bool (*discriminator)(void))
+{
+	if (interrupt_is_nmi != default_nmi_discriminator)
+		return -EBUSY;
+
+	interrupt_is_nmi = discriminator;
+	return 0;
+}
+#endif
+
 void __init init_IRQ(void)
 {
 	init_irq_stacks();
-- 
2.31.1


WARNING: multiple messages have this Message-ID (diff)
From: Pingfan Liu <kernelfans@gmail.com>
To: linux-arm-kernel@lists.infradead.org
Cc: Pingfan Liu <kernelfans@gmail.com>,
	Mark Rutland <mark.rutland@arm.com>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Joey Gouly <joey.gouly@arm.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	Julien Thierry <julien.thierry@arm.com>,
	Yuichi Ito <ito-yuichi@fujitsu.com>,
	rcu@vger.kernel.org
Subject: [PATCHv3 1/4] arm64: entry: judge nmi ealier to avoid deadlock in RCU
Date: Tue, 16 Nov 2021 16:24:47 +0800	[thread overview]
Message-ID: <20211116082450.10357-2-kernelfans@gmail.com> (raw)
In-Reply-To: <20211116082450.10357-1-kernelfans@gmail.com>

Linux kernel places strict semantics between NMI and maskable interrupt.
So does the RCU component, else deadlock may happen. But the current
arm64 entry code can partially breach this rule through calling
rcu_nmi_enter().

***  how a deadlock can happen if NMI mistaken as IRQ  ***

    rcu_nmi_enter()
    {

        if (rcu_dynticks_curr_cpu_in_eqs()) {

        	if (!in_nmi())
        		rcu_dynticks_task_exit();
                ...
        	if (!in_nmi()) {
        		instrumentation_begin();
        		rcu_cleanup_after_idle();
        		instrumentation_end();
        	}
                ...
        } else if (!in_nmi()) {
        	instrumentation_begin();
        	rcu_irq_enter_check_tick();
        }

    }

If a NMI is mistaken as a maskable interrupt, rcu_irq_enter_check_tick()
can hit a deadlock, which is demonstrated by the following scenario:

    note_gp_changes() runs in a task context
    {
       local_irq_save(flags); // this protects against irq, but not NMI
       rnp = rdp->mynode;
       ...
       raw_spin_trylock_rcu_node(rnp)
                                  -------> broken in by (p)NMI, without taking __nmi_enter()
                                           rcu_nmi_enter()
                                            ->__rcu_irq_enter_check_tick()
                                               ->raw_spin_lock_rcu_node(rdp->mynode);
                                                   deadlock happens!!!

    }

*** On arm64, how pNMI mistaken as IRQ ***

On arm64, pNMI is an analogue to NMI. In essence, it is a higher
priority interrupt but not disabled by local_irq_disable().

In current implementation
1) If a pNMI from a context where IRQs were masked, it can be recognized
   as nmi, and calls __nmi_enter() immediately. This is no problem.

2) But it causes trouble if a pNMI from a context where IRQs were
   unmasked, and temporarily regarded as maskable interrupt.  It is not
   treated as NMI, i.e. calling nmi_enter() until reading from GIC.

    __el1_irq()
    {
      irq_enter_rcu()   ----> hit the deadlock bug
        gic_handle_nmi()
          nmi_enter()
          nmi_exit()
      irq_exit_rcu()
    }

*** Remedy ***

If the irqchip level exposes an interface for detecting pNMI to arch level
code, it can meet the requirement at this early stage. That is the
interface (*interrupt_is_nmi)() in this patch.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Joey Gouly <joey.gouly@arm.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Julien Thierry <julien.thierry@arm.com>
Cc: Yuichi Ito <ito-yuichi@fujitsu.com>
Cc: rcu@vger.kernel.org
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm64/include/asm/irq.h     |  1 +
 arch/arm64/kernel/entry-common.c | 10 +++++++++-
 arch/arm64/kernel/irq.c          | 18 ++++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index fac08e18bcd5..f3eb13bfa65e 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -11,6 +11,7 @@ struct pt_regs;
 int set_handle_irq(void (*handle_irq)(struct pt_regs *));
 #define set_handle_irq	set_handle_irq
 int set_handle_fiq(void (*handle_fiq)(struct pt_regs *));
+int set_nmi_discriminator(bool (*discriminator)(void));
 
 static inline int nr_legacy_irqs(void)
 {
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
index f7408edf8571..5a1a5dd66d04 100644
--- a/arch/arm64/kernel/entry-common.c
+++ b/arch/arm64/kernel/entry-common.c
@@ -260,6 +260,14 @@ static void do_interrupt_handler(struct pt_regs *regs,
 
 extern void (*handle_arch_irq)(struct pt_regs *);
 extern void (*handle_arch_fiq)(struct pt_regs *);
+extern bool (*interrupt_is_nmi)(void);
+
+static inline bool is_in_pnmi(struct pt_regs *regs)
+{
+	if (!interrupts_enabled(regs) || (*interrupt_is_nmi)())
+		return true;
+	return false;
+}
 
 static void noinstr __panic_unhandled(struct pt_regs *regs, const char *vector,
 				      unsigned int esr)
@@ -454,7 +462,7 @@ static void noinstr el1_interrupt(struct pt_regs *regs,
 {
 	write_sysreg(DAIF_PROCCTX_NOIRQ, daif);
 
-	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && !interrupts_enabled(regs))
+	if (IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) && is_in_pnmi(regs))
 		__el1_pnmi(regs, handler);
 	else
 		__el1_irq(regs, handler);
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index bda49430c9ea..fabed09ed966 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -81,8 +81,14 @@ static void default_handle_fiq(struct pt_regs *regs)
 	panic("FIQ taken without a root FIQ handler\n");
 }
 
+static bool default_nmi_discriminator(void)
+{
+	return false;
+}
+
 void (*handle_arch_irq)(struct pt_regs *) __ro_after_init = default_handle_irq;
 void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init = default_handle_fiq;
+bool (*interrupt_is_nmi)(void) __ro_after_init = default_nmi_discriminator;
 
 int __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 {
@@ -104,6 +110,18 @@ int __init set_handle_fiq(void (*handle_fiq)(struct pt_regs *))
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI)
+
+int __init set_nmi_discriminator(bool (*discriminator)(void))
+{
+	if (interrupt_is_nmi != default_nmi_discriminator)
+		return -EBUSY;
+
+	interrupt_is_nmi = discriminator;
+	return 0;
+}
+#endif
+
 void __init init_IRQ(void)
 {
 	init_irq_stacks();
-- 
2.31.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-11-16  8:25 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-16  8:24 [PATCHv3 0/4] arm64: Fixes RCU deadlock due to a mistaken Pingfan Liu
2021-11-16  8:24 ` Pingfan Liu
2021-11-16  8:24 ` Pingfan Liu [this message]
2021-11-16  8:24   ` [PATCHv3 1/4] arm64: entry: judge nmi ealier to avoid deadlock in RCU Pingfan Liu
2021-11-17 11:38   ` Mark Rutland
2021-11-17 11:38     ` Mark Rutland
2021-11-19  2:01     ` Pingfan Liu
2021-11-19  2:01       ` Pingfan Liu
2021-11-19 14:04       ` Mark Rutland
2021-11-19 14:04         ` Mark Rutland
2021-11-16  8:24 ` [PATCHv3 2/4] arm64: entry: distinguish pNMI earlier in el0 interrupt Pingfan Liu
2021-11-16  8:24   ` Pingfan Liu
2021-11-16  8:24 ` [PATCHv3 3/4] irqchip: GICv3: expose pNMI discriminator Pingfan Liu
2021-11-16  8:24   ` Pingfan Liu
2021-11-16  9:53   ` Marc Zyngier
2021-11-16  9:53     ` Marc Zyngier
2021-11-17 10:16     ` Pingfan Liu
2021-11-17 10:16       ` Pingfan Liu
2021-11-17 11:01       ` Marc Zyngier
2021-11-17 11:01         ` Marc Zyngier
2021-11-19  2:38         ` Pingfan Liu
2021-11-19  2:38           ` Pingfan Liu
2021-11-16  8:24 ` [PATCHv3 4/4] arm64: entry: remove pNMI judgement in __el1_interrupt() path Pingfan Liu
2021-11-16  8:24   ` Pingfan Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211116082450.10357-2-kernelfans@gmail.com \
    --to=kernelfans@gmail.com \
    --cc=catalin.marinas@arm.com \
    --cc=ito-yuichi@fujitsu.com \
    --cc=joey.gouly@arm.com \
    --cc=julien.thierry@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=paulmck@kernel.org \
    --cc=rcu@vger.kernel.org \
    --cc=samitolvanen@google.com \
    --cc=tglx@linutronix.de \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.