[PATCH] arm64: set __exception_irq_entry with __irq_entry as a default

Mark Rutland mark.rutland at arm.com
Tue Apr 25 06:39:51 PDT 2023 

On Tue, Apr 25, 2023 at 11:31:31AM +0900, Youngmin Nam wrote:
> On Mon, Apr 24, 2023 at 02:08:14PM +0100, Mark Rutland wrote:
> > On Mon, Apr 24, 2023 at 02:09:05PM +0200, Dmitry Vyukov wrote:
> > > On Mon, 24 Apr 2023 at 13:01, Mark Rutland <mark.rutland at arm.com> wrote:
> > > >
> > > > On Mon, Apr 24, 2023 at 10:04:36AM +0900, Youngmin Nam wrote:
> > > > > filter_irq_stacks() is supposed to cut entries which are related irq entries
> > > > > from its call stack.
> > > > > And in_irqentry_text() which is called by filter_irq_stacks()
> > > > > uses __irqentry_text_start/end symbol to find irq entries in callstack.
> > > > >
> > > > > But it doesn't work correctly as without "CONFIG_FUNCTION_GRAPH_TRACER",
> > > > > arm64 kernel doesn't include gic_handle_irq which is entry point of arm64 irq
> > > > > between __irqentry_text_start and __irqentry_text_end as we discussed in below link.
> > > >
> > > > TBH, the __irqentry_text annotations don't make much sense, and I'd love to
> > > > remove them.
> > > >
> > > > The irqchip handlers are not the actual exception entry points, and we invoke a
> > > > fair amount of code between those and the actual IRQ handlers (e.g. to map from
> > > > the irq domain to the actual hander, which might involve poking chained irqchip
> > > > handlers), so it doesn't make much sense for the irqchip handlers to be
> > > > special.
> > > >
> > > > > https://lore.kernel.org/all/CACT4Y+aReMGLYua2rCLHgFpS9io5cZC04Q8GLs-uNmrn1ezxYQ@mail.gmail.com/#t
> > > > >
> > > > > This problem can makes unintentional deep call stack entries especially
> > > > > in KASAN enabled situation as below.
> > > >
> > > > What exactly does KASAN need here? Is this just to limit the depth of the
> > > > trace?
> > > 
> > > No, it's not just depth. Any uses of stack depot need stable
> > > repeatable traces, so that they are deduplicated well. For irq stacks
> > > it means removing the random part where the interrupt is delivered.
> > > Otherwise stack depot grows without limits and overflows.
> 
> Hi Dmitry Vyukov.
> Thanks for your additional comments.
> 
> > 
> > Sure -- you want to filter out the non-deterministic context that the interrupt
> > was taken *from*.
> > 
> > > We don't need the exact entry point for this. A frame "close enough"
> > > may work well if there are no memory allocations/frees skipped.
> > 
> > With that in mind, I think what we should do is cut this at the instant we
> > enter the exception; for the trace below that would be el1h_64_irq. I've added
> > some line spacing there to make it stand out.
> > 
> > That would mean that we'd have three entry points that an interrupt trace might
> > start from:
> > 
> > * el1h_64_irq()
> > * el0t_64_irq()
> > * el0t_32_irq()
> >
> 
> Hi Mark.
> Thanks for your kind review.
> 
> If I understand your intention corretly, I should add "__irq_entry"
> to C function of irq_handler as below.

I'd meant something like the below, marking the assembly (as x86 does) rather
than the C code. I'll try to sort that out and send a proper patch series after
-rc1.

Thanks,
Mark.

---->8----
>From 7e54be3ea2420af348c710afab743b94ced72881 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland at arm.com>
Date: Tue, 25 Apr 2023 14:13:39 +0100
Subject: [PATCH] WIP: arm64: entry: handle irqentry consistently

Follow the example of x86 in commit:

  f0178fc01fe46bab ("x86/entry: Unbreak __irqentry_text_start/end magic")

... and consistently treat the asm IRQ/FIQ entry points as irqentry, and
nothing else.

TODO:
* Explain stackdepot details
* Explain why dropping __kprobes is fine
* Explain why placing entry asm in .irqentry.text is fine.
* Explain why we don't need to mark ret_to_* or the actual vector
* Check how this works for ftrace

Signed-off-by: Mark Rutland <mark.rutland at arm.com>
---
 arch/arm64/include/asm/exception.h | 10 +++++-----
 arch/arm64/include/asm/irq.h       |  6 ++++++
 arch/arm64/kernel/entry.S          | 20 +++++++++++---------
 drivers/irqchip/irq-dw-apb-ictl.c  |  4 +++-
 4 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
index 92963f98afece..6c74a8a3aad99 100644
--- a/arch/arm64/include/asm/exception.h
+++ b/arch/arm64/include/asm/exception.h
@@ -13,11 +13,11 @@
 
 #include <linux/interrupt.h>
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-#define __exception_irq_entry	__irq_entry
-#else
-#define __exception_irq_entry	__kprobes
-#endif
+/*
+ * irqchip drivers shared with arch/arm mark IRQ handlers with
+ * __exception_irq_entry. Make this a NOP for arm64.
+ */
+#define __exception_irq_entry
 
 static inline unsigned long disr_to_esr(u64 disr)
 {
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index fac08e18bcd51..a1af8fafc6cb5 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -6,6 +6,12 @@
 
 #include <asm-generic/irq.h>
 
+/*
+ * The irq entry code is in assembly. Make the build fail if
+ * something moves a C function into the __irq_entry section.
+ */
+#define __irq_entry __invalid_section
+
 struct pt_regs;
 
 int set_handle_irq(void (*handle_irq)(struct pt_regs *));
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ab2a6e33c0528..a66cd0ce79956 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -562,7 +562,8 @@ SYM_CODE_END(__bad_stack)
 #endif /* CONFIG_VMAP_STACK */
 
 
-	.macro entry_handler el:req, ht:req, regsize:req, label:req
+	.macro entry_handler el:req, ht:req, regsize:req, label:req, section = ".entry.text"
+	.pushsection \section, "ax"
 SYM_CODE_START_LOCAL(el\el\ht\()_\regsize\()_\label)
 	kernel_entry \el, \regsize
 	mov	x0, sp
@@ -573,29 +574,30 @@ SYM_CODE_START_LOCAL(el\el\ht\()_\regsize\()_\label)
 	b	ret_to_kernel
 	.endif
 SYM_CODE_END(el\el\ht\()_\regsize\()_\label)
+	.popsection
 	.endm
 
 /*
  * Early exception handlers
  */
 	entry_handler	1, t, 64, sync
-	entry_handler	1, t, 64, irq
-	entry_handler	1, t, 64, fiq
+	entry_handler	1, t, 64, irq,	".irqentry.text"
+	entry_handler	1, t, 64, fiq,	".irqentry.text"
 	entry_handler	1, t, 64, error
 
 	entry_handler	1, h, 64, sync
-	entry_handler	1, h, 64, irq
-	entry_handler	1, h, 64, fiq
+	entry_handler	1, h, 64, irq,	".irqentry.text"
+	entry_handler	1, h, 64, fiq,	".irqentry.text"
 	entry_handler	1, h, 64, error
 
 	entry_handler	0, t, 64, sync
-	entry_handler	0, t, 64, irq
-	entry_handler	0, t, 64, fiq
+	entry_handler	0, t, 64, irq,	".irqentry.text"
+	entry_handler	0, t, 64, fiq,	".irqentry.text"
 	entry_handler	0, t, 64, error
 
 	entry_handler	0, t, 32, sync
-	entry_handler	0, t, 32, irq
-	entry_handler	0, t, 32, fiq
+	entry_handler	0, t, 32, irq,	".irqentry.text"
+	entry_handler	0, t, 32, fiq,	".irqentry.text"
 	entry_handler	0, t, 32, error
 
 SYM_CODE_START_LOCAL(ret_to_kernel)
diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index d5c1c750c8d2d..ad315bdfc3ef6 100644
--- a/drivers/irqchip/irq-dw-apb-ictl.c
+++ b/drivers/irqchip/irq-dw-apb-ictl.c
@@ -19,6 +19,8 @@
 #include <linux/of_irq.h>
 #include <linux/interrupt.h>
 
+#include <asm/exception.h>
+
 #define APB_INT_ENABLE_L	0x00
 #define APB_INT_ENABLE_H	0x04
 #define APB_INT_MASK_L		0x08
@@ -30,7 +32,7 @@
 /* irq domain of the primary interrupt controller. */
 static struct irq_domain *dw_apb_ictl_irq_domain;
 
-static void __irq_entry dw_apb_ictl_handle_irq(struct pt_regs *regs)
+static void __exception_irq_entry dw_apb_ictl_handle_irq(struct pt_regs *regs)
 {
 	struct irq_domain *d = dw_apb_ictl_irq_domain;
 	int n;
-- 
2.30.2

More information about the linux-arm-kernel mailing list