From: Naveen N. Rao <hidden> Date: 2016-11-17 15:09:10
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
From: Naveen N. Rao <hidden> Date: 2016-11-17 15:09:25
- 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 <redacted>
---
arch/powerpc/include/asm/ppc_asm.h | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
From: Naveen N. Rao <hidden> Date: 2016-11-17 15:09:30
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 <redacted>
Signed-off-by: Naveen N. Rao <redacted>
---
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(+)
@@ -36,12 +36,22 @@#include<asm/cacheflush.h>#include<asm/sstep.h>#include<asm/uaccess.h>+#include<asm/sections.h>DEFINE_PER_CPU(structkprobe*,current_kprobe)=NULL;DEFINE_PER_CPU(structkprobe_ctlblk,kprobe_ctlblk);structkretprobe_blackpointkretprobe_blacklist[]={{NULL,NULL}};+boolarch_within_kprobe_blacklist(unsignedlongaddr)+{+/* The __kprobes marked functions and entry code must not be probed */+return(addr>=(unsignedlong)__kprobes_text_start&&+addr<(unsignedlong)__kprobes_text_end)||+(addr>=(unsignedlong)_stext&&+addr<(unsignedlong)__entry_text_end);+}+int__kprobesarch_prepare_kprobe(structkprobe*p){intret=0;
From: Naveen N. Rao <hidden> Date: 2016-11-17 15:09:30
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 <redacted>
---
arch/powerpc/mm/slb_low.S | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
From: Naveen N. Rao <hidden> Date: 2016-11-17 15:09:32
We can't really take a trap at this point. So, blacklist these symbols.
Reported-by: Anton Blanchard <redacted>
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/mm/slb_low.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-11-18 05:48:04
"Naveen N. Rao" [off-list ref] 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
On Fri, 18 Nov 2016 16:48:01 +1100
Michael Ellerman [off-list ref] wrote:
"Naveen N. Rao" [off-list ref] writes:
quoted
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 [off-list ref]
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-11-18 09:36:23
"Naveen N. Rao" [off-list ref] writes:
quoted hunk
We can't really take a trap at this point. So, blacklist these symbols.
Reported-by: Anton Blanchard <redacted>
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/mm/slb_low.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
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
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2016-11-18 09:41:14
"Naveen N. Rao" [off-list ref] 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
From: Naveen N. Rao <hidden> Date: 2016-11-18 11:22:28
On 2016/11/18 04:48PM, Michael Ellerman wrote:
"Naveen N. Rao" [off-list ref] writes:
quoted
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
From: Naveen N. Rao <hidden> Date: 2016-11-18 11:24:54
Hi Masami,
On 2016/11/18 04:04PM, Masami Hiramatsu wrote:
On Fri, 18 Nov 2016 16:48:01 +1100
Michael Ellerman [off-list ref] wrote:
quoted
"Naveen N. Rao" [off-list ref] writes:
quoted
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.
From: Naveen N. Rao <hidden> Date: 2016-11-18 11:26:31
On 2016/11/18 08:36PM, Michael Ellerman wrote:
"Naveen N. Rao" [off-list ref] writes:
quoted
We can't really take a trap at this point. So, blacklist these symbols.
Reported-by: Anton Blanchard <redacted>
Signed-off-by: Naveen N. Rao <redacted>
---
arch/powerpc/mm/slb_low.S | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
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 :)
From: Naveen N. Rao <hidden> Date: 2016-11-18 11:37:04
On 2016/11/18 08:41PM, Michael Ellerman wrote:
"Naveen N. Rao" [off-list ref] writes:
quoted
- 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().