Messages in this thread | | | Date | Tue, 19 Dec 2017 15:38:08 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v10 3/5] bpf: add a bpf_override_function helper |
| |
On Mon, 18 Dec 2017 16:09:30 +0100 Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 12/18/2017 10:51 AM, Masami Hiramatsu wrote: > > On Fri, 15 Dec 2017 14:12:54 -0500 > > Josef Bacik <josef@toxicpanda.com> wrote: > >> From: Josef Bacik <jbacik@fb.com> > >> > >> Error injection is sloppy and very ad-hoc. BPF could fill this niche > >> perfectly with it's kprobe functionality. We could make sure errors are > >> only triggered in specific call chains that we care about with very > >> specific situations. Accomplish this with the bpf_override_funciton > >> helper. This will modify the probe'd callers return value to the > >> specified value and set the PC to an override function that simply > >> returns, bypassing the originally probed function. This gives us a nice > >> clean way to implement systematic error injection for all of our code > >> paths. > > > > OK, got it. I think the error_injectable function list should be defined > > in kernel/trace/bpf_trace.c because only bpf calls it and needs to care > > the "safeness". > > > > [...] > >> diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c > >> index 8dc0161cec8f..1ea748d682fd 100644 > >> --- a/arch/x86/kernel/kprobes/ftrace.c > >> +++ b/arch/x86/kernel/kprobes/ftrace.c > >> @@ -97,3 +97,17 @@ int arch_prepare_kprobe_ftrace(struct kprobe *p) > >> p->ainsn.boostable = false; > >> return 0; > >> } > >> + > >> +asmlinkage void override_func(void); > >> +asm( > >> + ".type override_func, @function\n" > >> + "override_func:\n" > >> + " ret\n" > >> + ".size override_func, .-override_func\n" > >> +); > >> + > >> +void arch_ftrace_kprobe_override_function(struct pt_regs *regs) > >> +{ > >> + regs->ip = (unsigned long)&override_func; > >> +} > >> +NOKPROBE_SYMBOL(arch_ftrace_kprobe_override_function); > > > > Calling this as "override_function" is meaningless. This is a function > > which just return. So I think combination of just_return_func() and > > arch_bpf_override_func_just_return() will be better. > > > > Moreover, this arch/x86/kernel/kprobes/ftrace.c is an archtecture > > dependent implementation of kprobes, not bpf. > > Josef, please work out any necessary cleanups that would still need > to be addressed based on Masami's feedback and send them as follow-up > patches, thanks. > > > Hmm, arch/x86/net/bpf_jit_comp.c will be better place? > > (No, it's JIT only and I'd really prefer to keep it that way, mixing > this would result in a huge mess.)
OK, that is same to kprobes. kernel/kprobes.c and arch/x86/kernel/kprobe/* are for instrumentation code. And kernel/trace/trace_kprobe.c is ftrace's kprobe user interface, just one implementation of kprobe usage. So please do not mix it up. It will result in a huge mess to me.
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |