All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/4] Blacklist powerpc exception vectors from kprobes
@ 2016-11-17 15:08 Naveen N. Rao
  2016-11-17 15:08 ` [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals Naveen N. Rao
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-17 15:08 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

This is the beginning of work to come up with a more relevant kprobe
blacklist on powerpc. In this series, we primarily blacklist exception
vectors and kernel entry code.

Naveen N. Rao (4):
  powerpc: asm: introduce new macros for assembly globals
  powerpc: kprobe: add arch specific blacklist
  powerpc: mm/slb: convert slb_low.S to use the new macros
  powerpc: mm/slb: blacklist symbols from kprobe

 arch/powerpc/include/asm/ppc_asm.h | 19 +++++++++++++++++--
 arch/powerpc/kernel/entry_32.S     |  2 ++
 arch/powerpc/kernel/entry_64.S     |  2 ++
 arch/powerpc/kernel/kprobes.c      | 10 ++++++++++
 arch/powerpc/mm/slb_low.S          | 30 +++++++++++++-----------------
 5 files changed, 44 insertions(+), 19 deletions(-)

-- 
2.10.2

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals
  2016-11-17 15:08 [RFC PATCH 0/4] Blacklist powerpc exception vectors from kprobes Naveen N. Rao
@ 2016-11-17 15:08 ` Naveen N. Rao
  2016-11-18  9:41   ` Michael Ellerman
  2016-11-17 15:08 ` [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist Naveen N. Rao
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-17 15:08 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

- Introduce _GLOBAL_SYM() for global symbols in assembly. This helps
reduce verbosity of assembly files.
- Introduce NOKPROBE variants of _GLOBAL() and _GLOBAL_SYM(). These are
used subsequently to blacklist certain assembly functions and symbols
from kprobe.
- Fix a small typo in kprobe comment and re-format it, to make it
clearer.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/ppc_asm.h | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h
index 025833b..2443545 100644
--- a/arch/powerpc/include/asm/ppc_asm.h
+++ b/arch/powerpc/include/asm/ppc_asm.h
@@ -254,13 +254,19 @@ GLUE(.,name):
 
 #endif
 
+#define _GLOBAL_SYM(name)	\
+	.globl name;		\
+name:
+
 /*
  * __kprobes (the C annotation) puts the symbol into the .kprobes.text
  * section, which gets emitted at the end of regular text.
  *
  * _ASM_NOKPROBE_SYMBOL and NOKPROBE_SYMBOL just adds the symbol to
- * a blacklist. The former is for core kprobe functions/data, the
- * latter is for those that incdentially must be excluded from probing
+ * a blacklist.
+ *
+ * The former (__kprobes) is for core kprobe functions/data, the
+ * latter is for those that incidentally must be excluded from probing
  * and allows them to be linked at more optimal location within text.
  */
 #ifdef CONFIG_KPROBES
@@ -272,6 +278,15 @@ GLUE(.,name):
 #define _ASM_NOKPROBE_SYMBOL(entry)
 #endif
 
+#define _GLOBAL_NOKPROBE(name)				\
+	_GLOBAL(name);					\
+	_ASM_NOKPROBE_SYMBOL(name)
+
+#define _GLOBAL_SYM_NOKPROBE(name)			\
+	_GLOBAL_SYM(name);				\
+	_ASM_NOKPROBE_SYMBOL(name)
+
+
 #define FUNC_START(name)	_GLOBAL(name)
 #define FUNC_END(name)
 
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist
  2016-11-17 15:08 [RFC PATCH 0/4] Blacklist powerpc exception vectors from kprobes Naveen N. Rao
  2016-11-17 15:08 ` [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals Naveen N. Rao
@ 2016-11-17 15:08 ` Naveen N. Rao
  2016-11-18  5:48   ` Michael Ellerman
  2016-11-17 15:08 ` [RFC PATCH 3/4] powerpc: mm/slb: convert slb_low.S to use the new macros Naveen N. Rao
  2016-11-17 15:08 ` [RFC PATCH 4/4] powerpc: mm/slb: blacklist symbols from kprobe Naveen N. Rao
  3 siblings, 1 reply; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-17 15:08 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

Add symbol to mark end of entry_*.S and use the same to blacklist all
addresses from kernel start (_stext) to entry code from kprobes. Much of
this code is early exception handling where we can't really take a trap.

Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/entry_32.S |  2 ++
 arch/powerpc/kernel/entry_64.S |  2 ++
 arch/powerpc/kernel/kprobes.c  | 10 ++++++++++
 3 files changed, 14 insertions(+)

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 3841d74..de1ed6e 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -1410,3 +1410,5 @@ _GLOBAL(return_to_handler)
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
 #endif /* CONFIG_FUNCTION_TRACER */
+
+_GLOBAL_SYM(__entry_text_end)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 6432d4b..f5f99920 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1551,3 +1551,5 @@ _GLOBAL(return_to_handler)
 	blr
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 #endif /* CONFIG_FUNCTION_TRACER */
+
+_GLOBAL_SYM(__entry_text_end)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 9479d8e..b5173d6 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -36,12 +36,22 @@
 #include <asm/cacheflush.h>
 #include <asm/sstep.h>
 #include <asm/uaccess.h>
+#include <asm/sections.h>
 
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
 
+bool arch_within_kprobe_blacklist(unsigned long addr)
+{
+	/* The __kprobes marked functions and entry code must not be probed */
+	return (addr >= (unsigned long)__kprobes_text_start &&
+	        addr < (unsigned long)__kprobes_text_end) ||
+	       (addr >= (unsigned long)_stext &&
+		addr < (unsigned long)__entry_text_end);
+}
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	int ret = 0;
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 3/4] powerpc: mm/slb: convert slb_low.S to use the new macros
  2016-11-17 15:08 [RFC PATCH 0/4] Blacklist powerpc exception vectors from kprobes Naveen N. Rao
  2016-11-17 15:08 ` [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals Naveen N. Rao
  2016-11-17 15:08 ` [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist Naveen N. Rao
@ 2016-11-17 15:08 ` Naveen N. Rao
  2016-11-17 15:08 ` [RFC PATCH 4/4] powerpc: mm/slb: blacklist symbols from kprobe Naveen N. Rao
  3 siblings, 0 replies; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-17 15:08 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

Also convert slb_finish_load[_1T] to a local symbol as this doesn't need
to be globally visible.

Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/mm/slb_low.S | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/mm/slb_low.S b/arch/powerpc/mm/slb_low.S
index e2974fc..c1c7456 100644
--- a/arch/powerpc/mm/slb_low.S
+++ b/arch/powerpc/mm/slb_low.S
@@ -59,8 +59,7 @@ _GLOBAL(slb_allocate_realmode)
 	/* Linear mapping encoding bits, the "li" instruction below will
 	 * be patched by the kernel at boot
 	 */
-.globl slb_miss_kernel_load_linear
-slb_miss_kernel_load_linear:
+_GLOBAL_SYM(slb_miss_kernel_load_linear)
 	li	r11,0
 	/*
 	 * context = (MAX_USER_CONTEXT) + ((ea >> 60) - 0xc) + 1
@@ -71,17 +70,16 @@ slb_miss_kernel_load_linear:
 
 
 BEGIN_FTR_SECTION
-	b	slb_finish_load
+	b	.Lslb_finish_load
 END_MMU_FTR_SECTION_IFCLR(MMU_FTR_1T_SEGMENT)
-	b	slb_finish_load_1T
+	b	.Lslb_finish_load_1T
 
 1:
 #ifdef CONFIG_SPARSEMEM_VMEMMAP
 	/* Check virtual memmap region. To be patches at kernel boot */
 	cmpldi	cr0,r9,0xf
 	bne	1f
-.globl slb_miss_kernel_load_vmemmap
-slb_miss_kernel_load_vmemmap:
+_GLOBAL_SYM(slb_miss_kernel_load_vmemmap)
 	li	r11,0
 	b	6f
 1:
@@ -97,8 +95,7 @@ slb_miss_kernel_load_vmemmap:
 	b	6f
 5:
 	/* IO mapping */
-.globl slb_miss_kernel_load_io
-slb_miss_kernel_load_io:
+_GLOBAL_SYM(slb_miss_kernel_load_io)
 	li	r11,0
 6:
 	/*
@@ -109,9 +106,9 @@ slb_miss_kernel_load_io:
 	addi	r9,r9,(MAX_USER_CONTEXT - 0xc + 1)@l
 
 BEGIN_FTR_SECTION
-	b	slb_finish_load
+	b	.Lslb_finish_load
 END_MMU_FTR_SECTION_IFCLR(MMU_FTR_1T_SEGMENT)
-	b	slb_finish_load_1T
+	b	.Lslb_finish_load_1T
 
 0:	/*
 	 * For userspace addresses, make sure this is region 0.
@@ -174,9 +171,9 @@ END_MMU_FTR_SECTION_IFCLR(MMU_FTR_1T_SEGMENT)
 	ld	r9,PACACONTEXTID(r13)
 BEGIN_FTR_SECTION
 	cmpldi	r10,0x1000
-	bge	slb_finish_load_1T
+	bge	.Lslb_finish_load_1T
 END_MMU_FTR_SECTION_IFSET(MMU_FTR_1T_SEGMENT)
-	b	slb_finish_load
+	b	.Lslb_finish_load
 
 8:	/* invalid EA - return an error indication */
 	crset	4*cr0+eq		/* indicate failure */
@@ -187,7 +184,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_1T_SEGMENT)
  *
  * r3 = EA, r9 = context, r10 = ESID, r11 = flags, clobbers r9, cr7 = <> PAGE_OFFSET
  */
-slb_finish_load:
+.Lslb_finish_load:
 	rldimi  r10,r9,ESID_BITS,0
 	ASM_VSID_SCRAMBLE(r10,r9,256M)
 	/*
@@ -206,8 +203,7 @@ slb_finish_load:
 7:	ld	r10,PACASTABRR(r13)
 	addi	r10,r10,1
 	/* This gets soft patched on boot. */
-.globl slb_compare_rr_to_size
-slb_compare_rr_to_size:
+_GLOBAL_SYM(slb_compare_rr_to_size)
 	cmpldi	r10,0
 
 	blt+	4f
@@ -256,7 +252,7 @@ slb_compare_rr_to_size:
  *
  * r3 = EA, r9 = context, r10 = ESID(256MB), r11 = flags, clobbers r9
  */
-slb_finish_load_1T:
+.Lslb_finish_load_1T:
 	srdi	r10,r10,(SID_SHIFT_1T - SID_SHIFT)	/* get 1T ESID */
 	rldimi  r10,r9,ESID_BITS_1T,0
 	ASM_VSID_SCRAMBLE(r10,r9,1T)
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC PATCH 4/4] powerpc: mm/slb: blacklist symbols from kprobe
  2016-11-17 15:08 [RFC PATCH 0/4] Blacklist powerpc exception vectors from kprobes Naveen N. Rao
                   ` (2 preceding siblings ...)
  2016-11-17 15:08 ` [RFC PATCH 3/4] powerpc: mm/slb: convert slb_low.S to use the new macros Naveen N. Rao
@ 2016-11-17 15:08 ` Naveen N. Rao
  2016-11-18  9:36   ` Michael Ellerman
  3 siblings, 1 reply; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-17 15:08 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

We can't really take a trap at this point. So, blacklist these symbols.

Reported-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
 arch/powerpc/mm/slb_low.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/mm/slb_low.S b/arch/powerpc/mm/slb_low.S
index c1c7456..c2bae92 100644
--- a/arch/powerpc/mm/slb_low.S
+++ b/arch/powerpc/mm/slb_low.S
@@ -30,7 +30,7 @@
  *	r9, r10, r11 are clobbered by this function
  * No other registers are examined or changed.
  */
-_GLOBAL(slb_allocate_realmode)
+_GLOBAL_NOKPROBE(slb_allocate_realmode)
 	/*
 	 * check for bad kernel/user address
 	 * (ea & ~REGION_MASK) >= PGTABLE_RANGE
@@ -59,7 +59,7 @@ _GLOBAL(slb_allocate_realmode)
 	/* Linear mapping encoding bits, the "li" instruction below will
 	 * be patched by the kernel at boot
 	 */
-_GLOBAL_SYM(slb_miss_kernel_load_linear)
+_GLOBAL_SYM_NOKPROBE(slb_miss_kernel_load_linear)
 	li	r11,0
 	/*
 	 * context = (MAX_USER_CONTEXT) + ((ea >> 60) - 0xc) + 1
@@ -79,7 +79,7 @@ END_MMU_FTR_SECTION_IFCLR(MMU_FTR_1T_SEGMENT)
 	/* Check virtual memmap region. To be patches at kernel boot */
 	cmpldi	cr0,r9,0xf
 	bne	1f
-_GLOBAL_SYM(slb_miss_kernel_load_vmemmap)
+_GLOBAL_SYM_NOKPROBE(slb_miss_kernel_load_vmemmap)
 	li	r11,0
 	b	6f
 1:
@@ -95,7 +95,7 @@ _GLOBAL_SYM(slb_miss_kernel_load_vmemmap)
 	b	6f
 5:
 	/* IO mapping */
-_GLOBAL_SYM(slb_miss_kernel_load_io)
+_GLOBAL_SYM_NOKPROBE(slb_miss_kernel_load_io)
 	li	r11,0
 6:
 	/*
@@ -203,7 +203,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_1T_SEGMENT)
 7:	ld	r10,PACASTABRR(r13)
 	addi	r10,r10,1
 	/* This gets soft patched on boot. */
-_GLOBAL_SYM(slb_compare_rr_to_size)
+_GLOBAL_SYM_NOKPROBE(slb_compare_rr_to_size)
 	cmpldi	r10,0
 
 	blt+	4f
-- 
2.10.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist
  2016-11-17 15:08 ` [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist Naveen N. Rao
@ 2016-11-18  5:48   ` Michael Ellerman
  2016-11-18  7:04     ` Masami Hiramatsu
  2016-11-18 11:22     ` Naveen N. Rao
  0 siblings, 2 replies; 13+ messages in thread
From: Michael Ellerman @ 2016-11-18  5:48 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> Add symbol to mark end of entry_*.S and use the same to blacklist all
> addresses from kernel start (_stext) to entry code from kprobes. Much of
> this code is early exception handling where we can't really take a trap.

I'm not sure about this. entry_*.S is actually a bit of jumble,
especially the 64bit version. I've been wanting to split it up for a
long time.

It doesn't actually contain any early exception handling. It does
contain the common syscall handler, and the exception return paths, some
of which should be black listed. And lots of other junk.

Also I'm not sure if it's guaranteed that there won't be other code
between _stext and the end of entry, it's not handled explicitly in the
linker script, it just tends to get linked early because it's in head-y.

So I think it would be better if we had a clearer picture of exactly
what in this file we want to blacklist.

cheers

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist
  2016-11-18  5:48   ` Michael Ellerman
@ 2016-11-18  7:04     ` Masami Hiramatsu
  2016-11-18 11:24       ` Naveen N. Rao
  2016-11-18 11:22     ` Naveen N. Rao
  1 sibling, 1 reply; 13+ messages in thread
From: Masami Hiramatsu @ 2016-11-18  7:04 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Naveen N. Rao, Nicholas Piggin, Anton Blanchard, linuxppc-dev,
	Masami Hiramatsu, Ananth N Mavinakayanahalli

On Fri, 18 Nov 2016 16:48:01 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > Add symbol to mark end of entry_*.S and use the same to blacklist all
> > addresses from kernel start (_stext) to entry code from kprobes. Much of
> > this code is early exception handling where we can't really take a trap.
> 
> I'm not sure about this. entry_*.S is actually a bit of jumble,
> especially the 64bit version. I've been wanting to split it up for a
> long time.
> 
> It doesn't actually contain any early exception handling. It does
> contain the common syscall handler, and the exception return paths, some
> of which should be black listed. And lots of other junk.
> 
> Also I'm not sure if it's guaranteed that there won't be other code
> between _stext and the end of entry, it's not handled explicitly in the
> linker script, it just tends to get linked early because it's in head-y.
> 
> So I think it would be better if we had a clearer picture of exactly
> what in this file we want to blacklist.

Fair enough.

OK, the purpose of the kprobe blacklist is to avoid crashing kernel
by putting kprobes in some critical area (critical for kprobes,
not what usually "critical region" means).

Since kprobes is using breakpoint(trap) exception, if there is another
kprobes(trap) on the path until kprobe_handler() handle it, the kernel
kicks same exception handler and fall into the recursive fault.
So the blacklist is used in kprobe to prohibit putting kprobes on such
functions for avoiding it.

So, we might be carefully choose the function for the blacklist.

BTW, Naveen, as far as I can see the kprobe implementation on ppc,
it still depends on exceptions_notify to handle trap. It is no more
recommended becuase notifier_call_chain involves too many unrelated
functions. I recommend you to callback kprobe_handler and
kprobe_post_handler directly from the trap handler as same as x86.

Unless that, kprobe_blacklist may not work.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 4/4] powerpc: mm/slb: blacklist symbols from kprobe
  2016-11-17 15:08 ` [RFC PATCH 4/4] powerpc: mm/slb: blacklist symbols from kprobe Naveen N. Rao
@ 2016-11-18  9:36   ` Michael Ellerman
  2016-11-18 11:26     ` Naveen N. Rao
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2016-11-18  9:36 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> We can't really take a trap at this point. So, blacklist these symbols.
>
> Reported-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/slb_low.S | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/mm/slb_low.S b/arch/powerpc/mm/slb_low.S
> index c1c7456..c2bae92 100644
> --- a/arch/powerpc/mm/slb_low.S
> +++ b/arch/powerpc/mm/slb_low.S
> @@ -30,7 +30,7 @@
>   *	r9, r10, r11 are clobbered by this function
>   * No other registers are examined or changed.
>   */
> -_GLOBAL(slb_allocate_realmode)
> +_GLOBAL_NOKPROBE(slb_allocate_realmode)

I think I'd prefer we just leave these as is, and add somewhere nearby:

_ASM_NOKPROBE_SYMBOL(slb_allocate_realmode)


It means someone reading the code doesn't need to worry about the
nokprobe part, just to understand the function. It also means we need
fewer macros :)

cheers

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals
  2016-11-17 15:08 ` [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals Naveen N. Rao
@ 2016-11-18  9:41   ` Michael Ellerman
  2016-11-18 11:36     ` Naveen N. Rao
  0 siblings, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2016-11-18  9:41 UTC (permalink / raw)
  To: Naveen N. Rao
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

"Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:

> - Introduce _GLOBAL_SYM() for global symbols in assembly. This helps
> reduce verbosity of assembly files.

Unfortunately you've walked into a bit of mine field here :)

In user space they use FUNC_START() to declare the start of a function,
and we should do the same. Anton added FUNC_START/END, but didn't quite
get around to converting everything, see 151f25112ff7 ("powerpc: define
FUNC_START/FUNC_END").

So what I'd like is all uses of _GLOBAL() to become FUNC_START(), and
then we can change _GLOBAL() to just define a global symbol.

We can probably decouple that from most of this series though, as I
mentioned in my other reply, just by using _ASM_NOKPROBE_SYMBOL().

cheers

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist
  2016-11-18  5:48   ` Michael Ellerman
  2016-11-18  7:04     ` Masami Hiramatsu
@ 2016-11-18 11:22     ` Naveen N. Rao
  1 sibling, 0 replies; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-18 11:22 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Ananth N Mavinakayanahalli, linuxppc-dev, Anton Blanchard,
	Nicholas Piggin, Masami Hiramatsu

On 2016/11/18 04:48PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > Add symbol to mark end of entry_*.S and use the same to blacklist all
> > addresses from kernel start (_stext) to entry code from kprobes. Much of
> > this code is early exception handling where we can't really take a trap.
> 
> I'm not sure about this. entry_*.S is actually a bit of jumble,
> especially the 64bit version. I've been wanting to split it up for a
> long time.

Ok. Let me take a stab at that.

> 
> It doesn't actually contain any early exception handling. It does
> contain the common syscall handler, and the exception return paths, some
> of which should be black listed. And lots of other junk.
> 
> Also I'm not sure if it's guaranteed that there won't be other code
> between _stext and the end of entry, it's not handled explicitly in the
> linker script, it just tends to get linked early because it's in head-y.

I actually considered that. One of the issues in trying to get entry_* 
linked in early has to do with the exception common handlers - they 
start at 0x7000 or 0x8000 and are placed in .text *and* I think they 
need to be within 64k from the exception vectors. As such, placing 
entry_* in a separate section and linking it after HEAD_TEXT resulted in 
moving down the common exception handlers.

Regardless of the kprobe blacklist, does it make sense to put the common 
exception handlers into a separate section so as to separate them out 
from the rest of the code?

> 
> So I think it would be better if we had a clearer picture of exactly
> what in this file we want to blacklist.

Agreed. As a first step, I wanted to get a coarser blacklist in place 
and fine tune it later. But, I can see why entry_* needs a smaller 
blacklist. I'll get back on this.


- Naveen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist
  2016-11-18  7:04     ` Masami Hiramatsu
@ 2016-11-18 11:24       ` Naveen N. Rao
  0 siblings, 0 replies; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-18 11:24 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Michael Ellerman, Ananth N Mavinakayanahalli, Nicholas Piggin,
	Anton Blanchard, linuxppc-dev

Hi Masami,

On 2016/11/18 04:04PM, Masami Hiramatsu wrote:
> On Fri, 18 Nov 2016 16:48:01 +1100
> Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> > 
> > > Add symbol to mark end of entry_*.S and use the same to blacklist all
> > > addresses from kernel start (_stext) to entry code from kprobes. Much of
> > > this code is early exception handling where we can't really take a trap.
> > 
> > I'm not sure about this. entry_*.S is actually a bit of jumble,
> > especially the 64bit version. I've been wanting to split it up for a
> > long time.
> > 
> > It doesn't actually contain any early exception handling. It does
> > contain the common syscall handler, and the exception return paths, some
> > of which should be black listed. And lots of other junk.
> > 
> > Also I'm not sure if it's guaranteed that there won't be other code
> > between _stext and the end of entry, it's not handled explicitly in the
> > linker script, it just tends to get linked early because it's in head-y.
> > 
> > So I think it would be better if we had a clearer picture of exactly
> > what in this file we want to blacklist.
> 
> Fair enough.
> 
> OK, the purpose of the kprobe blacklist is to avoid crashing kernel
> by putting kprobes in some critical area (critical for kprobes,
> not what usually "critical region" means).
> 
> Since kprobes is using breakpoint(trap) exception, if there is another
> kprobes(trap) on the path until kprobe_handler() handle it, the kernel
> kicks same exception handler and fall into the recursive fault.
> So the blacklist is used in kprobe to prohibit putting kprobes on such
> functions for avoiding it.
> 
> So, we might be carefully choose the function for the blacklist.

Agreed, though in this case, I'm trying to blacklist early exception 
code to begin with :)

> 
> BTW, Naveen, as far as I can see the kprobe implementation on ppc,
> it still depends on exceptions_notify to handle trap. It is no more
> recommended becuase notifier_call_chain involves too many unrelated
> functions. I recommend you to callback kprobe_handler and
> kprobe_post_handler directly from the trap handler as same as x86.

Ah, thanks for the tip. Patch to follow.


- Naveen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 4/4] powerpc: mm/slb: blacklist symbols from kprobe
  2016-11-18  9:36   ` Michael Ellerman
@ 2016-11-18 11:26     ` Naveen N. Rao
  0 siblings, 0 replies; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-18 11:26 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

On 2016/11/18 08:36PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > We can't really take a trap at this point. So, blacklist these symbols.
> >
> > Reported-by: Anton Blanchard <anton@samba.org>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/mm/slb_low.S | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/mm/slb_low.S b/arch/powerpc/mm/slb_low.S
> > index c1c7456..c2bae92 100644
> > --- a/arch/powerpc/mm/slb_low.S
> > +++ b/arch/powerpc/mm/slb_low.S
> > @@ -30,7 +30,7 @@
> >   *	r9, r10, r11 are clobbered by this function
> >   * No other registers are examined or changed.
> >   */
> > -_GLOBAL(slb_allocate_realmode)
> > +_GLOBAL_NOKPROBE(slb_allocate_realmode)
> 
> I think I'd prefer we just leave these as is, and add somewhere nearby:
> 
> _ASM_NOKPROBE_SYMBOL(slb_allocate_realmode)
> 
> 
> It means someone reading the code doesn't need to worry about the
> nokprobe part, just to understand the function. It also means we need
> fewer macros :)

:)
Ok.

- Naveen

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals
  2016-11-18  9:41   ` Michael Ellerman
@ 2016-11-18 11:36     ` Naveen N. Rao
  0 siblings, 0 replies; 13+ messages in thread
From: Naveen N. Rao @ 2016-11-18 11:36 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nicholas Piggin, Anton Blanchard, linuxppc-dev, Masami Hiramatsu,
	Ananth N Mavinakayanahalli

On 2016/11/18 08:41PM, Michael Ellerman wrote:
> "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> 
> > - Introduce _GLOBAL_SYM() for global symbols in assembly. This helps
> > reduce verbosity of assembly files.
> 
> Unfortunately you've walked into a bit of mine field here :)
> 
> In user space they use FUNC_START() to declare the start of a function,
> and we should do the same. Anton added FUNC_START/END, but didn't quite
> get around to converting everything, see 151f25112ff7 ("powerpc: define
> FUNC_START/FUNC_END").
> 
> So what I'd like is all uses of _GLOBAL() to become FUNC_START(), and
> then we can change _GLOBAL() to just define a global symbol.

Can't say I didn't get tempted to rename _GLOBAL() to _GLOBAL_FUNC() :D
I'll convert the files I touch.

> 
> We can probably decouple that from most of this series though, as I
> mentioned in my other reply, just by using _ASM_NOKPROBE_SYMBOL().

Sure.

Thanks!
- Naveen

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-11-18 11:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-17 15:08 [RFC PATCH 0/4] Blacklist powerpc exception vectors from kprobes Naveen N. Rao
2016-11-17 15:08 ` [RFC PATCH 1/4] powerpc: asm: introduce new macros for assembly globals Naveen N. Rao
2016-11-18  9:41   ` Michael Ellerman
2016-11-18 11:36     ` Naveen N. Rao
2016-11-17 15:08 ` [RFC PATCH 2/4] powerpc: kprobe: add arch specific blacklist Naveen N. Rao
2016-11-18  5:48   ` Michael Ellerman
2016-11-18  7:04     ` Masami Hiramatsu
2016-11-18 11:24       ` Naveen N. Rao
2016-11-18 11:22     ` Naveen N. Rao
2016-11-17 15:08 ` [RFC PATCH 3/4] powerpc: mm/slb: convert slb_low.S to use the new macros Naveen N. Rao
2016-11-17 15:08 ` [RFC PATCH 4/4] powerpc: mm/slb: blacklist symbols from kprobe Naveen N. Rao
2016-11-18  9:36   ` Michael Ellerman
2016-11-18 11:26     ` Naveen N. Rao

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.