lkml.org 
[lkml]   [2017]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] bpf: add a bpf_override_function helper
From
Date
On 10/30/17 2:19 PM, Josef Bacik 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.
>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
> arch/Kconfig | 3 +++
> arch/x86/Kconfig | 1 +
> arch/x86/include/asm/kprobes.h | 4 ++++
> arch/x86/include/asm/ptrace.h | 5 +++++
> arch/x86/kernel/kprobes/ftrace.c | 14 ++++++++++++
> include/uapi/linux/bpf.h | 7 +++++-
> kernel/trace/Kconfig | 11 ++++++++++
> kernel/trace/bpf_trace.c | 47 +++++++++++++++++++++++++++++++++++-----
> kernel/trace/trace.h | 6 +++++
> kernel/trace/trace_kprobe.c | 23 ++++++++++++++------
> 10 files changed, 108 insertions(+), 13 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index d789a89cb32c..4fb618082259 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -195,6 +195,9 @@ config HAVE_OPTPROBES
> config HAVE_KPROBES_ON_FTRACE
> bool
>
> +config HAVE_KPROBE_OVERRIDE
> + bool
> +
> config HAVE_NMI
> bool
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 971feac13506..5126d2750dd0 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -152,6 +152,7 @@ config X86
> select HAVE_KERNEL_XZ
> select HAVE_KPROBES
> select HAVE_KPROBES_ON_FTRACE
> + select HAVE_KPROBE_OVERRIDE
> select HAVE_KRETPROBES
> select HAVE_KVM
> select HAVE_LIVEPATCH if X86_64
> diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
> index 6cf65437b5e5..c6c3b1f4306a 100644
> --- a/arch/x86/include/asm/kprobes.h
> +++ b/arch/x86/include/asm/kprobes.h
> @@ -67,6 +67,10 @@ extern const int kretprobe_blacklist_size;
> void arch_remove_kprobe(struct kprobe *p);
> asmlinkage void kretprobe_trampoline(void);
>
> +#ifdef CONFIG_KPROBES_ON_FTRACE
> +extern void arch_ftrace_kprobe_override_function(struct pt_regs *regs);
> +#endif
> +
> /* Architecture specific copy of original instruction*/
> struct arch_specific_insn {
> /* copy of the original instruction */
> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> index 91c04c8e67fa..f04e71800c2f 100644
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -108,6 +108,11 @@ static inline unsigned long regs_return_value(struct pt_regs *regs)
> return regs->ax;
> }
>
> +static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
> +{
> + regs->ax = rc;
> +}
> +
> /*
> * user_mode(regs) determines whether a register set came from user
> * mode. On x86_32, this is true if V8086 mode was enabled OR if the
> diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
> index 041f7b6dfa0f..3c455bf490cb 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);
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0b7b54d898bd..1ad5b87a42f6 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -673,6 +673,10 @@ union bpf_attr {
> * @buf: buf to fill
> * @buf_size: size of the buf
> * Return : 0 on success or negative error code
> + *
> + * int bpf_override_return(pt_regs, rc)
> + * @pt_regs: pointer to struct pt_regs
> + * @rc: the return value to set
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -732,7 +736,8 @@ union bpf_attr {
> FN(xdp_adjust_meta), \
> FN(perf_event_read_value), \
> FN(perf_prog_read_value), \
> - FN(getsockopt),
> + FN(getsockopt), \
> + FN(override_return),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 434c840e2d82..9dc0deeaad2b 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -518,6 +518,17 @@ config FUNCTION_PROFILER
>
> If in doubt, say N.
>
> +config BPF_KPROBE_OVERRIDE
> + bool "Enable BPF programs to override a kprobed function"
> + depends on BPF_EVENTS
> + depends on KPROBES_ON_FTRACE
> + depends on HAVE_KPROBE_OVERRIDE
> + depends on DYNAMIC_FTRACE_WITH_REGS
> + default n
> + help
> + Allows BPF to override the execution of a probed function and
> + set a different return value. This is used for error injection.
> +
> config FTRACE_MCOUNT_RECORD
> def_bool y
> depends on DYNAMIC_FTRACE
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 136aa6bb0422..38b6d6016b71 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -13,10 +13,14 @@
> #include <linux/filter.h>
> #include <linux/uaccess.h>
> #include <linux/ctype.h>
> +#include <asm/kprobes.h>
> +
> #include "trace.h"
>
> u64 bpf_get_stackid(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
>
> +static DEFINE_PER_CPU(int, pc_modified);
> +
> /**
> * trace_call_bpf - invoke BPF program
> * @call: tracepoint event
> @@ -27,16 +31,18 @@ u64 bpf_get_stackid(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
> *
> * Return: BPF programs always return an integer which is interpreted by
> * kprobe handler as:
> - * 0 - return from kprobe (event is filtered out)
> - * 1 - store kprobe event into ring buffer
> - * Other values are reserved and currently alias to 1
> + * TRACE_KPROBE_SKIP - return from kprobe (event is filtered out)
> + * TRACE_KPOBE_STORE - store kprobe event into ring buffer
> + * TRACE_KPROBE_MODIFIED - we modified the registers, make sure the dispatcher
> + * skips the event and returns so the kprobe infrastructure
> + * doesn't mess with the next instruction.
> */
> unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
> {
> unsigned int ret;
>
> if (in_nmi()) /* not supported yet */
> - return 1;
> + return TRACE_KPROBE_STORE;
>
> preempt_disable();
>
> @@ -47,7 +53,7 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
> * and don't send kprobe event into ring-buffer,
> * so return zero here
> */
> - ret = 0;
> + ret = TRACE_KPROBE_SKIP;
> goto out;
> }
>
> @@ -67,7 +73,13 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
> * rcu_dereference() which is accepted risk.
> */
> ret = BPF_PROG_RUN_ARRAY_CHECK(call->prog_array, ctx, BPF_PROG_RUN);
> + if (ret)
> + ret = TRACE_KPROBE_STORE;
>
> + if (__this_cpu_read(pc_modified)) {
> + __this_cpu_write(pc_modified, 0);
> + ret = TRACE_KPROBE_MODIFIED;

we probably need to fork trace_call_bpf() specifically for kprobes,
since this new functionality is not applicable to tracepoints and
uprobes. Like perf_event type bpf prog is using bpf_overflow_handler()

> + }
> out:
> __this_cpu_dec(bpf_prog_active);
> preempt_enable();
> @@ -76,6 +88,29 @@ unsigned int trace_call_bpf(struct trace_event_call *call, void *ctx)
> }
> EXPORT_SYMBOL_GPL(trace_call_bpf);
>
> +#ifdef CONFIG_BPF_KPROBE_OVERRIDE
> +BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
> +{
> + __this_cpu_write(pc_modified, 1);
> + regs_set_return_value(regs, rc);
> + arch_ftrace_kprobe_override_function(regs);
> + return 0;
> +}
> +#else
> +BPF_CALL_2(bpf_override_return, struct pt_regs *, regs, unsigned long, rc)
> +{
> + return -EINVAL;
> +}
> +#endif
> +
> +static const struct bpf_func_proto bpf_override_return_proto = {
> + .func = bpf_override_return,
> + .gpl_only = true,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_ANYTHING,
> +};
> +
> BPF_CALL_3(bpf_probe_read, void *, dst, u32, size, const void *, unsafe_ptr)
> {
> int ret;
> @@ -551,6 +586,8 @@ static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func
> return &bpf_get_stackid_proto;
> case BPF_FUNC_perf_event_read_value:
> return &bpf_perf_event_read_value_proto;
> + case BPF_FUNC_override_return:
> + return &bpf_override_return_proto;

good call to allow it on kprobes only.
It probably needs to be tighten further to allow it
in ftrace-based kprobes only.
imo 'depends on KPROBES_ON_FTRACE' isn't not enough,
since kprobe in the middle of the function will still work via trap
and won't work with this override_func().

> default:
> return tracing_func_proto(func_id);
> }
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 652c682707cd..317ff2e961ac 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -278,6 +278,12 @@ enum {
> TRACE_ARRAY_FL_GLOBAL = (1 << 0)
> };
>
> +enum {
> + TRACE_KPROBE_SKIP = 0,
> + TRACE_KPROBE_STORE,
> + TRACE_KPROBE_MODIFIED,
> +};
> +
> extern struct list_head ftrace_trace_arrays;
>
> extern struct mutex trace_types_lock;
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index abf92e478cfb..722fc6568134 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1170,7 +1170,7 @@ static int kretprobe_event_define_fields(struct trace_event_call *event_call)
> #ifdef CONFIG_PERF_EVENTS
>
> /* Kprobe profile handler */
> -static void
> +static int
> kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
> {
> struct trace_event_call *call = &tk->tp.call;
> @@ -1179,12 +1179,19 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
> int size, __size, dsize;
> int rctx;
>
> - if (bpf_prog_array_valid(call) && !trace_call_bpf(call, regs))
> - return;
> + if (bpf_prog_array_valid(call)) {
> + int ret = trace_call_bpf(call, regs);

actually, can we keep trace_call_bpf() as-is and move
if (__this_cpu_read(pc_modified))
logic into here ?
I think kprobe_perf_func() runs with preempt_disabled.
May be specialized trace_call_kprobe_bpf() would be better
still to avoid double preempt_disable.

\
 
 \ /
  Last update: 2017-10-31 02:36    [W:1.476 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site