From: Josef Bacik <[email protected]>
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.
Acked-by: Alexei Starovoitov <[email protected]>
Signed-off-by: Josef Bacik <[email protected]>
---
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/linux/filter.h | 3 ++-
include/linux/trace_events.h | 1 +
include/uapi/linux/bpf.h | 7 ++++++-
kernel/bpf/verifier.c | 2 ++
kernel/events/core.c | 7 +++++++
kernel/trace/Kconfig | 11 +++++++++++
kernel/trace/bpf_trace.c | 35 +++++++++++++++++++++++++++++++++++
kernel/trace/trace_kprobe.c | 40 +++++++++++++++++++++++++++++++++-------
kernel/trace/trace_probe.h | 6 ++++++
14 files changed, 130 insertions(+), 9 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/linux/filter.h b/include/linux/filter.h
index cdd78a7beaae..dfa44fd74bae 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -458,7 +458,8 @@ struct bpf_prog {
locked:1, /* Program image locked? */
gpl_compatible:1, /* Is filter GPL compatible? */
cb_access:1, /* Is control block accessed? */
- dst_needed:1; /* Do we need dst entry? */
+ dst_needed:1, /* Do we need dst entry? */
+ kprobe_override:1; /* Do we override a kprobe? */
kmemcheck_bitfield_end(meta);
enum bpf_prog_type type; /* Type of BPF program */
u32 len; /* Number of filter blocks */
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index fc6aeca945db..be8bd5a8efaa 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -522,6 +522,7 @@ do { \
struct perf_event;
DECLARE_PER_CPU(struct pt_regs, perf_trace_regs);
+DECLARE_PER_CPU(int, bpf_kprobe_override);
extern int perf_trace_init(struct perf_event *event);
extern void perf_trace_destroy(struct perf_event *event);
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/bpf/verifier.c b/kernel/bpf/verifier.c
index d906775e12c1..f8f7927a9152 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
prog->dst_needed = 1;
if (insn->imm == BPF_FUNC_get_prandom_u32)
bpf_user_rnd_init_once();
+ if (insn->imm == BPF_FUNC_override_return)
+ prog->kprobe_override = 1;
if (insn->imm == BPF_FUNC_tail_call) {
/* If we tail call into other programs, we
* cannot make any assumptions since they can
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 9660ee65fbef..0d7fce52391d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
return -EINVAL;
}
+ /* Kprobe override only works for kprobes, not uprobes. */
+ if (prog->kprobe_override &&
+ !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {
+ bpf_prog_put(prog);
+ return -EINVAL;
+ }
+
if (is_tracepoint || is_syscall_tp) {
int off = trace_event_get_offsets(event->tp_event);
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..26be195a4e39 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -13,6 +13,10 @@
#include <linux/filter.h>
#include <linux/uaccess.h>
#include <linux/ctype.h>
+#include <linux/kprobes.h>
+#include <asm/kprobes.h>
+
+#include "trace_probe.h"
#include "trace.h"
u64 bpf_get_stackid(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
@@ -76,6 +80,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(bpf_kprobe_override, 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 +578,10 @@ 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:
+ pr_warn_ratelimited("%s[%d] is installing a program with bpf_override_return helper that may cause unexpected behavior!",
+ current->comm, task_pid_nr(current));
+ return &bpf_override_return_proto;
default:
return tracing_func_proto(func_id);
}
@@ -766,6 +797,10 @@ int perf_event_attach_bpf_prog(struct perf_event *event,
struct bpf_prog_array *new_array;
int ret = -EEXIST;
+ /* Kprobe override only works for ftrace based kprobes. */
+ if (prog->kprobe_override && !trace_kprobe_ftrace(event->tp_event))
+ return -EINVAL;
+
mutex_lock(&bpf_event_mutex);
if (event->prog)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index abf92e478cfb..8e3c9ec1faf7 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -42,6 +42,7 @@ struct trace_kprobe {
(offsetof(struct trace_kprobe, tp.args) + \
(sizeof(struct probe_arg) * (n)))
+DEFINE_PER_CPU(int, bpf_kprobe_override);
static nokprobe_inline bool trace_kprobe_is_return(struct trace_kprobe *tk)
{
@@ -87,6 +88,12 @@ static nokprobe_inline unsigned long trace_kprobe_nhit(struct trace_kprobe *tk)
return nhit;
}
+int trace_kprobe_ftrace(struct trace_event_call *call)
+{
+ struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
+ return kprobe_ftrace(&tk->rp.kp);
+}
+
static int register_kprobe_event(struct trace_kprobe *tk);
static int unregister_kprobe_event(struct trace_kprobe *tk);
@@ -1170,7 +1177,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 +1186,29 @@ 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;
+
+ ret = trace_call_bpf(call, regs);
+
+ /*
+ * We need to check and see if we modified the pc of the
+ * pt_regs, and if so clear the kprobe and return 1 so that we
+ * don't do the instruction skipping. Also reset our state so
+ * we are clean the next pass through.
+ */
+ if (__this_cpu_read(bpf_kprobe_override)) {
+ __this_cpu_write(bpf_kprobe_override, 0);
+ reset_current_kprobe();
+ return 1;
+ }
+ if (!ret)
+ return 0;
+ }
head = this_cpu_ptr(call->perf_events);
if (hlist_empty(head))
- return;
+ return 0;
dsize = __get_data_size(&tk->tp, regs);
__size = sizeof(*entry) + tk->tp.size + dsize;
@@ -1193,13 +1217,14 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
entry = perf_trace_buf_alloc(size, NULL, &rctx);
if (!entry)
- return;
+ return 0;
entry->ip = (unsigned long)tk->rp.kp.addr;
memset(&entry[1], 0, dsize);
store_trace_args(sizeof(*entry), &tk->tp, regs, (u8 *)&entry[1], dsize);
perf_trace_buf_submit(entry, size, rctx, call->event.type, 1, regs,
head, NULL, NULL);
+ return 0;
}
NOKPROBE_SYMBOL(kprobe_perf_func);
@@ -1275,6 +1300,7 @@ static int kprobe_register(struct trace_event_call *event,
static int kprobe_dispatcher(struct kprobe *kp, struct pt_regs *regs)
{
struct trace_kprobe *tk = container_of(kp, struct trace_kprobe, rp.kp);
+ int ret = 0;
raw_cpu_inc(*tk->nhit);
@@ -1282,9 +1308,9 @@ static int kprobe_dispatcher(struct kprobe *kp, struct pt_regs *regs)
kprobe_trace_func(tk, regs);
#ifdef CONFIG_PERF_EVENTS
if (tk->tp.flags & TP_FLAG_PROFILE)
- kprobe_perf_func(tk, regs);
+ ret = kprobe_perf_func(tk, regs);
#endif
- return 0; /* We don't tweek kernel, so just return 0 */
+ return ret;
}
NOKPROBE_SYMBOL(kprobe_dispatcher);
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 903273c93e61..adbb3f7d1fb5 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -253,6 +253,7 @@ struct symbol_cache;
unsigned long update_symbol_cache(struct symbol_cache *sc);
void free_symbol_cache(struct symbol_cache *sc);
struct symbol_cache *alloc_symbol_cache(const char *sym, long offset);
+int trace_kprobe_ftrace(struct trace_event_call *call);
#else
/* uprobes do not support symbol fetch methods */
#define fetch_symbol_u8 NULL
@@ -278,6 +279,11 @@ alloc_symbol_cache(const char *sym, long offset)
{
return NULL;
}
+
+static inline int trace_kprobe_ftrace(struct trace_event_call *call)
+{
+ return 0;
+}
#endif /* CONFIG_KPROBE_EVENTS */
struct probe_arg {
--
2.7.5
From 1583108637101186433@xxx Sat Nov 04 04:34:07 +0000 2017
X-GM-THRID: 1583108480388964563
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On 11/01/2017 06:00 PM, Josef Bacik wrote:
> From: Josef Bacik <[email protected]>
>
> 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 <[email protected]>
Looks useful (seccomp/BPF has something similar but on syscall level).
I think given we change kernel behavior, it would be good if we emit
similar warning like in case of bpf_get_probe_write_proto(), such that
from staring at the log (e.g. in case of subsequent crash), it could
help identifying that cause to trigger the bug could have been due to
bpf_override_function().
From 1582884987565115132@xxx Wed Nov 01 17:19:18 +0000 2017
X-GM-THRID: 1582884059549901177
X-Gmail-Labels: Inbox,Category Forums
On 11/1/17 10:00 AM, Josef Bacik wrote:
> From: Josef Bacik <[email protected]>
>
> 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 <[email protected]>
Both bpf and tracing bits look great to me.
Acked-by: Alexei Starovoitov <[email protected]>
From 1582884059549901177@xxx Wed Nov 01 17:04:33 +0000 2017
X-GM-THRID: 1582884059549901177
X-Gmail-Labels: Inbox,Category Forums
* Josef Bacik <[email protected]> wrote:
> > > Then 'not crashing kernel' requirement will be preserved.
> > > btrfs or whatever else we will be testing with override_return
> > > will be functioning in 'stress test' mode and if bpf program
> > > is not careful and returns error all the time then one particular
> > > subsystem (like btrfs) will not be functional, but the kernel
> > > will not be crashing.
> > > Thoughts?
> >
> > Yeah, that approach sounds much better to me: it should be fundamentally be
> > opt-in, and should be documented that it should not be possible to crash the
> > kernel via changing the return value.
> >
> > I'd make it a bit clearer in the naming what the purpose of the annotation is: for
> > example would BPF_ALLOW_ERROR_INJECTION() work for you guys? I.e. I think it
> > should generally be used to change actual integer error values - or at most user
> > pointers, but not kernel pointers. Not enforced in a type safe manner, but the
> > naming should give enough hints?
> >
> > Such return-injection BFR programs can still totally confuse user-space obviously:
> > for example returning an IO error could corrupt application data - but that's the
> > nature of such facilities and similar results could already be achieved via ptrace
> > as well. But the result of a BPF program should never be _worse_ than ptrace, in
> > terms of kernel integrity.
> >
> > Note that with such a safety mechanism in place no kernel message has to be
> > generated either I suspect.
> >
> > In any case, my NAK would be lifted with such an approach.
>
> I'm going to want to annotate kmalloc, so it's still going to be possible to
> make things go horribly wrong, is this still going to be ok with you? Obviously
> I want to use this for btrfs, but really what I used this for originally was an
> NBD problem where I had to do special handling for getting EINTR back from
> kernel_sendmsg, which was a pain to trigger properly without this patch. Opt-in
> is going to make it so we're just flagging important function calls anwyay
> because those are the ones that fail rarely and that we want to test, which puts
> us back in the same situation you are worried about, so it doesn't make much
> sense to me to do it this way. Thanks,
I suppose - let's see how it goes? The important factor is the opt-in aspect I
believe.
Technically the kernel should never crash on a kmalloc() failure either, although
obviously things can go horribly wrong from user-space's perspective.
Thanks,
Ingo
From 1583967099133399127@xxx Mon Nov 13 15:59:00 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Sun, Nov 12, 2017 at 11:38:24AM +0100, Ingo Molnar wrote:
>
> * Alexei Starovoitov <[email protected]> wrote:
>
> > > One of the major advantages of having an in-kernel BPF sandbox is to never
> > > crash the kernel - and allowing BPF programs to just randomly modify the
> > > return value of kernel functions sounds immensely broken to me.
> > >
> > > (And yes, I realize that kprobes are used here as a vehicle, but the point
> > > remains.)
> >
> > yeah. modifying arbitrary function return pushes bpf outside of
> > its safety guarantees and in that sense doing the same
> > override_return could be done from a kernel module if kernel
> > provides the x64 side of the facility introduced by this patch.
> > On the other side adding parts of this feature to the kernel only
> > to be used by external kernel module is quite ugly too and not
> > something that was ever done before.
> > How about we restrict this bpf_override_return() only to the functions
> > which callers expect to handle errors ?
> > We can add something similar to NOKPROBE_SYMBOL(). Like
> > ALLOW_RETURN_OVERRIDE() and on btrfs side mark the functions
> > we're going to test with this feature.
> >
> > Then 'not crashing kernel' requirement will be preserved.
> > btrfs or whatever else we will be testing with override_return
> > will be functioning in 'stress test' mode and if bpf program
> > is not careful and returns error all the time then one particular
> > subsystem (like btrfs) will not be functional, but the kernel
> > will not be crashing.
> > Thoughts?
>
> Yeah, that approach sounds much better to me: it should be fundamentally be
> opt-in, and should be documented that it should not be possible to crash the
> kernel via changing the return value.
>
> I'd make it a bit clearer in the naming what the purpose of the annotation is: for
> example would BPF_ALLOW_ERROR_INJECTION() work for you guys? I.e. I think it
> should generally be used to change actual integer error values - or at most user
> pointers, but not kernel pointers. Not enforced in a type safe manner, but the
> naming should give enough hints?
>
> Such return-injection BFR programs can still totally confuse user-space obviously:
> for example returning an IO error could corrupt application data - but that's the
> nature of such facilities and similar results could already be achieved via ptrace
> as well. But the result of a BPF program should never be _worse_ than ptrace, in
> terms of kernel integrity.
>
> Note that with such a safety mechanism in place no kernel message has to be
> generated either I suspect.
>
> In any case, my NAK would be lifted with such an approach.
>
I'm going to want to annotate kmalloc, so it's still going to be possible to
make things go horribly wrong, is this still going to be ok with you? Obviously
I want to use this for btrfs, but really what I used this for originally was an
NBD problem where I had to do special handling for getting EINTR back from
kernel_sendmsg, which was a pain to trigger properly without this patch. Opt-in
is going to make it so we're just flagging important function calls anwyay
because those are the ones that fail rarely and that we want to test, which puts
us back in the same situation you are worried about, so it doesn't make much
sense to me to do it this way. Thanks,
Josef
From 1583856423249027322@xxx Sun Nov 12 10:39:51 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
* Alexei Starovoitov <[email protected]> wrote:
> > One of the major advantages of having an in-kernel BPF sandbox is to never
> > crash the kernel - and allowing BPF programs to just randomly modify the
> > return value of kernel functions sounds immensely broken to me.
> >
> > (And yes, I realize that kprobes are used here as a vehicle, but the point
> > remains.)
>
> yeah. modifying arbitrary function return pushes bpf outside of
> its safety guarantees and in that sense doing the same
> override_return could be done from a kernel module if kernel
> provides the x64 side of the facility introduced by this patch.
> On the other side adding parts of this feature to the kernel only
> to be used by external kernel module is quite ugly too and not
> something that was ever done before.
> How about we restrict this bpf_override_return() only to the functions
> which callers expect to handle errors ?
> We can add something similar to NOKPROBE_SYMBOL(). Like
> ALLOW_RETURN_OVERRIDE() and on btrfs side mark the functions
> we're going to test with this feature.
>
> Then 'not crashing kernel' requirement will be preserved.
> btrfs or whatever else we will be testing with override_return
> will be functioning in 'stress test' mode and if bpf program
> is not careful and returns error all the time then one particular
> subsystem (like btrfs) will not be functional, but the kernel
> will not be crashing.
> Thoughts?
Yeah, that approach sounds much better to me: it should be fundamentally be
opt-in, and should be documented that it should not be possible to crash the
kernel via changing the return value.
I'd make it a bit clearer in the naming what the purpose of the annotation is: for
example would BPF_ALLOW_ERROR_INJECTION() work for you guys? I.e. I think it
should generally be used to change actual integer error values - or at most user
pointers, but not kernel pointers. Not enforced in a type safe manner, but the
naming should give enough hints?
Such return-injection BFR programs can still totally confuse user-space obviously:
for example returning an IO error could corrupt application data - but that's the
nature of such facilities and similar results could already be achieved via ptrace
as well. But the result of a BPF program should never be _worse_ than ptrace, in
terms of kernel integrity.
Note that with such a safety mechanism in place no kernel message has to be
generated either I suspect.
In any case, my NAK would be lifted with such an approach.
Thanks,
Ingo
From 1583842032885975307@xxx Sun Nov 12 06:51:08 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On 11/11/17 4:14 PM, Ingo Molnar wrote:
>
> * Josef Bacik <[email protected]> wrote:
>
>> On Fri, Nov 10, 2017 at 10:34:59AM +0100, Ingo Molnar wrote:
>>>
>>> * Josef Bacik <[email protected]> wrote:
>>>
>>>> @@ -551,6 +578,10 @@ 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:
>>>> + pr_warn_ratelimited("%s[%d] is installing a program with bpf_override_return helper that may cause unexpected behavior!",
>>>> + current->comm, task_pid_nr(current));
>>>> + return &bpf_override_return_proto;
>>>
>>> So if this new functionality is used we'll always print this into the syslog?
>>>
>>> The warning is also a bit passive aggressive about informing the user: what
>>> unexpected behavior can happen, what is the worst case?
>>>
>>
>> It's modeled after the other warnings bpf will spit out, but with this feature
>> you are skipping a function and instead returning some arbitrary value, so
>> anything could go wrong if you mess something up. For instance I screwed up my
>> initial test case and made every IO submitted return an error instead of just on
>> the one file system I was attempting to test, so all sorts of hilarity ensued.
>
> Ok, then for the x86 bits:
>
> NAK-ed-by: Ingo Molnar <[email protected]>
>
> One of the major advantages of having an in-kernel BPF sandbox is to never crash
> the kernel - and allowing BPF programs to just randomly modify the return value of
> kernel functions sounds immensely broken to me.
>
> (And yes, I realize that kprobes are used here as a vehicle, but the point
> remains.)
yeah. modifying arbitrary function return pushes bpf outside of
its safety guarantees and in that sense doing the same
override_return could be done from a kernel module if kernel
provides the x64 side of the facility introduced by this patch.
On the other side adding parts of this feature to the kernel only
to be used by external kernel module is quite ugly too and not
something that was ever done before.
How about we restrict this bpf_override_return() only to the functions
which callers expect to handle errors ?
We can add something similar to NOKPROBE_SYMBOL(). Like
ALLOW_RETURN_OVERRIDE() and on btrfs side mark the functions
we're going to test with this feature.
Then 'not crashing kernel' requirement will be preserved.
btrfs or whatever else we will be testing with override_return
will be functioning in 'stress test' mode and if bpf program
is not careful and returns error all the time then one particular
subsystem (like btrfs) will not be functional, but the kernel
will not be crashing.
Thoughts?
From 1583770416719769419@xxx Sat Nov 11 11:52:49 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Sat, Nov 11, 2017 at 09:14:55AM +0100, Ingo Molnar wrote:
>
> * Josef Bacik <[email protected]> wrote:
>
> > On Fri, Nov 10, 2017 at 10:34:59AM +0100, Ingo Molnar wrote:
> > >
> > > * Josef Bacik <[email protected]> wrote:
> > >
> > > > @@ -551,6 +578,10 @@ 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:
> > > > + pr_warn_ratelimited("%s[%d] is installing a program with bpf_override_return helper that may cause unexpected behavior!",
> > > > + current->comm, task_pid_nr(current));
> > > > + return &bpf_override_return_proto;
> > >
> > > So if this new functionality is used we'll always print this into the syslog?
> > >
> > > The warning is also a bit passive aggressive about informing the user: what
> > > unexpected behavior can happen, what is the worst case?
> > >
> >
> > It's modeled after the other warnings bpf will spit out, but with this feature
> > you are skipping a function and instead returning some arbitrary value, so
> > anything could go wrong if you mess something up. For instance I screwed up my
> > initial test case and made every IO submitted return an error instead of just on
> > the one file system I was attempting to test, so all sorts of hilarity ensued.
>
> Ok, then for the x86 bits:
>
> NAK-ed-by: Ingo Molnar <[email protected]>
>
> One of the major advantages of having an in-kernel BPF sandbox is to never crash
> the kernel - and allowing BPF programs to just randomly modify the return value of
> kernel functions sounds immensely broken to me.
>
> (And yes, I realize that kprobes are used here as a vehicle, but the point
> remains.)
>
Only root can use this feature, and did you read the first email? The whole
point of this is that error path checkig fucking sucks, and this gives us the
ability to systematically check our error paths and make the kernel way more
robust than it currently is. Can things go wrong? Sure, that's why its a
config option and root only. You only want to turn this on for testing and not
have it on in production. This is a valuable tool and well worth the risk.
Thanks,
Josef
From 1583756781786131344@xxx Sat Nov 11 08:16:06 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
* Josef Bacik <[email protected]> wrote:
> On Fri, Nov 10, 2017 at 10:34:59AM +0100, Ingo Molnar wrote:
> >
> > * Josef Bacik <[email protected]> wrote:
> >
> > > @@ -551,6 +578,10 @@ 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:
> > > + pr_warn_ratelimited("%s[%d] is installing a program with bpf_override_return helper that may cause unexpected behavior!",
> > > + current->comm, task_pid_nr(current));
> > > + return &bpf_override_return_proto;
> >
> > So if this new functionality is used we'll always print this into the syslog?
> >
> > The warning is also a bit passive aggressive about informing the user: what
> > unexpected behavior can happen, what is the worst case?
> >
>
> It's modeled after the other warnings bpf will spit out, but with this feature
> you are skipping a function and instead returning some arbitrary value, so
> anything could go wrong if you mess something up. For instance I screwed up my
> initial test case and made every IO submitted return an error instead of just on
> the one file system I was attempting to test, so all sorts of hilarity ensued.
Ok, then for the x86 bits:
NAK-ed-by: Ingo Molnar <[email protected]>
One of the major advantages of having an in-kernel BPF sandbox is to never crash
the kernel - and allowing BPF programs to just randomly modify the return value of
kernel functions sounds immensely broken to me.
(And yes, I realize that kprobes are used here as a vehicle, but the point
remains.)
Thanks,
Ingo
From 1583700118443169300@xxx Fri Nov 10 17:15:27 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On Fri, Nov 10, 2017 at 10:34:59AM +0100, Ingo Molnar wrote:
>
> * Josef Bacik <[email protected]> wrote:
>
> > @@ -551,6 +578,10 @@ 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:
> > + pr_warn_ratelimited("%s[%d] is installing a program with bpf_override_return helper that may cause unexpected behavior!",
> > + current->comm, task_pid_nr(current));
> > + return &bpf_override_return_proto;
>
> So if this new functionality is used we'll always print this into the syslog?
>
> The warning is also a bit passive aggressive about informing the user: what
> unexpected behavior can happen, what is the worst case?
>
It's modeled after the other warnings bpf will spit out, but with this feature
you are skipping a function and instead returning some arbitrary value, so
anything could go wrong if you mess something up. For instance I screwed up my
initial test case and made every IO submitted return an error instead of just on
the one file system I was attempting to test, so all sorts of hilarity ensued.
Thanks,
Josef
From 1583671210869608232@xxx Fri Nov 10 09:35:59 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
* Josef Bacik <[email protected]> wrote:
> @@ -551,6 +578,10 @@ 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:
> + pr_warn_ratelimited("%s[%d] is installing a program with bpf_override_return helper that may cause unexpected behavior!",
> + current->comm, task_pid_nr(current));
> + return &bpf_override_return_proto;
So if this new functionality is used we'll always print this into the syslog?
The warning is also a bit passive aggressive about informing the user: what
unexpected behavior can happen, what is the worst case?
Thanks,
Ingo
From 1583469275288674532@xxx Wed Nov 08 04:06:18 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread
On 11/07/2017 09:28 PM, Josef Bacik wrote:
> From: Josef Bacik <[email protected]>
>
> 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.
>
> Acked-by: Alexei Starovoitov <[email protected]>
> Signed-off-by: Josef Bacik <[email protected]>
BPF bits:
Acked-by: Daniel Borkmann <[email protected]>
From 1583463755336960738@xxx Wed Nov 08 02:38:34 +0000 2017
X-GM-THRID: 1583463755336960738
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread