Skip to contentSkip to conversation

kprobes/x86: Do not jump-optimize kprobes on irq entry code

PATCH In review Masami Hiramatsu 2 participants 3 messages Jul 27, 2017

Subsystems (2)
x86 Generic include/asm header files
lore
Share & tools
lore

Masami Hiramatsu

author
#0 lore raw

PATCH v4 1/1 kprobes/x86: Do not jump-optimize kprobes on irq entry code

Since the kernel segment registers are not prepared at the entry of irq-entry code, if a kprobe on such code is jump-optimized, accessing per-cpu variables may cause kernel panic.
However, if the kprobe is not optimized, it kicks int3 exception and set segment registers correctly.

This checks probe-address and if it is in irq-entry code, it prohibits optimizing such kprobes. This means we can continuously probing such interrupt handlers by kprobes but it is not optimized anymore.

Signed-off-by Masami Hiramatsu <[email protected]>
Reported-by Francis Deslauriers <[email protected]>
Tested-by Francis Deslauriers <[email protected]>

---
Changes in v4:
- Fix to use CONFIG_OPTPROBES instead of CONFIG_KPROBES.

---
 arch/x86/kernel/kprobes/opt.c     |    9 ++++++---
 include/asm-generic/vmlinux.lds.h |    6 ++++--
 include/linux/interrupt.h         |    3 ++-
 3 files changed, 12 insertions(+), 6 deletions(-)
arch/x86/kernel/kprobes/opt.c +6 -3
index 69ea0bc..c26e7f9 100644
@@ -29,6 +29,7 @@
 #include <linux/kallsyms.h>
 #include <linux/ftrace.h>
 #include <linux/frame.h>
+#include <linux/interrupt.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

@@ -251,10 +252,12 @@ static int can_optimize(unsigned long paddr)
 
 	/*
 	 * Do not optimize in the entry code due to the unstable
-	 * stack handling.
+	 * stack handling and registers setup.
 	 */
-	if ((paddr >= (unsigned long)__entry_text_start) &&
-	    (paddr <  (unsigned long)__entry_text_end))
+	if (((paddr >= (unsigned long)__entry_text_start) &&
+	     (paddr <  (unsigned long)__entry_text_end)) ||
+	    ((paddr >= (unsigned long)__irqentry_text_start) &&
+	     (paddr <  (unsigned long)__irqentry_text_end)))
 		return 0;
 
 	/* Check there is enough space for a relative jump. */
include/asm-generic/vmlinux.lds.h +4 -2
index da0be9a..3092a1f 100644
@@ -483,7 +483,8 @@
 		*(.entry.text)						\
 		VMLINUX_SYMBOL(__entry_text_end) = .;
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) ||	\
+	defined(CONFIG_OPTPROBES)
 #define IRQENTRY_TEXT							\
 		ALIGN_FUNCTION();					\
 		VMLINUX_SYMBOL(__irqentry_text_start) = .;		\

@@ -493,7 +494,8 @@
 #define IRQENTRY_TEXT
 #endif
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) ||	\
+	defined(CONFIG_OPTPROBES)
 #define SOFTIRQENTRY_TEXT						\
 		ALIGN_FUNCTION();					\
 		VMLINUX_SYMBOL(__softirqentry_text_start) = .;		\
include/linux/interrupt.h +2 -1
index a2fdddd..f95b046 100644
@@ -726,7 +726,8 @@ extern int early_irq_init(void);
 extern int arch_probe_nr_irqs(void);
 extern int arch_early_irq_init(void);
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) || \
+	defined(CONFIG_OPTPROBES)
 /*
  * We want to know which function is an entrypoint of a hardirq or a softirq.
  */
#1 lore raw
Quoted 81 lines · Masami Hiramatsu

Since the kernel segment registers are not prepared at the entry of irq-entry code, if a kprobe on such code is jump-optimized, accessing per-cpu variables may cause kernel panic.
However, if the kprobe is not optimized, it kicks int3 exception and set segment registers correctly.

This checks probe-address and if it is in irq-entry code, it prohibits optimizing such kprobes. This means we can continuously probing such interrupt handlers by kprobes but it is not optimized anymore.

Signed-off-by Masami Hiramatsu <[email protected]>
Reported-by Francis Deslauriers <[email protected]>
Tested-by Francis Deslauriers <[email protected]>

---
Changes in v4:
- Fix to use CONFIG_OPTPROBES instead of CONFIG_KPROBES.

---
 arch/x86/kernel/kprobes/opt.c     |    9 ++++++---
 include/asm-generic/vmlinux.lds.h |    6 ++++--
 include/linux/interrupt.h         |    3 ++-
 3 files changed, 12 insertions(+), 6 deletions(-)
arch/x86/kernel/kprobes/opt.c +6 -3
index 69ea0bc..c26e7f9 100644
@@ -29,6 +29,7 @@
 #include <linux/kallsyms.h>
 #include <linux/ftrace.h>
 #include <linux/frame.h>
+#include <linux/interrupt.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

@@ -251,10 +252,12 @@ static int can_optimize(unsigned long paddr)
 
 	/*
 	 * Do not optimize in the entry code due to the unstable
-	 * stack handling.
+	 * stack handling and registers setup.
 	 */
-	if ((paddr >= (unsigned long)__entry_text_start) &&
-	    (paddr <  (unsigned long)__entry_text_end))
+	if (((paddr >= (unsigned long)__entry_text_start) &&
+	     (paddr <  (unsigned long)__entry_text_end)) ||
+	    ((paddr >= (unsigned long)__irqentry_text_start) &&
+	     (paddr <  (unsigned long)__irqentry_text_end)))
 		return 0;
 
 	/* Check there is enough space for a relative jump. */
include/asm-generic/vmlinux.lds.h +4 -2
index da0be9a..3092a1f 100644
@@ -483,7 +483,8 @@
 		*(.entry.text)						\
 		VMLINUX_SYMBOL(__entry_text_end) = .;
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) ||	\
+	defined(CONFIG_OPTPROBES)
 #define IRQENTRY_TEXT							\
 		ALIGN_FUNCTION();					\
 		VMLINUX_SYMBOL(__irqentry_text_start) = .;		\

@@ -493,7 +494,8 @@
 #define IRQENTRY_TEXT
 #endif
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) ||	\
+	defined(CONFIG_OPTPROBES)
 #define SOFTIRQENTRY_TEXT						\
 		ALIGN_FUNCTION();					\
 		VMLINUX_SYMBOL(__softirqentry_text_start) = .;		\
include/linux/interrupt.h +0 -0
index a2fdddd..f95b046 100644

include/linux/interrupt.h extern int early_irq_init(void); · line 730

Quoted 6 lines
include/linux/interrupt.h +2 -1
@@ -726,7 +726,8 @@ extern int early_irq_init(void);
 extern int arch_probe_nr_irqs(void);
 extern int arch_early_irq_init(void);
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) || \
+	defined(CONFIG_OPTPROBES)

Please split the generic changes to asm-generic/vmlinux.lds.h and interrupt.h into a separate patch, with its own changelog, because this is not a kprobes/x86 only patch anymore.

Thanks,

Ingo

author
#2 lore raw
Quoted 87 lines · On Thu, 27 Jul 2017 10:24:46 +0200 Ingo Molnar <[email protected]> wrote:
Quoted 84 lines · Masami Hiramatsu

Since the kernel segment registers are not prepared at the entry of irq-entry code, if a kprobe on such code is jump-optimized, accessing per-cpu variables may cause kernel panic.
However, if the kprobe is not optimized, it kicks int3 exception and set segment registers correctly.

This checks probe-address and if it is in irq-entry code, it prohibits optimizing such kprobes. This means we can continuously probing such interrupt handlers by kprobes but it is not optimized anymore.

Signed-off-by Masami Hiramatsu <[email protected]>
Reported-by Francis Deslauriers <[email protected]>
Tested-by Francis Deslauriers <[email protected]>

---
Changes in v4:
- Fix to use CONFIG_OPTPROBES instead of CONFIG_KPROBES.

---
 arch/x86/kernel/kprobes/opt.c     |    9 ++++++---
 include/asm-generic/vmlinux.lds.h |    6 ++++--
 include/linux/interrupt.h         |    3 ++-
 3 files changed, 12 insertions(+), 6 deletions(-)
arch/x86/kernel/kprobes/opt.c +6 -3
index 69ea0bc..c26e7f9 100644
@@ -29,6 +29,7 @@
 #include <linux/kallsyms.h>
 #include <linux/ftrace.h>
 #include <linux/frame.h>
+#include <linux/interrupt.h>
 
 #include <asm/text-patching.h>
 #include <asm/cacheflush.h>

@@ -251,10 +252,12 @@ static int can_optimize(unsigned long paddr)
 
 	/*
 	 * Do not optimize in the entry code due to the unstable
-	 * stack handling.
+	 * stack handling and registers setup.
 	 */
-	if ((paddr >= (unsigned long)__entry_text_start) &&
-	    (paddr <  (unsigned long)__entry_text_end))
+	if (((paddr >= (unsigned long)__entry_text_start) &&
+	     (paddr <  (unsigned long)__entry_text_end)) ||
+	    ((paddr >= (unsigned long)__irqentry_text_start) &&
+	     (paddr <  (unsigned long)__irqentry_text_end)))
 		return 0;
 
 	/* Check there is enough space for a relative jump. */
include/asm-generic/vmlinux.lds.h +4 -2
index da0be9a..3092a1f 100644
@@ -483,7 +483,8 @@
 		*(.entry.text)						\
 		VMLINUX_SYMBOL(__entry_text_end) = .;
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) ||	\
+	defined(CONFIG_OPTPROBES)
 #define IRQENTRY_TEXT							\
 		ALIGN_FUNCTION();					\
 		VMLINUX_SYMBOL(__irqentry_text_start) = .;		\

@@ -493,7 +494,8 @@
 #define IRQENTRY_TEXT
 #endif
 
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) ||	\
+	defined(CONFIG_OPTPROBES)
 #define SOFTIRQENTRY_TEXT						\
 		ALIGN_FUNCTION();					\
 		VMLINUX_SYMBOL(__softirqentry_text_start) = .;		\
include/linux/interrupt.h +0 -0
index a2fdddd..f95b046 100644
@@ -726,7 +726,8 @@ extern int early_irq_init(void);
 extern int arch_probe_nr_irqs(void);
 extern int arch_early_irq_init(void);
Quoted 7 lines

include/linux/interrupt.h extern int early_irq_init(void); · line 730

Quoted 3 lines
include/linux/interrupt.h +2 -1
-#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN)
+#if defined(CONFIG_FUNCTION_GRAPH_TRACER) || defined(CONFIG_KASAN) || \
+	defined(CONFIG_OPTPROBES)

Please split the generic changes to asm-generic/vmlinux.lds.h and interrupt.h into a separate patch, with its own changelog, because this is not a kprobes/x86 only patch anymore.

OK. BTW, I think it is a good time to make a new (hidden) Kconfig entry for irqentry, like CONFIG_IRQENTRY and above 3 configs select it. What would you think?

Thank you,

Quoted 4 lines

Thanks,

Ingo

Signature · 1 line
Masami Hiramatsu <[email protected]>