| Message ID | e34af1637b3c190cc0f2e41131f2524e75720ed3.1479394571.git.naveen.n.rao@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
"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
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,
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
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
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;
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(+)