[PATCH v9 0/5] Add the ability to do BPF directed error injection

[PATCH v9 0/5] Add the ability to do BPF directed error injection

[Josef Bacik]

This is the same as v8, just rebased onto the bpf tree.

v8->v9:
- rebased onto the bpf tree.

v7->v8:
- removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.

v6->v7:
- moved the opt-in macro to bpf.h out of kprobes.h.

v5->v6:
- add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this
  feature.  This way only functions that opt-in will be allowed to be
  overridden.
- added a btrfs patch to allow error injection for open_ctree() so that the bpf
  sample actually works.

v4->v5:
- disallow kprobe_override programs from being put in the prog map array so we
  don't tail call into something we didn't check.  This allows us to make the
  normal path still fast without a bunch of percpu operations.

v3->v4:
- fix a build error found by kbuild test bot (I didn't wait long enough
  apparently.)
- Added a warning message as per Daniels suggestion.

v2->v3:
- added a ->kprobe_override flag to bpf_prog.
- added some sanity checks to disallow attaching bpf progs that have
  ->kprobe_override set that aren't for ftrace kprobes.
- added the trace_kprobe_ftrace helper to check if the trace_event_call is a
  ftrace kprobe.
- renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this
  value in the kprobe path, and thus only write to it if we're overriding or
  clearing the override.

v1->v2:
- moved things around to make sure that bpf_override_return could really only be
  used for an ftrace kprobe.
- killed the special return values from trace_call_bpf.
- renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if
  it was being called from an ftrace kprobe context.
- reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state.
- updated the test as per Alexei's review.

- Original message -

A lot of our error paths are not well tested because we have no good way of
injecting errors generically.  Some subystems (block, memory) have ways to
inject errors, but they are random so it's hard to get reproduceable results.

With BPF we can add determinism to our error injection.  We can use kprobes and
other things to verify we are injecting errors at the exact case we are trying
to test.  This patch gives us the tool to actual do the error injection part.
It is very simple, we just set the return value of the pt_regs we're given to
whatever we provide, and then override the PC with a dummy function that simply
returns.

Right now this only works on x86, but it would be simple enough to expand to
other architectures.  Thanks,

Josef

[PATCH v9 1/5] add infrastructure for tagging functions as error injectable

[Josef Bacik]

From: Josef Bacik <jbacik@fb.com>

Using BPF we can override kprob'ed functions and return arbitrary
values.  Obviously this can be a bit unsafe, so make this feature opt-in
for functions.  Simply tag a function with KPROBE_ERROR_INJECT_SYMBOL in
order to give BPF access to that function for error injection purposes.

Signed-off-by: Josef Bacik <jbacik@fb.com>
Acked-by: Ingo Molnar <mingo@kernel.org>
---
 include/asm-generic/vmlinux.lds.h |  10 +++
 include/linux/bpf.h               |  11 +++
 include/linux/kprobes.h           |   1 +
 include/linux/module.h            |   5 ++
 kernel/kprobes.c                  | 163 ++++++++++++++++++++++++++++++++++++++
 kernel/module.c                   |   6 +-
 6 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index ee8b707d9fa9..a2e8582d094a 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -136,6 +136,15 @@
 #define KPROBE_BLACKLIST()
 #endif
 
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+#define ERROR_INJECT_LIST()	. = ALIGN(8);						\
+				VMLINUX_SYMBOL(__start_kprobe_error_inject_list) = .;	\
+				KEEP(*(_kprobe_error_inject_list))			\
+				VMLINUX_SYMBOL(__stop_kprobe_error_inject_list) = .;
+#else
+#define ERROR_INJECT_LIST()
+#endif
+
 #ifdef CONFIG_EVENT_TRACING
 #define FTRACE_EVENTS()	. = ALIGN(8);					\
 			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
@@ -564,6 +573,7 @@
 	FTRACE_EVENTS()							\
 	TRACE_SYSCALLS()						\
 	KPROBE_BLACKLIST()						\
+	ERROR_INJECT_LIST()						\
 	MEM_DISCARD(init.rodata)					\
 	CLK_OF_TABLES()							\
 	RESERVEDMEM_OF_TABLES()						\
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e55e4255a210..7f4d2a953173 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -576,4 +576,15 @@ extern const struct bpf_func_proto bpf_sock_map_update_proto;
 void bpf_user_rnd_init_once(void);
 u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
 
+#if defined(__KERNEL__) && !defined(__ASSEMBLY__)
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+#define BPF_ALLOW_ERROR_INJECTION(fname)				\
+static unsigned long __used						\
+	__attribute__((__section__("_kprobe_error_inject_list")))	\
+	_eil_addr_##fname = (unsigned long)fname;
+#else
+#define BPF_ALLOW_ERROR_INJECTION(fname)
+#endif
+#endif
+
 #endif /* _LINUX_BPF_H */
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 9440a2fc8893..963fd364f3d6 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -271,6 +271,7 @@ extern bool arch_kprobe_on_func_entry(unsigned long offset);
 extern bool kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset);
 
 extern bool within_kprobe_blacklist(unsigned long addr);
+extern bool within_kprobe_error_injection_list(unsigned long addr);
 
 struct kprobe_insn_cache {
 	struct mutex mutex;
diff --git a/include/linux/module.h b/include/linux/module.h
index c69b49abe877..548fa09fa806 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -475,6 +475,11 @@ struct module {
 	ctor_fn_t *ctors;
 	unsigned int num_ctors;
 #endif
+
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+	unsigned int num_kprobe_ei_funcs;
+	unsigned long *kprobe_ei_funcs;
+#endif
 } ____cacheline_aligned __randomize_layout;
 #ifndef MODULE_ARCH_INIT
 #define MODULE_ARCH_INIT {}
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index da2ccf142358..b4aab48ad258 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -83,6 +83,16 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
 	return &(kretprobe_table_locks[hash].lock);
 }
 
+/* List of symbols that can be overriden for error injection. */
+static LIST_HEAD(kprobe_error_injection_list);
+static DEFINE_MUTEX(kprobe_ei_mutex);
+struct kprobe_ei_entry {
+	struct list_head list;
+	unsigned long start_addr;
+	unsigned long end_addr;
+	void *priv;
+};
+
 /* Blacklist -- list of struct kprobe_blacklist_entry */
 static LIST_HEAD(kprobe_blacklist);
 
@@ -1394,6 +1404,17 @@ bool within_kprobe_blacklist(unsigned long addr)
 	return false;
 }
 
+bool within_kprobe_error_injection_list(unsigned long addr)
+{
+	struct kprobe_ei_entry *ent;
+
+	list_for_each_entry(ent, &kprobe_error_injection_list, list) {
+		if (addr >= ent->start_addr && addr < ent->end_addr)
+			return true;
+	}
+	return false;
+}
+
 /*
  * If we have a symbol_name argument, look it up and add the offset field
  * to it. This way, we can specify a relative address to a symbol.
@@ -2168,6 +2189,86 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
 	return 0;
 }
 
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+/* Markers of the _kprobe_error_inject_list section */
+extern unsigned long __start_kprobe_error_inject_list[];
+extern unsigned long __stop_kprobe_error_inject_list[];
+
+/*
+ * Lookup and populate the kprobe_error_injection_list.
+ *
+ * For safety reasons we only allow certain functions to be overriden with
+ * bpf_error_injection, so we need to populate the list of the symbols that have
+ * been marked as safe for overriding.
+ */
+static void populate_kprobe_error_injection_list(unsigned long *start,
+						 unsigned long *end,
+						 void *priv)
+{
+	unsigned long *iter;
+	struct kprobe_ei_entry *ent;
+	unsigned long entry, offset = 0, size = 0;
+
+	mutex_lock(&kprobe_ei_mutex);
+	for (iter = start; iter < end; iter++) {
+		entry = arch_deref_entry_point((void *)*iter);
+
+		if (!kernel_text_address(entry) ||
+		    !kallsyms_lookup_size_offset(entry, &size, &offset)) {
+			pr_err("Failed to find error inject entry at %p\n",
+				(void *)entry);
+			continue;
+		}
+
+		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
+		if (!ent)
+			break;
+		ent->start_addr = entry;
+		ent->end_addr = entry + size;
+		ent->priv = priv;
+		INIT_LIST_HEAD(&ent->list);
+		list_add_tail(&ent->list, &kprobe_error_injection_list);
+	}
+	mutex_unlock(&kprobe_ei_mutex);
+}
+
+static void __init populate_kernel_kprobe_ei_list(void)
+{
+	populate_kprobe_error_injection_list(__start_kprobe_error_inject_list,
+					     __stop_kprobe_error_inject_list,
+					     NULL);
+}
+
+static void module_load_kprobe_ei_list(struct module *mod)
+{
+	if (!mod->num_kprobe_ei_funcs)
+		return;
+	populate_kprobe_error_injection_list(mod->kprobe_ei_funcs,
+					     mod->kprobe_ei_funcs +
+					     mod->num_kprobe_ei_funcs, mod);
+}
+
+static void module_unload_kprobe_ei_list(struct module *mod)
+{
+	struct kprobe_ei_entry *ent, *n;
+	if (!mod->num_kprobe_ei_funcs)
+		return;
+
+	mutex_lock(&kprobe_ei_mutex);
+	list_for_each_entry_safe(ent, n, &kprobe_error_injection_list, list) {
+		if (ent->priv == mod) {
+			list_del_init(&ent->list);
+			kfree(ent);
+		}
+	}
+	mutex_unlock(&kprobe_ei_mutex);
+}
+#else
+static inline void __init populate_kernel_kprobe_ei_list(void) {}
+static inline void module_load_kprobe_ei_list(struct module *m) {}
+static inline void module_unload_kprobe_ei_list(struct module *m) {}
+#endif
+
 /* Module notifier call back, checking kprobes on the module */
 static int kprobes_module_callback(struct notifier_block *nb,
 				   unsigned long val, void *data)
@@ -2178,6 +2279,11 @@ static int kprobes_module_callback(struct notifier_block *nb,
 	unsigned int i;
 	int checkcore = (val == MODULE_STATE_GOING);
 
+	if (val == MODULE_STATE_COMING)
+		module_load_kprobe_ei_list(mod);
+	else if (val == MODULE_STATE_GOING)
+		module_unload_kprobe_ei_list(mod);
+
 	if (val != MODULE_STATE_GOING && val != MODULE_STATE_LIVE)
 		return NOTIFY_DONE;
 
@@ -2240,6 +2346,8 @@ static int __init init_kprobes(void)
 		pr_err("Please take care of using kprobes.\n");
 	}
 
+	populate_kernel_kprobe_ei_list();
+
 	if (kretprobe_blacklist_size) {
 		/* lookup the function address from its name */
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
@@ -2407,6 +2515,56 @@ static const struct file_operations debugfs_kprobe_blacklist_ops = {
 	.release        = seq_release,
 };
 
+/*
+ * kprobes/error_injection_list -- shows which functions can be overriden for
+ * error injection.
+ * */
+static void *kprobe_ei_seq_start(struct seq_file *m, loff_t *pos)
+{
+	mutex_lock(&kprobe_ei_mutex);
+	return seq_list_start(&kprobe_error_injection_list, *pos);
+}
+
+static void kprobe_ei_seq_stop(struct seq_file *m, void *v)
+{
+	mutex_unlock(&kprobe_ei_mutex);
+}
+
+static void *kprobe_ei_seq_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	return seq_list_next(v, &kprobe_error_injection_list, pos);
+}
+
+static int kprobe_ei_seq_show(struct seq_file *m, void *v)
+{
+	char buffer[KSYM_SYMBOL_LEN];
+	struct kprobe_ei_entry *ent =
+		list_entry(v, struct kprobe_ei_entry, list);
+
+	sprint_symbol(buffer, ent->start_addr);
+	seq_printf(m, "%s\n", buffer);
+	return 0;
+}
+
+static const struct seq_operations kprobe_ei_seq_ops = {
+	.start = kprobe_ei_seq_start,
+	.next  = kprobe_ei_seq_next,
+	.stop  = kprobe_ei_seq_stop,
+	.show  = kprobe_ei_seq_show,
+};
+
+static int kprobe_ei_open(struct inode *inode, struct file *filp)
+{
+	return seq_open(filp, &kprobe_ei_seq_ops);
+}
+
+static const struct file_operations debugfs_kprobe_ei_ops = {
+	.open           = kprobe_ei_open,
+	.read           = seq_read,
+	.llseek         = seq_lseek,
+	.release        = seq_release,
+};
+
 static void arm_all_kprobes(void)
 {
 	struct hlist_head *head;
@@ -2548,6 +2706,11 @@ static int __init debugfs_kprobe_init(void)
 	if (!file)
 		goto error;
 
+	file = debugfs_create_file("error_injection_list", 0444, dir, NULL,
+				  &debugfs_kprobe_ei_ops);
+	if (!file)
+		goto error;
+
 	return 0;
 
 error:
diff --git a/kernel/module.c b/kernel/module.c
index dea01ac9cb74..bd695bfdc5c4 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3118,7 +3118,11 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 					     sizeof(*mod->ftrace_callsites),
 					     &mod->num_ftrace_callsites);
 #endif
-
+#ifdef CONFIG_BPF_KPROBE_OVERRIDE
+	mod->kprobe_ei_funcs = section_objs(info, "_kprobe_error_inject_list",
+					    sizeof(*mod->kprobe_ei_funcs),
+					    &mod->num_kprobe_ei_funcs);
+#endif
 	mod->extable = section_objs(info, "__ex_table",
 				    sizeof(*mod->extable), &mod->num_exentries);
 
-- 
2.7.5

[PATCH v9 2/5] btrfs: make open_ctree error injectable

[Josef Bacik]

From: Josef Bacik <jbacik@fb.com>

This allows us to do error injection with BPF for open_ctree.

Signed-off-by: Josef Bacik <jbacik@fb.com>
Acked-by: Ingo Molnar <mingo@kernel.org>
---
 fs/btrfs/disk-io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 10a2a579cc7f..02b5f5667754 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -30,6 +30,7 @@
 #include <linux/ratelimit.h>
 #include <linux/uuid.h>
 #include <linux/semaphore.h>
+#include <linux/bpf.h>
 #include <asm/unaligned.h>
 #include "ctree.h"
 #include "disk-io.h"
@@ -3123,6 +3124,7 @@ int open_ctree(struct super_block *sb,
 		goto fail_block_groups;
 	goto retry_root_backup;
 }
+BPF_ALLOW_ERROR_INJECTION(open_ctree);
 
 static void btrfs_end_buffer_write_sync(struct buffer_head *bh, int uptodate)
 {
-- 
2.7.5

[PATCH v9 3/5] bpf: add a bpf_override_function helper

[Josef Bacik]

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.

Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Ingo Molnar <mingo@kernel.org>
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/linux/filter.h           |  3 ++-
 include/linux/trace_events.h     |  1 +
 include/uapi/linux/bpf.h         |  7 ++++-
 kernel/bpf/core.c                |  3 +++
 kernel/bpf/verifier.c            |  2 ++
 kernel/events/core.c             |  7 +++++
 kernel/trace/Kconfig             | 11 ++++++++
 kernel/trace/bpf_trace.c         | 38 +++++++++++++++++++++++++++
 kernel/trace/trace_kprobe.c      | 55 +++++++++++++++++++++++++++++++++++-----
 kernel/trace/trace_probe.h       | 12 +++++++++
 15 files changed, 157 insertions(+), 9 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 400b9e1b2f27..d3f4aaf9cb7a 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -196,6 +196,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 8eed3f94bfc7..04d66e6fa447 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -154,6 +154,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 9f2e3102e0bb..36abb23a7a35 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 14131dd06b29..6de1fd3d0097 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -109,6 +109,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 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);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 0062302e1285..5feb441d3dd9 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? */
 	enum bpf_prog_type	type;		/* Type of BPF program */
 	u32			len;		/* Number of filter blocks */
 	u32			jited_len;	/* Size of jited insns in bytes */
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index af44e7c2d577..5fea451f6e28 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -528,6 +528,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 80d62e88590c..595bda120cfb 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -677,6 +677,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),			\
@@ -736,7 +740,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/core.c b/kernel/bpf/core.c
index b9f8686a84cf..fc5a8ab4239a 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1320,6 +1320,9 @@ EVAL4(PROG_NAME_LIST, 416, 448, 480, 512)
 bool bpf_prog_array_compatible(struct bpf_array *array,
 			       const struct bpf_prog *fp)
 {
+	if (fp->kprobe_override)
+		return false;
+
 	if (!array->owner_prog_type) {
 		/* There's no owner yet where we could check for
 		 * compatibility.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7afa92e9b409..e807bda7fe29 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4413,6 +4413,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 16beab4767e1..6e3862bbe9c2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8077,6 +8077,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 af7dad126c13..3e6fd580fe7f 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -529,6 +529,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 27d1f4ffa3de..e4bfdbc5a905 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,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;
 	default:
 		return tracing_func_proto(func_id);
 	}
@@ -766,6 +795,15 @@ 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, and only if they
+	 * are on the opt-in list.
+	 */
+	if (prog->kprobe_override &&
+	    (!trace_kprobe_ftrace(event->tp_event) ||
+	     !trace_kprobe_error_injectable(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 492700c5fb4d..5db849809a56 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,27 @@ 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);
+}
+
+int trace_kprobe_error_injectable(struct trace_event_call *call)
+{
+	struct trace_kprobe *tk = (struct trace_kprobe *)call->data;
+	unsigned long addr;
+
+	if (tk->symbol) {
+		addr = (unsigned long)
+			kallsyms_lookup_name(trace_kprobe_symbol(tk));
+		addr += tk->rp.kp.offset;
+	} else {
+		addr = (unsigned long)tk->rp.kp.addr;
+	}
+	return within_kprobe_error_injection_list(addr);
+}
+
 static int register_kprobe_event(struct trace_kprobe *tk);
 static int unregister_kprobe_event(struct trace_kprobe *tk);
 
@@ -1170,7 +1192,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 +1201,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 +1232,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);
+	return 0;
 }
 NOKPROBE_SYMBOL(kprobe_perf_func);
 
@@ -1275,6 +1315,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 +1323,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 fb66e3eaa192..5e54d748c84c 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -252,6 +252,8 @@ 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);
+int trace_kprobe_error_injectable(struct trace_event_call *call);
 #else
 /* uprobes do not support symbol fetch methods */
 #define fetch_symbol_u8			NULL
@@ -277,6 +279,16 @@ alloc_symbol_cache(const char *sym, long offset)
 {
 	return NULL;
 }
+
+static inline int trace_kprobe_ftrace(struct trace_event_call *call)
+{
+	return 0;
+}
+
+static inline int trace_kprobe_error_injectable(struct trace_event_call *call)
+{
+	return 0;
+}
 #endif /* CONFIG_KPROBE_EVENTS */
 
 struct probe_arg {
-- 
2.7.5

[PATCH v9 4/5] samples/bpf: add a test for bpf_override_return

[Josef Bacik]

From: Josef Bacik <jbacik@fb.com>

This adds a basic test for bpf_override_return to verify it works.  We
override the main function for mounting a btrfs fs so it'll return
-ENOMEM and then make sure that trying to mount a btrfs fs will fail.

Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 samples/bpf/Makefile                      |  4 ++++
 samples/bpf/test_override_return.sh       | 15 +++++++++++++++
 samples/bpf/tracex7_kern.c                | 16 ++++++++++++++++
 samples/bpf/tracex7_user.c                | 28 ++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h            |  7 ++++++-
 tools/testing/selftests/bpf/bpf_helpers.h |  3 ++-
 6 files changed, 71 insertions(+), 2 deletions(-)
 create mode 100755 samples/bpf/test_override_return.sh
 create mode 100644 samples/bpf/tracex7_kern.c
 create mode 100644 samples/bpf/tracex7_user.c

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index adeaa1302f34..4fb944a7ecf8 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -12,6 +12,7 @@ hostprogs-y += tracex3
 hostprogs-y += tracex4
 hostprogs-y += tracex5
 hostprogs-y += tracex6
+hostprogs-y += tracex7
 hostprogs-y += test_probe_write_user
 hostprogs-y += trace_output
 hostprogs-y += lathist
@@ -58,6 +59,7 @@ tracex3-objs := bpf_load.o $(LIBBPF) tracex3_user.o
 tracex4-objs := bpf_load.o $(LIBBPF) tracex4_user.o
 tracex5-objs := bpf_load.o $(LIBBPF) tracex5_user.o
 tracex6-objs := bpf_load.o $(LIBBPF) tracex6_user.o
+tracex7-objs := bpf_load.o $(LIBBPF) tracex7_user.o
 load_sock_ops-objs := bpf_load.o $(LIBBPF) load_sock_ops.o
 test_probe_write_user-objs := bpf_load.o $(LIBBPF) test_probe_write_user_user.o
 trace_output-objs := bpf_load.o $(LIBBPF) trace_output_user.o
@@ -101,6 +103,7 @@ always += tracex3_kern.o
 always += tracex4_kern.o
 always += tracex5_kern.o
 always += tracex6_kern.o
+always += tracex7_kern.o
 always += sock_flags_kern.o
 always += test_probe_write_user_kern.o
 always += trace_output_kern.o
@@ -155,6 +158,7 @@ HOSTLOADLIBES_tracex3 += -lelf
 HOSTLOADLIBES_tracex4 += -lelf -lrt
 HOSTLOADLIBES_tracex5 += -lelf
 HOSTLOADLIBES_tracex6 += -lelf
+HOSTLOADLIBES_tracex7 += -lelf
 HOSTLOADLIBES_test_cgrp2_sock2 += -lelf
 HOSTLOADLIBES_load_sock_ops += -lelf
 HOSTLOADLIBES_test_probe_write_user += -lelf
diff --git a/samples/bpf/test_override_return.sh b/samples/bpf/test_override_return.sh
new file mode 100755
index 000000000000..e68b9ee6814b
--- /dev/null
+++ b/samples/bpf/test_override_return.sh
@@ -0,0 +1,15 @@
+#!/bin/bash
+
+rm -f testfile.img
+dd if=/dev/zero of=testfile.img bs=1M seek=1000 count=1
+DEVICE=$(losetup --show -f testfile.img)
+mkfs.btrfs -f $DEVICE
+mkdir tmpmnt
+./tracex7 $DEVICE
+if [ $? -eq 0 ]
+then
+	echo "SUCCESS!"
+else
+	echo "FAILED!"
+fi
+losetup -d $DEVICE
diff --git a/samples/bpf/tracex7_kern.c b/samples/bpf/tracex7_kern.c
new file mode 100644
index 000000000000..1ab308a43e0f
--- /dev/null
+++ b/samples/bpf/tracex7_kern.c
@@ -0,0 +1,16 @@
+#include <uapi/linux/ptrace.h>
+#include <uapi/linux/bpf.h>
+#include <linux/version.h>
+#include "bpf_helpers.h"
+
+SEC("kprobe/open_ctree")
+int bpf_prog1(struct pt_regs *ctx)
+{
+	unsigned long rc = -12;
+
+	bpf_override_return(ctx, rc);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+u32 _version SEC("version") = LINUX_VERSION_CODE;
diff --git a/samples/bpf/tracex7_user.c b/samples/bpf/tracex7_user.c
new file mode 100644
index 000000000000..8a52ac492e8b
--- /dev/null
+++ b/samples/bpf/tracex7_user.c
@@ -0,0 +1,28 @@
+#define _GNU_SOURCE
+
+#include <stdio.h>
+#include <linux/bpf.h>
+#include <unistd.h>
+#include "libbpf.h"
+#include "bpf_load.h"
+
+int main(int argc, char **argv)
+{
+	FILE *f;
+	char filename[256];
+	char command[256];
+	int ret;
+
+	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+
+	if (load_bpf_file(filename)) {
+		printf("%s", bpf_log_buf);
+		return 1;
+	}
+
+	snprintf(command, 256, "mount %s tmpmnt/", argv[1]);
+	f = popen(command, "r");
+	ret = pclose(f);
+
+	return ret ? 0 : 1;
+}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 4c223ab30293..cf446c25c0ec 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -677,6 +677,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),			\
@@ -736,7 +740,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/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index fd9a17fa8a8b..33cb00e46c49 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -82,7 +82,8 @@ static int (*bpf_perf_event_read_value)(void *map, unsigned long long flags,
 static int (*bpf_perf_prog_read_value)(void *ctx, void *buf,
 				       unsigned int buf_size) =
 	(void *) BPF_FUNC_perf_prog_read_value;
-
+static int (*bpf_override_return)(void *ctx, unsigned long rc) =
+	(void *) BPF_FUNC_override_return;
 
 /* llvm builtin functions that eBPF C program may use to
  * emit BPF_LD_ABS and BPF_LD_IND instructions
-- 
2.7.5

[PATCH v9 5/5] btrfs: allow us to inject errors at io_ctl_init

[Josef Bacik]

From: Josef Bacik <jbacik@fb.com>

This was instrumental in reproducing a space cache bug.

Signed-off-by: Josef Bacik <jbacik@fb.com>
Acked-by: Ingo Molnar <mingo@kernel.org>
---
 fs/btrfs/free-space-cache.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 4426d1c73e50..fb1382893bfc 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/math64.h>
 #include <linux/ratelimit.h>
+#include <linux/bpf.h>
 #include "ctree.h"
 #include "free-space-cache.h"
 #include "transaction.h"
@@ -332,6 +333,7 @@ static int io_ctl_init(struct btrfs_io_ctl *io_ctl, struct inode *inode,
 
 	return 0;
 }
+BPF_ALLOW_ERROR_INJECTION(io_ctl_init);
 
 static void io_ctl_free(struct btrfs_io_ctl *io_ctl)
 {
-- 
2.7.5

Re: [PATCH v9 0/5] Add the ability to do BPF directed error injection

[Alexei Starovoitov]

On 12/11/17 8:36 AM, Josef Bacik wrote:
> This is the same as v8, just rebased onto the bpf tree.
>
> v8->v9:
> - rebased onto the bpf tree.
>
> v7->v8:
> - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.
>
> v6->v7:
> - moved the opt-in macro to bpf.h out of kprobes.h.
>
> v5->v6:
> - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this
>   feature.  This way only functions that opt-in will be allowed to be
>   overridden.
> - added a btrfs patch to allow error injection for open_ctree() so that the bpf
>   sample actually works.
>
> v4->v5:
> - disallow kprobe_override programs from being put in the prog map array so we
>   don't tail call into something we didn't check.  This allows us to make the
>   normal path still fast without a bunch of percpu operations.
>
> v3->v4:
> - fix a build error found by kbuild test bot (I didn't wait long enough
>   apparently.)
> - Added a warning message as per Daniels suggestion.
>
> v2->v3:
> - added a ->kprobe_override flag to bpf_prog.
> - added some sanity checks to disallow attaching bpf progs that have
>   ->kprobe_override set that aren't for ftrace kprobes.
> - added the trace_kprobe_ftrace helper to check if the trace_event_call is a
>   ftrace kprobe.
> - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this
>   value in the kprobe path, and thus only write to it if we're overriding or
>   clearing the override.
>
> v1->v2:
> - moved things around to make sure that bpf_override_return could really only be
>   used for an ftrace kprobe.
> - killed the special return values from trace_call_bpf.
> - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if
>   it was being called from an ftrace kprobe context.
> - reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state.
> - updated the test as per Alexei's review.
>
> - Original message -
>
> A lot of our error paths are not well tested because we have no good way of
> injecting errors generically.  Some subystems (block, memory) have ways to
> inject errors, but they are random so it's hard to get reproduceable results.
>
> With BPF we can add determinism to our error injection.  We can use kprobes and
> other things to verify we are injecting errors at the exact case we are trying
> to test.  This patch gives us the tool to actual do the error injection part.
> It is very simple, we just set the return value of the pt_regs we're given to
> whatever we provide, and then override the PC with a dummy function that simply
> returns.
>
> Right now this only works on x86, but it would be simple enough to expand to
> other architectures.  Thanks,

Applied, thanks Josef!

While applying in the patch "bpf: add a bpf_override_function helper"
I moved ifdef CONFIG_BPF_KPROBE_OVERRIDE few lines,
so when it's not set the program will fail at load time with error
"unknown func bpf_override_return#58"
instead of returning EINVAL at run-time.
That's more standard way of adding new helpers.

Thanks

Re: [PATCH v9 0/5] Add the ability to do BPF directed error injection

[Darrick J. Wong]

On Mon, Dec 11, 2017 at 11:36:45AM -0500, Josef Bacik wrote:
> This is the same as v8, just rebased onto the bpf tree.
> 
> v8->v9:
> - rebased onto the bpf tree.
> 
> v7->v8:
> - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.
> 
> v6->v7:
> - moved the opt-in macro to bpf.h out of kprobes.h.
> 
> v5->v6:
> - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this
>   feature.  This way only functions that opt-in will be allowed to be
>   overridden.
> - added a btrfs patch to allow error injection for open_ctree() so that the bpf
>   sample actually works.
> 
> v4->v5:
> - disallow kprobe_override programs from being put in the prog map array so we
>   don't tail call into something we didn't check.  This allows us to make the
>   normal path still fast without a bunch of percpu operations.
> 
> v3->v4:
> - fix a build error found by kbuild test bot (I didn't wait long enough
>   apparently.)
> - Added a warning message as per Daniels suggestion.
> 
> v2->v3:
> - added a ->kprobe_override flag to bpf_prog.
> - added some sanity checks to disallow attaching bpf progs that have
>   ->kprobe_override set that aren't for ftrace kprobes.
> - added the trace_kprobe_ftrace helper to check if the trace_event_call is a
>   ftrace kprobe.
> - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this
>   value in the kprobe path, and thus only write to it if we're overriding or
>   clearing the override.
> 
> v1->v2:
> - moved things around to make sure that bpf_override_return could really only be
>   used for an ftrace kprobe.
> - killed the special return values from trace_call_bpf.
> - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if
>   it was being called from an ftrace kprobe context.
> - reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state.
> - updated the test as per Alexei's review.
> 
> - Original message -
> 
> A lot of our error paths are not well tested because we have no good way of
> injecting errors generically.  Some subystems (block, memory) have ways to
> inject errors, but they are random so it's hard to get reproduceable results.
> 
> With BPF we can add determinism to our error injection.  We can use kprobes and
> other things to verify we are injecting errors at the exact case we are trying
> to test.  This patch gives us the tool to actual do the error injection part.
> It is very simple, we just set the return value of the pt_regs we're given to
> whatever we provide, and then override the PC with a dummy function that simply
> returns.

Heh, this looks cool.  I decided to try it to see what happens, and saw
a bunch of dmesg pasted in below.  Is that supposed to happen?  Or am I
the only fs developer still running with lockdep enabled? :)

It looks like bpf_override_return has some sort of side effect such that
we get the splat, since commenting it out makes the symptom go away.

<shrug>

--D

[ 1847.769183] BTRFS error (device (null)): open_ctree failed
[ 1847.770130] BUG: sleeping function called from invalid context at /storage/home/djwong/cdev/work/linux-xfs/kernel/locking/rwsem.c:69
[ 1847.771976] in_atomic(): 1, irqs_disabled(): 0, pid: 1524, name: mount
[ 1847.773016] 1 lock held by mount/1524:
[ 1847.773530]  #0:  (&type->s_umount_key#34/1){+.+.}, at: [<00000000653a9bb4>] sget_userns+0x302/0x4f0
[ 1847.774731] Preemption disabled at:
[ 1847.774735] [<          (null)>]           (null)
[ 1847.777009] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
[ 1847.778800] Call Trace:
[ 1847.779047]  dump_stack+0x7c/0xbe
[ 1847.779361]  ___might_sleep+0x1f7/0x260
[ 1847.779720]  down_write+0x29/0xb0
[ 1847.780046]  unregister_shrinker+0x15/0x70
[ 1847.780427]  deactivate_locked_super+0x2e/0x60
[ 1847.780935]  btrfs_mount+0xbb6/0x1000 [btrfs]
[ 1847.781353]  ? __lockdep_init_map+0x5c/0x1d0
[ 1847.781750]  ? mount_fs+0xf/0x80
[ 1847.782065]  ? alloc_vfsmnt+0x1a1/0x230
[ 1847.782429]  mount_fs+0xf/0x80
[ 1847.782733]  vfs_kern_mount+0x62/0x160
[ 1847.783128]  btrfs_mount+0x3d3/0x1000 [btrfs]
[ 1847.783493]  ? __lockdep_init_map+0x5c/0x1d0
[ 1847.783849]  ? __lockdep_init_map+0x5c/0x1d0
[ 1847.784207]  ? mount_fs+0xf/0x80
[ 1847.784502]  mount_fs+0xf/0x80
[ 1847.784835]  vfs_kern_mount+0x62/0x160
[ 1847.785235]  do_mount+0x1b1/0xd50
[ 1847.785594]  ? _copy_from_user+0x5b/0x90
[ 1847.786028]  ? memdup_user+0x4b/0x70
[ 1847.786501]  SyS_mount+0x85/0xd0
[ 1847.786835]  entry_SYSCALL_64_fastpath+0x1f/0x96
[ 1847.787311] RIP: 0033:0x7f6ebecc1b5a
[ 1847.787691] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
[ 1847.788383] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
[ 1847.789106] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
[ 1847.789807] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
[ 1847.790511] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
[ 1847.791211] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
[ 1847.792029] BUG: scheduling while atomic: mount/1524/0x00000002
[ 1847.792680] 1 lock held by mount/1524:
[ 1847.793087]  #0:  (rcu_preempt_state.exp_mutex){+.+.}, at: [<00000000a6c536a9>] _synchronize_rcu_expedited+0x1ce/0x400
[ 1847.794129] Modules linked in: xfs libcrc32c btrfs xor zstd_decompress zstd_compress xxhash lzo_compress lzo_decompress zlib_deflate raid6_pq dax_pmem device_dax nd_pmem sch_fq_codel af_packet [last unloaded: xfs]
[ 1847.795949] Preemption disabled at:
[ 1847.795951] [<          (null)>]           (null)
[ 1847.796844] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
[ 1847.797621] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1djwong0 04/01/2014
[ 1847.798510] Call Trace:
[ 1847.798786]  dump_stack+0x7c/0xbe
[ 1847.799134]  __schedule_bug+0x88/0xe0
[ 1847.799517]  __schedule+0x78c/0xb20
[ 1847.799890]  ? trace_hardirqs_on_caller+0x119/0x180
[ 1847.800391]  schedule+0x40/0x90
[ 1847.800729]  _synchronize_rcu_expedited+0x36b/0x400
[ 1847.801218]  ? rcu_preempt_qs+0xa0/0xa0
[ 1847.801616]  ? remove_wait_queue+0x60/0x60
[ 1847.802040]  ? rcu_preempt_qs+0xa0/0xa0
[ 1847.802433]  ? rcu_exp_wait_wake+0x630/0x630
[ 1847.802872]  ? __lock_acquire+0xfb9/0x1120
[ 1847.803302]  ? __lock_acquire+0x534/0x1120
[ 1847.803725]  ? bdi_unregister+0x57/0x1a0
[ 1847.804135]  bdi_unregister+0x5c/0x1a0
[ 1847.804519]  bdi_put+0xcb/0xe0
[ 1847.804746]  generic_shutdown_super+0xe2/0x110
[ 1847.805066]  kill_anon_super+0xe/0x20
[ 1847.805344]  btrfs_kill_super+0x12/0xa0 [btrfs]
[ 1847.805664]  deactivate_locked_super+0x34/0x60
[ 1847.806111]  btrfs_mount+0xbb6/0x1000 [btrfs]
[ 1847.806476]  ? __lockdep_init_map+0x5c/0x1d0
[ 1847.806824]  ? mount_fs+0xf/0x80
[ 1847.807104]  ? alloc_vfsmnt+0x1a1/0x230
[ 1847.807416]  mount_fs+0xf/0x80
[ 1847.807712]  vfs_kern_mount+0x62/0x160
[ 1847.808112]  btrfs_mount+0x3d3/0x1000 [btrfs]
[ 1847.808565]  ? __lockdep_init_map+0x5c/0x1d0
[ 1847.809005]  ? __lockdep_init_map+0x5c/0x1d0
[ 1847.809425]  ? mount_fs+0xf/0x80
[ 1847.809731]  mount_fs+0xf/0x80
[ 1847.810070]  vfs_kern_mount+0x62/0x160
[ 1847.810469]  do_mount+0x1b1/0xd50
[ 1847.810821]  ? _copy_from_user+0x5b/0x90
[ 1847.811237]  ? memdup_user+0x4b/0x70
[ 1847.811622]  SyS_mount+0x85/0xd0
[ 1847.811996]  entry_SYSCALL_64_fastpath+0x1f/0x96
[ 1847.812465] RIP: 0033:0x7f6ebecc1b5a
[ 1847.812840] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
[ 1847.813615] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
[ 1847.814302] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
[ 1847.814770] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
[ 1847.815246] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
[ 1847.815720] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001

--D

> 
> Right now this only works on x86, but it would be simple enough to expand to
> other architectures.  Thanks,
> 
> Josef
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 0/5] Add the ability to do BPF directed error injection

[Josef Bacik]

On Tue, Dec 12, 2017 at 03:11:50PM -0800, Darrick J. Wong wrote:
> On Mon, Dec 11, 2017 at 11:36:45AM -0500, Josef Bacik wrote:
> > This is the same as v8, just rebased onto the bpf tree.
> > 
> > v8->v9:
> > - rebased onto the bpf tree.
> > 
> > v7->v8:
> > - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.
> > 
> > v6->v7:
> > - moved the opt-in macro to bpf.h out of kprobes.h.
> > 
> > v5->v6:
> > - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this
> >   feature.  This way only functions that opt-in will be allowed to be
> >   overridden.
> > - added a btrfs patch to allow error injection for open_ctree() so that the bpf
> >   sample actually works.
> > 
> > v4->v5:
> > - disallow kprobe_override programs from being put in the prog map array so we
> >   don't tail call into something we didn't check.  This allows us to make the
> >   normal path still fast without a bunch of percpu operations.
> > 
> > v3->v4:
> > - fix a build error found by kbuild test bot (I didn't wait long enough
> >   apparently.)
> > - Added a warning message as per Daniels suggestion.
> > 
> > v2->v3:
> > - added a ->kprobe_override flag to bpf_prog.
> > - added some sanity checks to disallow attaching bpf progs that have
> >   ->kprobe_override set that aren't for ftrace kprobes.
> > - added the trace_kprobe_ftrace helper to check if the trace_event_call is a
> >   ftrace kprobe.
> > - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this
> >   value in the kprobe path, and thus only write to it if we're overriding or
> >   clearing the override.
> > 
> > v1->v2:
> > - moved things around to make sure that bpf_override_return could really only be
> >   used for an ftrace kprobe.
> > - killed the special return values from trace_call_bpf.
> > - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if
> >   it was being called from an ftrace kprobe context.
> > - reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state.
> > - updated the test as per Alexei's review.
> > 
> > - Original message -
> > 
> > A lot of our error paths are not well tested because we have no good way of
> > injecting errors generically.  Some subystems (block, memory) have ways to
> > inject errors, but they are random so it's hard to get reproduceable results.
> > 
> > With BPF we can add determinism to our error injection.  We can use kprobes and
> > other things to verify we are injecting errors at the exact case we are trying
> > to test.  This patch gives us the tool to actual do the error injection part.
> > It is very simple, we just set the return value of the pt_regs we're given to
> > whatever we provide, and then override the PC with a dummy function that simply
> > returns.
> 
> Heh, this looks cool.  I decided to try it to see what happens, and saw
> a bunch of dmesg pasted in below.  Is that supposed to happen?  Or am I
> the only fs developer still running with lockdep enabled? :)
> 
> It looks like bpf_override_return has some sort of side effect such that
> we get the splat, since commenting it out makes the symptom go away.
> 
> <shrug>
> 
> --D
> 
> [ 1847.769183] BTRFS error (device (null)): open_ctree failed
> [ 1847.770130] BUG: sleeping function called from invalid context at /storage/home/djwong/cdev/work/linux-xfs/kernel/locking/rwsem.c:69
> [ 1847.771976] in_atomic(): 1, irqs_disabled(): 0, pid: 1524, name: mount
> [ 1847.773016] 1 lock held by mount/1524:
> [ 1847.773530]  #0:  (&type->s_umount_key#34/1){+.+.}, at: [<00000000653a9bb4>] sget_userns+0x302/0x4f0
> [ 1847.774731] Preemption disabled at:
> [ 1847.774735] [<          (null)>]           (null)
> [ 1847.777009] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> [ 1847.778800] Call Trace:
> [ 1847.779047]  dump_stack+0x7c/0xbe
> [ 1847.779361]  ___might_sleep+0x1f7/0x260
> [ 1847.779720]  down_write+0x29/0xb0
> [ 1847.780046]  unregister_shrinker+0x15/0x70
> [ 1847.780427]  deactivate_locked_super+0x2e/0x60
> [ 1847.780935]  btrfs_mount+0xbb6/0x1000 [btrfs]
> [ 1847.781353]  ? __lockdep_init_map+0x5c/0x1d0
> [ 1847.781750]  ? mount_fs+0xf/0x80
> [ 1847.782065]  ? alloc_vfsmnt+0x1a1/0x230
> [ 1847.782429]  mount_fs+0xf/0x80
> [ 1847.782733]  vfs_kern_mount+0x62/0x160
> [ 1847.783128]  btrfs_mount+0x3d3/0x1000 [btrfs]
> [ 1847.783493]  ? __lockdep_init_map+0x5c/0x1d0
> [ 1847.783849]  ? __lockdep_init_map+0x5c/0x1d0
> [ 1847.784207]  ? mount_fs+0xf/0x80
> [ 1847.784502]  mount_fs+0xf/0x80
> [ 1847.784835]  vfs_kern_mount+0x62/0x160
> [ 1847.785235]  do_mount+0x1b1/0xd50
> [ 1847.785594]  ? _copy_from_user+0x5b/0x90
> [ 1847.786028]  ? memdup_user+0x4b/0x70
> [ 1847.786501]  SyS_mount+0x85/0xd0
> [ 1847.786835]  entry_SYSCALL_64_fastpath+0x1f/0x96
> [ 1847.787311] RIP: 0033:0x7f6ebecc1b5a
> [ 1847.787691] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> [ 1847.788383] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> [ 1847.789106] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> [ 1847.789807] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> [ 1847.790511] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> [ 1847.791211] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
> [ 1847.792029] BUG: scheduling while atomic: mount/1524/0x00000002
> [ 1847.792680] 1 lock held by mount/1524:
> [ 1847.793087]  #0:  (rcu_preempt_state.exp_mutex){+.+.}, at: [<00000000a6c536a9>] _synchronize_rcu_expedited+0x1ce/0x400
> [ 1847.794129] Modules linked in: xfs libcrc32c btrfs xor zstd_decompress zstd_compress xxhash lzo_compress lzo_decompress zlib_deflate raid6_pq dax_pmem device_dax nd_pmem sch_fq_codel af_packet [last unloaded: xfs]
> [ 1847.795949] Preemption disabled at:
> [ 1847.795951] [<          (null)>]           (null)
> [ 1847.796844] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> [ 1847.797621] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1djwong0 04/01/2014
> [ 1847.798510] Call Trace:
> [ 1847.798786]  dump_stack+0x7c/0xbe
> [ 1847.799134]  __schedule_bug+0x88/0xe0
> [ 1847.799517]  __schedule+0x78c/0xb20
> [ 1847.799890]  ? trace_hardirqs_on_caller+0x119/0x180
> [ 1847.800391]  schedule+0x40/0x90
> [ 1847.800729]  _synchronize_rcu_expedited+0x36b/0x400
> [ 1847.801218]  ? rcu_preempt_qs+0xa0/0xa0
> [ 1847.801616]  ? remove_wait_queue+0x60/0x60
> [ 1847.802040]  ? rcu_preempt_qs+0xa0/0xa0
> [ 1847.802433]  ? rcu_exp_wait_wake+0x630/0x630
> [ 1847.802872]  ? __lock_acquire+0xfb9/0x1120
> [ 1847.803302]  ? __lock_acquire+0x534/0x1120
> [ 1847.803725]  ? bdi_unregister+0x57/0x1a0
> [ 1847.804135]  bdi_unregister+0x5c/0x1a0
> [ 1847.804519]  bdi_put+0xcb/0xe0
> [ 1847.804746]  generic_shutdown_super+0xe2/0x110
> [ 1847.805066]  kill_anon_super+0xe/0x20
> [ 1847.805344]  btrfs_kill_super+0x12/0xa0 [btrfs]
> [ 1847.805664]  deactivate_locked_super+0x34/0x60
> [ 1847.806111]  btrfs_mount+0xbb6/0x1000 [btrfs]
> [ 1847.806476]  ? __lockdep_init_map+0x5c/0x1d0
> [ 1847.806824]  ? mount_fs+0xf/0x80
> [ 1847.807104]  ? alloc_vfsmnt+0x1a1/0x230
> [ 1847.807416]  mount_fs+0xf/0x80
> [ 1847.807712]  vfs_kern_mount+0x62/0x160
> [ 1847.808112]  btrfs_mount+0x3d3/0x1000 [btrfs]
> [ 1847.808565]  ? __lockdep_init_map+0x5c/0x1d0
> [ 1847.809005]  ? __lockdep_init_map+0x5c/0x1d0
> [ 1847.809425]  ? mount_fs+0xf/0x80
> [ 1847.809731]  mount_fs+0xf/0x80
> [ 1847.810070]  vfs_kern_mount+0x62/0x160
> [ 1847.810469]  do_mount+0x1b1/0xd50
> [ 1847.810821]  ? _copy_from_user+0x5b/0x90
> [ 1847.811237]  ? memdup_user+0x4b/0x70
> [ 1847.811622]  SyS_mount+0x85/0xd0
> [ 1847.811996]  entry_SYSCALL_64_fastpath+0x1f/0x96
> [ 1847.812465] RIP: 0033:0x7f6ebecc1b5a
> [ 1847.812840] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> [ 1847.813615] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> [ 1847.814302] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> [ 1847.814770] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> [ 1847.815246] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> [ 1847.815720] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
i> 

Looks like this is new, Masami this is happening because of your change here

5bb4fc2d8641 ("kprobes/x86: Disable preemption in ftrace-based jprobes")

which makes it not do the preempt_enable() if the handler returns 1.  Why is
that?  Should I be doing preempt_enable_no_resched() from the handler before
returning 1?  Or is this just an oversight on your part?  Thanks,

Josef

Re: [PATCH v9 0/5] Add the ability to do BPF directed error injection

[Darrick J. Wong]

On Wed, Dec 13, 2017 at 01:03:57PM -0500, Josef Bacik wrote:
> On Tue, Dec 12, 2017 at 03:11:50PM -0800, Darrick J. Wong wrote:
> > On Mon, Dec 11, 2017 at 11:36:45AM -0500, Josef Bacik wrote:
> > > This is the same as v8, just rebased onto the bpf tree.
> > > 
> > > v8->v9:
> > > - rebased onto the bpf tree.
> > > 
> > > v7->v8:
> > > - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.
> > > 
> > > v6->v7:
> > > - moved the opt-in macro to bpf.h out of kprobes.h.
> > > 
> > > v5->v6:
> > > - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this
> > >   feature.  This way only functions that opt-in will be allowed to be
> > >   overridden.
> > > - added a btrfs patch to allow error injection for open_ctree() so that the bpf
> > >   sample actually works.
> > > 
> > > v4->v5:
> > > - disallow kprobe_override programs from being put in the prog map array so we
> > >   don't tail call into something we didn't check.  This allows us to make the
> > >   normal path still fast without a bunch of percpu operations.
> > > 
> > > v3->v4:
> > > - fix a build error found by kbuild test bot (I didn't wait long enough
> > >   apparently.)
> > > - Added a warning message as per Daniels suggestion.
> > > 
> > > v2->v3:
> > > - added a ->kprobe_override flag to bpf_prog.
> > > - added some sanity checks to disallow attaching bpf progs that have
> > >   ->kprobe_override set that aren't for ftrace kprobes.
> > > - added the trace_kprobe_ftrace helper to check if the trace_event_call is a
> > >   ftrace kprobe.
> > > - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this
> > >   value in the kprobe path, and thus only write to it if we're overriding or
> > >   clearing the override.
> > > 
> > > v1->v2:
> > > - moved things around to make sure that bpf_override_return could really only be
> > >   used for an ftrace kprobe.
> > > - killed the special return values from trace_call_bpf.
> > > - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if
> > >   it was being called from an ftrace kprobe context.
> > > - reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state.
> > > - updated the test as per Alexei's review.
> > > 
> > > - Original message -
> > > 
> > > A lot of our error paths are not well tested because we have no good way of
> > > injecting errors generically.  Some subystems (block, memory) have ways to
> > > inject errors, but they are random so it's hard to get reproduceable results.
> > > 
> > > With BPF we can add determinism to our error injection.  We can use kprobes and
> > > other things to verify we are injecting errors at the exact case we are trying
> > > to test.  This patch gives us the tool to actual do the error injection part.
> > > It is very simple, we just set the return value of the pt_regs we're given to
> > > whatever we provide, and then override the PC with a dummy function that simply
> > > returns.
> > 
> > Heh, this looks cool.  I decided to try it to see what happens, and saw
> > a bunch of dmesg pasted in below.  Is that supposed to happen?  Or am I
> > the only fs developer still running with lockdep enabled? :)
> > 
> > It looks like bpf_override_return has some sort of side effect such that
> > we get the splat, since commenting it out makes the symptom go away.
> > 
> > <shrug>
> > 
> > --D
> > 
> > [ 1847.769183] BTRFS error (device (null)): open_ctree failed
> > [ 1847.770130] BUG: sleeping function called from invalid context at /storage/home/djwong/cdev/work/linux-xfs/kernel/locking/rwsem.c:69
> > [ 1847.771976] in_atomic(): 1, irqs_disabled(): 0, pid: 1524, name: mount
> > [ 1847.773016] 1 lock held by mount/1524:
> > [ 1847.773530]  #0:  (&type->s_umount_key#34/1){+.+.}, at: [<00000000653a9bb4>] sget_userns+0x302/0x4f0
> > [ 1847.774731] Preemption disabled at:
> > [ 1847.774735] [<          (null)>]           (null)
> > [ 1847.777009] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> > [ 1847.778800] Call Trace:
> > [ 1847.779047]  dump_stack+0x7c/0xbe
> > [ 1847.779361]  ___might_sleep+0x1f7/0x260
> > [ 1847.779720]  down_write+0x29/0xb0
> > [ 1847.780046]  unregister_shrinker+0x15/0x70
> > [ 1847.780427]  deactivate_locked_super+0x2e/0x60
> > [ 1847.780935]  btrfs_mount+0xbb6/0x1000 [btrfs]
> > [ 1847.781353]  ? __lockdep_init_map+0x5c/0x1d0
> > [ 1847.781750]  ? mount_fs+0xf/0x80
> > [ 1847.782065]  ? alloc_vfsmnt+0x1a1/0x230
> > [ 1847.782429]  mount_fs+0xf/0x80
> > [ 1847.782733]  vfs_kern_mount+0x62/0x160
> > [ 1847.783128]  btrfs_mount+0x3d3/0x1000 [btrfs]
> > [ 1847.783493]  ? __lockdep_init_map+0x5c/0x1d0
> > [ 1847.783849]  ? __lockdep_init_map+0x5c/0x1d0
> > [ 1847.784207]  ? mount_fs+0xf/0x80
> > [ 1847.784502]  mount_fs+0xf/0x80
> > [ 1847.784835]  vfs_kern_mount+0x62/0x160
> > [ 1847.785235]  do_mount+0x1b1/0xd50
> > [ 1847.785594]  ? _copy_from_user+0x5b/0x90
> > [ 1847.786028]  ? memdup_user+0x4b/0x70
> > [ 1847.786501]  SyS_mount+0x85/0xd0
> > [ 1847.786835]  entry_SYSCALL_64_fastpath+0x1f/0x96
> > [ 1847.787311] RIP: 0033:0x7f6ebecc1b5a
> > [ 1847.787691] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> > [ 1847.788383] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> > [ 1847.789106] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> > [ 1847.789807] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> > [ 1847.790511] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> > [ 1847.791211] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
> > [ 1847.792029] BUG: scheduling while atomic: mount/1524/0x00000002
> > [ 1847.792680] 1 lock held by mount/1524:
> > [ 1847.793087]  #0:  (rcu_preempt_state.exp_mutex){+.+.}, at: [<00000000a6c536a9>] _synchronize_rcu_expedited+0x1ce/0x400
> > [ 1847.794129] Modules linked in: xfs libcrc32c btrfs xor zstd_decompress zstd_compress xxhash lzo_compress lzo_decompress zlib_deflate raid6_pq dax_pmem device_dax nd_pmem sch_fq_codel af_packet [last unloaded: xfs]
> > [ 1847.795949] Preemption disabled at:
> > [ 1847.795951] [<          (null)>]           (null)
> > [ 1847.796844] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> > [ 1847.797621] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1djwong0 04/01/2014
> > [ 1847.798510] Call Trace:
> > [ 1847.798786]  dump_stack+0x7c/0xbe
> > [ 1847.799134]  __schedule_bug+0x88/0xe0
> > [ 1847.799517]  __schedule+0x78c/0xb20
> > [ 1847.799890]  ? trace_hardirqs_on_caller+0x119/0x180
> > [ 1847.800391]  schedule+0x40/0x90
> > [ 1847.800729]  _synchronize_rcu_expedited+0x36b/0x400
> > [ 1847.801218]  ? rcu_preempt_qs+0xa0/0xa0
> > [ 1847.801616]  ? remove_wait_queue+0x60/0x60
> > [ 1847.802040]  ? rcu_preempt_qs+0xa0/0xa0
> > [ 1847.802433]  ? rcu_exp_wait_wake+0x630/0x630
> > [ 1847.802872]  ? __lock_acquire+0xfb9/0x1120
> > [ 1847.803302]  ? __lock_acquire+0x534/0x1120
> > [ 1847.803725]  ? bdi_unregister+0x57/0x1a0
> > [ 1847.804135]  bdi_unregister+0x5c/0x1a0
> > [ 1847.804519]  bdi_put+0xcb/0xe0
> > [ 1847.804746]  generic_shutdown_super+0xe2/0x110
> > [ 1847.805066]  kill_anon_super+0xe/0x20
> > [ 1847.805344]  btrfs_kill_super+0x12/0xa0 [btrfs]
> > [ 1847.805664]  deactivate_locked_super+0x34/0x60
> > [ 1847.806111]  btrfs_mount+0xbb6/0x1000 [btrfs]
> > [ 1847.806476]  ? __lockdep_init_map+0x5c/0x1d0
> > [ 1847.806824]  ? mount_fs+0xf/0x80
> > [ 1847.807104]  ? alloc_vfsmnt+0x1a1/0x230
> > [ 1847.807416]  mount_fs+0xf/0x80
> > [ 1847.807712]  vfs_kern_mount+0x62/0x160
> > [ 1847.808112]  btrfs_mount+0x3d3/0x1000 [btrfs]
> > [ 1847.808565]  ? __lockdep_init_map+0x5c/0x1d0
> > [ 1847.809005]  ? __lockdep_init_map+0x5c/0x1d0
> > [ 1847.809425]  ? mount_fs+0xf/0x80
> > [ 1847.809731]  mount_fs+0xf/0x80
> > [ 1847.810070]  vfs_kern_mount+0x62/0x160
> > [ 1847.810469]  do_mount+0x1b1/0xd50
> > [ 1847.810821]  ? _copy_from_user+0x5b/0x90
> > [ 1847.811237]  ? memdup_user+0x4b/0x70
> > [ 1847.811622]  SyS_mount+0x85/0xd0
> > [ 1847.811996]  entry_SYSCALL_64_fastpath+0x1f/0x96
> > [ 1847.812465] RIP: 0033:0x7f6ebecc1b5a
> > [ 1847.812840] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> > [ 1847.813615] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> > [ 1847.814302] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> > [ 1847.814770] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> > [ 1847.815246] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> > [ 1847.815720] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
> i> 
> 
> Looks like this is new, Masami this is happening because of your change here
> 
> 5bb4fc2d8641 ("kprobes/x86: Disable preemption in ftrace-based jprobes")
> 
> which makes it not do the preempt_enable() if the handler returns 1.  Why is
> that?  Should I be doing preempt_enable_no_resched() from the handler before
> returning 1?  Or is this just an oversight on your part?  Thanks,

FWIW I shut up the preemption imbalance warnings with the attached
coarse bandaid.  No idea if that's the correct fix...

--D

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 5db8498..fd948e3 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1215,8 +1215,10 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
 		if (__this_cpu_read(bpf_kprobe_override)) {
 			__this_cpu_write(bpf_kprobe_override, 0);
 			reset_current_kprobe();
+			preempt_enable();
 			return 1;
 		}
+		preempt_enable();
 		if (!ret)
 			return 0;
 	}
> 
> Josef

Re: [PATCH v9 0/5] Add the ability to do BPF directed error injection

[Josef Bacik]

On Wed, Dec 13, 2017 at 10:07:32AM -0800, Darrick J. Wong wrote:
> On Wed, Dec 13, 2017 at 01:03:57PM -0500, Josef Bacik wrote:
> > On Tue, Dec 12, 2017 at 03:11:50PM -0800, Darrick J. Wong wrote:
> > > On Mon, Dec 11, 2017 at 11:36:45AM -0500, Josef Bacik wrote:
> > > > This is the same as v8, just rebased onto the bpf tree.
> > > > 
> > > > v8->v9:
> > > > - rebased onto the bpf tree.
> > > > 
> > > > v7->v8:
> > > > - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.
> > > > 
> > > > v6->v7:
> > > > - moved the opt-in macro to bpf.h out of kprobes.h.
> > > > 
> > > > v5->v6:
> > > > - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this
> > > >   feature.  This way only functions that opt-in will be allowed to be
> > > >   overridden.
> > > > - added a btrfs patch to allow error injection for open_ctree() so that the bpf
> > > >   sample actually works.
> > > > 
> > > > v4->v5:
> > > > - disallow kprobe_override programs from being put in the prog map array so we
> > > >   don't tail call into something we didn't check.  This allows us to make the
> > > >   normal path still fast without a bunch of percpu operations.
> > > > 
> > > > v3->v4:
> > > > - fix a build error found by kbuild test bot (I didn't wait long enough
> > > >   apparently.)
> > > > - Added a warning message as per Daniels suggestion.
> > > > 
> > > > v2->v3:
> > > > - added a ->kprobe_override flag to bpf_prog.
> > > > - added some sanity checks to disallow attaching bpf progs that have
> > > >   ->kprobe_override set that aren't for ftrace kprobes.
> > > > - added the trace_kprobe_ftrace helper to check if the trace_event_call is a
> > > >   ftrace kprobe.
> > > > - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this
> > > >   value in the kprobe path, and thus only write to it if we're overriding or
> > > >   clearing the override.
> > > > 
> > > > v1->v2:
> > > > - moved things around to make sure that bpf_override_return could really only be
> > > >   used for an ftrace kprobe.
> > > > - killed the special return values from trace_call_bpf.
> > > > - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if
> > > >   it was being called from an ftrace kprobe context.
> > > > - reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state.
> > > > - updated the test as per Alexei's review.
> > > > 
> > > > - Original message -
> > > > 
> > > > A lot of our error paths are not well tested because we have no good way of
> > > > injecting errors generically.  Some subystems (block, memory) have ways to
> > > > inject errors, but they are random so it's hard to get reproduceable results.
> > > > 
> > > > With BPF we can add determinism to our error injection.  We can use kprobes and
> > > > other things to verify we are injecting errors at the exact case we are trying
> > > > to test.  This patch gives us the tool to actual do the error injection part.
> > > > It is very simple, we just set the return value of the pt_regs we're given to
> > > > whatever we provide, and then override the PC with a dummy function that simply
> > > > returns.
> > > 
> > > Heh, this looks cool.  I decided to try it to see what happens, and saw
> > > a bunch of dmesg pasted in below.  Is that supposed to happen?  Or am I
> > > the only fs developer still running with lockdep enabled? :)
> > > 
> > > It looks like bpf_override_return has some sort of side effect such that
> > > we get the splat, since commenting it out makes the symptom go away.
> > > 
> > > <shrug>
> > > 
> > > --D
> > > 
> > > [ 1847.769183] BTRFS error (device (null)): open_ctree failed
> > > [ 1847.770130] BUG: sleeping function called from invalid context at /storage/home/djwong/cdev/work/linux-xfs/kernel/locking/rwsem.c:69
> > > [ 1847.771976] in_atomic(): 1, irqs_disabled(): 0, pid: 1524, name: mount
> > > [ 1847.773016] 1 lock held by mount/1524:
> > > [ 1847.773530]  #0:  (&type->s_umount_key#34/1){+.+.}, at: [<00000000653a9bb4>] sget_userns+0x302/0x4f0
> > > [ 1847.774731] Preemption disabled at:
> > > [ 1847.774735] [<          (null)>]           (null)
> > > [ 1847.777009] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> > > [ 1847.778800] Call Trace:
> > > [ 1847.779047]  dump_stack+0x7c/0xbe
> > > [ 1847.779361]  ___might_sleep+0x1f7/0x260
> > > [ 1847.779720]  down_write+0x29/0xb0
> > > [ 1847.780046]  unregister_shrinker+0x15/0x70
> > > [ 1847.780427]  deactivate_locked_super+0x2e/0x60
> > > [ 1847.780935]  btrfs_mount+0xbb6/0x1000 [btrfs]
> > > [ 1847.781353]  ? __lockdep_init_map+0x5c/0x1d0
> > > [ 1847.781750]  ? mount_fs+0xf/0x80
> > > [ 1847.782065]  ? alloc_vfsmnt+0x1a1/0x230
> > > [ 1847.782429]  mount_fs+0xf/0x80
> > > [ 1847.782733]  vfs_kern_mount+0x62/0x160
> > > [ 1847.783128]  btrfs_mount+0x3d3/0x1000 [btrfs]
> > > [ 1847.783493]  ? __lockdep_init_map+0x5c/0x1d0
> > > [ 1847.783849]  ? __lockdep_init_map+0x5c/0x1d0
> > > [ 1847.784207]  ? mount_fs+0xf/0x80
> > > [ 1847.784502]  mount_fs+0xf/0x80
> > > [ 1847.784835]  vfs_kern_mount+0x62/0x160
> > > [ 1847.785235]  do_mount+0x1b1/0xd50
> > > [ 1847.785594]  ? _copy_from_user+0x5b/0x90
> > > [ 1847.786028]  ? memdup_user+0x4b/0x70
> > > [ 1847.786501]  SyS_mount+0x85/0xd0
> > > [ 1847.786835]  entry_SYSCALL_64_fastpath+0x1f/0x96
> > > [ 1847.787311] RIP: 0033:0x7f6ebecc1b5a
> > > [ 1847.787691] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> > > [ 1847.788383] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> > > [ 1847.789106] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> > > [ 1847.789807] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> > > [ 1847.790511] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> > > [ 1847.791211] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
> > > [ 1847.792029] BUG: scheduling while atomic: mount/1524/0x00000002
> > > [ 1847.792680] 1 lock held by mount/1524:
> > > [ 1847.793087]  #0:  (rcu_preempt_state.exp_mutex){+.+.}, at: [<00000000a6c536a9>] _synchronize_rcu_expedited+0x1ce/0x400
> > > [ 1847.794129] Modules linked in: xfs libcrc32c btrfs xor zstd_decompress zstd_compress xxhash lzo_compress lzo_decompress zlib_deflate raid6_pq dax_pmem device_dax nd_pmem sch_fq_codel af_packet [last unloaded: xfs]
> > > [ 1847.795949] Preemption disabled at:
> > > [ 1847.795951] [<          (null)>]           (null)
> > > [ 1847.796844] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> > > [ 1847.797621] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1djwong0 04/01/2014
> > > [ 1847.798510] Call Trace:
> > > [ 1847.798786]  dump_stack+0x7c/0xbe
> > > [ 1847.799134]  __schedule_bug+0x88/0xe0
> > > [ 1847.799517]  __schedule+0x78c/0xb20
> > > [ 1847.799890]  ? trace_hardirqs_on_caller+0x119/0x180
> > > [ 1847.800391]  schedule+0x40/0x90
> > > [ 1847.800729]  _synchronize_rcu_expedited+0x36b/0x400
> > > [ 1847.801218]  ? rcu_preempt_qs+0xa0/0xa0
> > > [ 1847.801616]  ? remove_wait_queue+0x60/0x60
> > > [ 1847.802040]  ? rcu_preempt_qs+0xa0/0xa0
> > > [ 1847.802433]  ? rcu_exp_wait_wake+0x630/0x630
> > > [ 1847.802872]  ? __lock_acquire+0xfb9/0x1120
> > > [ 1847.803302]  ? __lock_acquire+0x534/0x1120
> > > [ 1847.803725]  ? bdi_unregister+0x57/0x1a0
> > > [ 1847.804135]  bdi_unregister+0x5c/0x1a0
> > > [ 1847.804519]  bdi_put+0xcb/0xe0
> > > [ 1847.804746]  generic_shutdown_super+0xe2/0x110
> > > [ 1847.805066]  kill_anon_super+0xe/0x20
> > > [ 1847.805344]  btrfs_kill_super+0x12/0xa0 [btrfs]
> > > [ 1847.805664]  deactivate_locked_super+0x34/0x60
> > > [ 1847.806111]  btrfs_mount+0xbb6/0x1000 [btrfs]
> > > [ 1847.806476]  ? __lockdep_init_map+0x5c/0x1d0
> > > [ 1847.806824]  ? mount_fs+0xf/0x80
> > > [ 1847.807104]  ? alloc_vfsmnt+0x1a1/0x230
> > > [ 1847.807416]  mount_fs+0xf/0x80
> > > [ 1847.807712]  vfs_kern_mount+0x62/0x160
> > > [ 1847.808112]  btrfs_mount+0x3d3/0x1000 [btrfs]
> > > [ 1847.808565]  ? __lockdep_init_map+0x5c/0x1d0
> > > [ 1847.809005]  ? __lockdep_init_map+0x5c/0x1d0
> > > [ 1847.809425]  ? mount_fs+0xf/0x80
> > > [ 1847.809731]  mount_fs+0xf/0x80
> > > [ 1847.810070]  vfs_kern_mount+0x62/0x160
> > > [ 1847.810469]  do_mount+0x1b1/0xd50
> > > [ 1847.810821]  ? _copy_from_user+0x5b/0x90
> > > [ 1847.811237]  ? memdup_user+0x4b/0x70
> > > [ 1847.811622]  SyS_mount+0x85/0xd0
> > > [ 1847.811996]  entry_SYSCALL_64_fastpath+0x1f/0x96
> > > [ 1847.812465] RIP: 0033:0x7f6ebecc1b5a
> > > [ 1847.812840] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> > > [ 1847.813615] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> > > [ 1847.814302] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> > > [ 1847.814770] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> > > [ 1847.815246] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> > > [ 1847.815720] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
> > i> 
> > 
> > Looks like this is new, Masami this is happening because of your change here
> > 
> > 5bb4fc2d8641 ("kprobes/x86: Disable preemption in ftrace-based jprobes")
> > 
> > which makes it not do the preempt_enable() if the handler returns 1.  Why is
> > that?  Should I be doing preempt_enable_no_resched() from the handler before
> > returning 1?  Or is this just an oversight on your part?  Thanks,
> 
> FWIW I shut up the preemption imbalance warnings with the attached
> coarse bandaid.  No idea if that's the correct fix...
> 
> --D
> 
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 5db8498..fd948e3 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1215,8 +1215,10 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
>  		if (__this_cpu_read(bpf_kprobe_override)) {
>  			__this_cpu_write(bpf_kprobe_override, 0);
>  			reset_current_kprobe();
> +			preempt_enable();
>  			return 1;
>  		}
> +		preempt_enable();
>  		if (!ret)
>  			return 0;
>  	}

Yeah I'd like to avoid doing this and know why exactly we leave a unpaired
preempt_disable() in kprobe_ftrace_handler() so we don't do something like this
only to have the handler change again in the future and break us again.  Thanks,

Josef

Re: [PATCH v9 0/5] Add the ability to do BPF directed error injection

[Masami Hiramatsu]

On Wed, 13 Dec 2017 13:57:45 -0500
Josef Bacik <josef@toxicpanda.com> wrote:

> On Wed, Dec 13, 2017 at 10:07:32AM -0800, Darrick J. Wong wrote:
> > On Wed, Dec 13, 2017 at 01:03:57PM -0500, Josef Bacik wrote:
> > > On Tue, Dec 12, 2017 at 03:11:50PM -0800, Darrick J. Wong wrote:
> > > > On Mon, Dec 11, 2017 at 11:36:45AM -0500, Josef Bacik wrote:
> > > > > This is the same as v8, just rebased onto the bpf tree.
> > > > > 
> > > > > v8->v9:
> > > > > - rebased onto the bpf tree.
> > > > > 
> > > > > v7->v8:
> > > > > - removed the _ASM_KPROBE_ERROR_INJECT since it was not needed.
> > > > > 
> > > > > v6->v7:
> > > > > - moved the opt-in macro to bpf.h out of kprobes.h.
> > > > > 
> > > > > v5->v6:
> > > > > - add BPF_ALLOW_ERROR_INJECTION() tagging for functions that will support this
> > > > >   feature.  This way only functions that opt-in will be allowed to be
> > > > >   overridden.
> > > > > - added a btrfs patch to allow error injection for open_ctree() so that the bpf
> > > > >   sample actually works.
> > > > > 
> > > > > v4->v5:
> > > > > - disallow kprobe_override programs from being put in the prog map array so we
> > > > >   don't tail call into something we didn't check.  This allows us to make the
> > > > >   normal path still fast without a bunch of percpu operations.
> > > > > 
> > > > > v3->v4:
> > > > > - fix a build error found by kbuild test bot (I didn't wait long enough
> > > > >   apparently.)
> > > > > - Added a warning message as per Daniels suggestion.
> > > > > 
> > > > > v2->v3:
> > > > > - added a ->kprobe_override flag to bpf_prog.
> > > > > - added some sanity checks to disallow attaching bpf progs that have
> > > > >   ->kprobe_override set that aren't for ftrace kprobes.
> > > > > - added the trace_kprobe_ftrace helper to check if the trace_event_call is a
> > > > >   ftrace kprobe.
> > > > > - renamed bpf_kprobe_state to bpf_kprobe_override, fixed it so we only read this
> > > > >   value in the kprobe path, and thus only write to it if we're overriding or
> > > > >   clearing the override.
> > > > > 
> > > > > v1->v2:
> > > > > - moved things around to make sure that bpf_override_return could really only be
> > > > >   used for an ftrace kprobe.
> > > > > - killed the special return values from trace_call_bpf.
> > > > > - renamed pc_modified to bpf_kprobe_state so bpf_override_return could tell if
> > > > >   it was being called from an ftrace kprobe context.
> > > > > - reworked the logic in kprobe_perf_func to take advantage of bpf_kprobe_state.
> > > > > - updated the test as per Alexei's review.
> > > > > 
> > > > > - Original message -
> > > > > 
> > > > > A lot of our error paths are not well tested because we have no good way of
> > > > > injecting errors generically.  Some subystems (block, memory) have ways to
> > > > > inject errors, but they are random so it's hard to get reproduceable results.
> > > > > 
> > > > > With BPF we can add determinism to our error injection.  We can use kprobes and
> > > > > other things to verify we are injecting errors at the exact case we are trying
> > > > > to test.  This patch gives us the tool to actual do the error injection part.
> > > > > It is very simple, we just set the return value of the pt_regs we're given to
> > > > > whatever we provide, and then override the PC with a dummy function that simply
> > > > > returns.
> > > > 
> > > > Heh, this looks cool.  I decided to try it to see what happens, and saw
> > > > a bunch of dmesg pasted in below.  Is that supposed to happen?  Or am I
> > > > the only fs developer still running with lockdep enabled? :)
> > > > 
> > > > It looks like bpf_override_return has some sort of side effect such that
> > > > we get the splat, since commenting it out makes the symptom go away.
> > > > 
> > > > <shrug>
> > > > 
> > > > --D
> > > > 
> > > > [ 1847.769183] BTRFS error (device (null)): open_ctree failed
> > > > [ 1847.770130] BUG: sleeping function called from invalid context at /storage/home/djwong/cdev/work/linux-xfs/kernel/locking/rwsem.c:69
> > > > [ 1847.771976] in_atomic(): 1, irqs_disabled(): 0, pid: 1524, name: mount
> > > > [ 1847.773016] 1 lock held by mount/1524:
> > > > [ 1847.773530]  #0:  (&type->s_umount_key#34/1){+.+.}, at: [<00000000653a9bb4>] sget_userns+0x302/0x4f0
> > > > [ 1847.774731] Preemption disabled at:
> > > > [ 1847.774735] [<          (null)>]           (null)
> > > > [ 1847.777009] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> > > > [ 1847.778800] Call Trace:
> > > > [ 1847.779047]  dump_stack+0x7c/0xbe
> > > > [ 1847.779361]  ___might_sleep+0x1f7/0x260
> > > > [ 1847.779720]  down_write+0x29/0xb0
> > > > [ 1847.780046]  unregister_shrinker+0x15/0x70
> > > > [ 1847.780427]  deactivate_locked_super+0x2e/0x60
> > > > [ 1847.780935]  btrfs_mount+0xbb6/0x1000 [btrfs]
> > > > [ 1847.781353]  ? __lockdep_init_map+0x5c/0x1d0
> > > > [ 1847.781750]  ? mount_fs+0xf/0x80
> > > > [ 1847.782065]  ? alloc_vfsmnt+0x1a1/0x230
> > > > [ 1847.782429]  mount_fs+0xf/0x80
> > > > [ 1847.782733]  vfs_kern_mount+0x62/0x160
> > > > [ 1847.783128]  btrfs_mount+0x3d3/0x1000 [btrfs]
> > > > [ 1847.783493]  ? __lockdep_init_map+0x5c/0x1d0
> > > > [ 1847.783849]  ? __lockdep_init_map+0x5c/0x1d0
> > > > [ 1847.784207]  ? mount_fs+0xf/0x80
> > > > [ 1847.784502]  mount_fs+0xf/0x80
> > > > [ 1847.784835]  vfs_kern_mount+0x62/0x160
> > > > [ 1847.785235]  do_mount+0x1b1/0xd50
> > > > [ 1847.785594]  ? _copy_from_user+0x5b/0x90
> > > > [ 1847.786028]  ? memdup_user+0x4b/0x70
> > > > [ 1847.786501]  SyS_mount+0x85/0xd0
> > > > [ 1847.786835]  entry_SYSCALL_64_fastpath+0x1f/0x96
> > > > [ 1847.787311] RIP: 0033:0x7f6ebecc1b5a
> > > > [ 1847.787691] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> > > > [ 1847.788383] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> > > > [ 1847.789106] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> > > > [ 1847.789807] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> > > > [ 1847.790511] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> > > > [ 1847.791211] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
> > > > [ 1847.792029] BUG: scheduling while atomic: mount/1524/0x00000002
> > > > [ 1847.792680] 1 lock held by mount/1524:
> > > > [ 1847.793087]  #0:  (rcu_preempt_state.exp_mutex){+.+.}, at: [<00000000a6c536a9>] _synchronize_rcu_expedited+0x1ce/0x400
> > > > [ 1847.794129] Modules linked in: xfs libcrc32c btrfs xor zstd_decompress zstd_compress xxhash lzo_compress lzo_decompress zlib_deflate raid6_pq dax_pmem device_dax nd_pmem sch_fq_codel af_packet [last unloaded: xfs]
> > > > [ 1847.795949] Preemption disabled at:
> > > > [ 1847.795951] [<          (null)>]           (null)
> > > > [ 1847.796844] CPU: 2 PID: 1524 Comm: mount Tainted: G        W        4.15.0-rc3-xfsx #3
> > > > [ 1847.797621] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1djwong0 04/01/2014
> > > > [ 1847.798510] Call Trace:
> > > > [ 1847.798786]  dump_stack+0x7c/0xbe
> > > > [ 1847.799134]  __schedule_bug+0x88/0xe0
> > > > [ 1847.799517]  __schedule+0x78c/0xb20
> > > > [ 1847.799890]  ? trace_hardirqs_on_caller+0x119/0x180
> > > > [ 1847.800391]  schedule+0x40/0x90
> > > > [ 1847.800729]  _synchronize_rcu_expedited+0x36b/0x400
> > > > [ 1847.801218]  ? rcu_preempt_qs+0xa0/0xa0
> > > > [ 1847.801616]  ? remove_wait_queue+0x60/0x60
> > > > [ 1847.802040]  ? rcu_preempt_qs+0xa0/0xa0
> > > > [ 1847.802433]  ? rcu_exp_wait_wake+0x630/0x630
> > > > [ 1847.802872]  ? __lock_acquire+0xfb9/0x1120
> > > > [ 1847.803302]  ? __lock_acquire+0x534/0x1120
> > > > [ 1847.803725]  ? bdi_unregister+0x57/0x1a0
> > > > [ 1847.804135]  bdi_unregister+0x5c/0x1a0
> > > > [ 1847.804519]  bdi_put+0xcb/0xe0
> > > > [ 1847.804746]  generic_shutdown_super+0xe2/0x110
> > > > [ 1847.805066]  kill_anon_super+0xe/0x20
> > > > [ 1847.805344]  btrfs_kill_super+0x12/0xa0 [btrfs]
> > > > [ 1847.805664]  deactivate_locked_super+0x34/0x60
> > > > [ 1847.806111]  btrfs_mount+0xbb6/0x1000 [btrfs]
> > > > [ 1847.806476]  ? __lockdep_init_map+0x5c/0x1d0
> > > > [ 1847.806824]  ? mount_fs+0xf/0x80
> > > > [ 1847.807104]  ? alloc_vfsmnt+0x1a1/0x230
> > > > [ 1847.807416]  mount_fs+0xf/0x80
> > > > [ 1847.807712]  vfs_kern_mount+0x62/0x160
> > > > [ 1847.808112]  btrfs_mount+0x3d3/0x1000 [btrfs]
> > > > [ 1847.808565]  ? __lockdep_init_map+0x5c/0x1d0
> > > > [ 1847.809005]  ? __lockdep_init_map+0x5c/0x1d0
> > > > [ 1847.809425]  ? mount_fs+0xf/0x80
> > > > [ 1847.809731]  mount_fs+0xf/0x80
> > > > [ 1847.810070]  vfs_kern_mount+0x62/0x160
> > > > [ 1847.810469]  do_mount+0x1b1/0xd50
> > > > [ 1847.810821]  ? _copy_from_user+0x5b/0x90
> > > > [ 1847.811237]  ? memdup_user+0x4b/0x70
> > > > [ 1847.811622]  SyS_mount+0x85/0xd0
> > > > [ 1847.811996]  entry_SYSCALL_64_fastpath+0x1f/0x96
> > > > [ 1847.812465] RIP: 0033:0x7f6ebecc1b5a
> > > > [ 1847.812840] RSP: 002b:00007ffc7bd1c958 EFLAGS: 00000202 ORIG_RAX: 00000000000000a5
> > > > [ 1847.813615] RAX: ffffffffffffffda RBX: 00007f6ebefba63a RCX: 00007f6ebecc1b5a
> > > > [ 1847.814302] RDX: 0000000000bfd010 RSI: 0000000000bfa230 RDI: 0000000000bfa210
> > > > [ 1847.814770] RBP: 0000000000bfa0f0 R08: 0000000000000000 R09: 0000000000000014
> > > > [ 1847.815246] R10: 00000000c0ed0000 R11: 0000000000000202 R12: 00007f6ebf1ca83c
> > > > [ 1847.815720] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000001
> > > i> 
> > > 
> > > Looks like this is new, Masami this is happening because of your change here
> > > 
> > > 5bb4fc2d8641 ("kprobes/x86: Disable preemption in ftrace-based jprobes")
> > > 
> > > which makes it not do the preempt_enable() if the handler returns 1.  Why is
> > > that?

Yes, because this (return 1) is expected to be done only by jprobe.

> > >  Should I be doing preempt_enable_no_resched() from the handler before
> > > returning 1? Or is this just an oversight on your part?  Thanks,

Yes, or you have to hook after return path and fixup preempt count as
jprobe did.

(And now jprobe is coming to an end.)

> > 
> > FWIW I shut up the preemption imbalance warnings with the attached
> > coarse bandaid.  No idea if that's the correct fix...

No, this is not correct way to fix this issue.
I guess your BPF extention is trying to change instrunction pointer to
another address (right?). If so, you have to carefully do followings
before returning to modified address.

- reset current_kprobes
- call preempt_enable_no_resched()


> > 
> > --D
> > 
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 5db8498..fd948e3 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -1215,8 +1215,10 @@ kprobe_perf_func(struct trace_kprobe *tk, struct pt_regs *regs)
> >  		if (__this_cpu_read(bpf_kprobe_override)) {
> >  			__this_cpu_write(bpf_kprobe_override, 0);
> >  			reset_current_kprobe();
> > +			preempt_enable();
> >  			return 1;
> >  		}
> > +		preempt_enable();
> >  		if (!ret)
> >  			return 0;
> >  	}
> 
> Yeah I'd like to avoid doing this and know why exactly we leave a unpaired
> preempt_disable() in kprobe_ftrace_handler() so we don't do something like this
> only to have the handler change again in the future and break us again.  Thanks,

Ah, I see. kprobe_perf_func invokes BPF and BPF changes instruction address.
In that case, only what you need is adding preempt_enable_no_resched() at
right after the reset_current_kprobe().

Anyway, Could you CC the series to me?

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>