Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752565AbeAJWaA (ORCPT + 1 other); Wed, 10 Jan 2018 17:30:00 -0500 Received: from mail.kernel.org ([198.145.29.99]:36522 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752088AbeAJW36 (ORCPT ); Wed, 10 Jan 2018 17:29:58 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 179EE21742 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Thu, 11 Jan 2018 07:18:32 +0900 From: Masami Hiramatsu To: Josef Bacik Cc: mhiramat@kernel.org, Alexei Starovoitov , Josef Bacik , rostedt@goodmis.org, mingo@redhat.com, davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, ast@kernel.org, kernel-team@fb.com, daniel@iogearbox.net, linux-btrfs@vger.kernel.org, darrick.wong@oracle.com, Akinobu Mita Subject: Re: [PATCH bpf-next v3 3/5] error-injection: Separate error-injection from kprobe Message-Id: <20180111071832.a7bcfe95cf5a17e0fb03e02d@kernel.org> In-Reply-To: <20180110153614.3mbzmejnjlfw6lqj@destiny> References: <151557939382.6629.18074658376309258555.stgit@devbox> <151557948555.6629.8642887195554280720.stgit@devbox> <20180110153614.3mbzmejnjlfw6lqj@destiny> X-Mailer: Sylpheed 3.5.1 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Wed, 10 Jan 2018 10:36:15 -0500 Josef Bacik wrote: > On Wed, Jan 10, 2018 at 07:18:05PM +0900, Masami Hiramatsu wrote: > > Since error-injection framework is not limited to be used > > by kprobes, nor bpf. Other kernel subsystems can use it > > freely for checking safeness of error-injection, e.g. > > livepatch, ftrace etc. > > So this separate error-injection framework from kprobes. > > > > Some differences has been made: > > > > - "kprobe" word is removed from any APIs/structures. > > - BPF_ALLOW_ERROR_INJECTION() is renamed to > > ALLOW_ERROR_INJECTION() since it is not limited for BPF too. > > - CONFIG_FUNCTION_ERROR_INJECTION is the config item of this > > feature. It is automatically enabled if the arch supports > > error injection feature for kprobe or ftrace etc. > > > > Signed-off-by: Masami Hiramatsu > > --- > > Changes in v3: > > - Fix a build error for asmlinkage on i386 by including compiler.h > > - Fix "CONFIG_FUNCTION_ERROR_INJECT" typo. > > - Separate CONFIG_MODULES dependent code > > - Add CONFIG_KPROBES dependency for arch_deref_entry_point() > > - Call error-injection init function in late_initcall stage. > > - Fix read-side mutex lock > > - Some cosmetic cleanups > > --- > > arch/Kconfig | 2 > > arch/x86/Kconfig | 2 > > arch/x86/include/asm/error-injection.h | 13 ++ > > arch/x86/kernel/kprobes/core.c | 14 -- > > arch/x86/lib/Makefile | 1 > > arch/x86/lib/error-inject.c | 19 +++ > > fs/btrfs/disk-io.c | 2 > > fs/btrfs/free-space-cache.c | 2 > > include/asm-generic/error-injection.h | 20 +++ > > include/asm-generic/vmlinux.lds.h | 14 +- > > include/linux/bpf.h | 12 -- > > include/linux/error-injection.h | 21 +++ > > include/linux/kprobes.h | 1 > > include/linux/module.h | 6 - > > kernel/kprobes.c | 163 ------------------------ > > kernel/module.c | 8 + > > kernel/trace/Kconfig | 2 > > kernel/trace/bpf_trace.c | 2 > > kernel/trace/trace_kprobe.c | 3 > > lib/Kconfig.debug | 4 + > > lib/Makefile | 1 > > lib/error-inject.c | 213 ++++++++++++++++++++++++++++++++ > > 22 files changed, 315 insertions(+), 210 deletions(-) > > create mode 100644 arch/x86/include/asm/error-injection.h > > create mode 100644 arch/x86/lib/error-inject.c > > create mode 100644 include/asm-generic/error-injection.h > > create mode 100644 include/linux/error-injection.h > > create mode 100644 lib/error-inject.c > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index d3f4aaf9cb7a..97376accfb14 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -196,7 +196,7 @@ config HAVE_OPTPROBES > > config HAVE_KPROBES_ON_FTRACE > > bool > > > > -config HAVE_KPROBE_OVERRIDE > > +config HAVE_FUNCTION_ERROR_INJECTION > > bool > > > > config HAVE_NMI > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > index 45dc6233f2b9..366b19cb79b7 100644 > > --- a/arch/x86/Kconfig > > +++ b/arch/x86/Kconfig > > @@ -154,7 +154,7 @@ config X86 > > select HAVE_KERNEL_XZ > > select HAVE_KPROBES > > select HAVE_KPROBES_ON_FTRACE > > - select HAVE_KPROBE_OVERRIDE > > + select HAVE_FUNCTION_ERROR_INJECTION > > select HAVE_KRETPROBES > > select HAVE_KVM > > select HAVE_LIVEPATCH if X86_64 > > diff --git a/arch/x86/include/asm/error-injection.h b/arch/x86/include/asm/error-injection.h > > new file mode 100644 > > index 000000000000..47b7a1296245 > > --- /dev/null > > +++ b/arch/x86/include/asm/error-injection.h > > @@ -0,0 +1,13 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _ASM_ERROR_INJECTION_H > > +#define _ASM_ERROR_INJECTION_H > > + > > +#include > > +#include > > +#include > > +#include > > + > > +asmlinkage void just_return_func(void); > > +void override_function_with_return(struct pt_regs *regs); > > + > > +#endif /* _ASM_ERROR_INJECTION_H */ > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c > > index b02a377d5905..bd36f3c33cd0 100644 > > --- a/arch/x86/kernel/kprobes/core.c > > +++ b/arch/x86/kernel/kprobes/core.c > > @@ -1183,17 +1183,3 @@ int arch_trampoline_kprobe(struct kprobe *p) > > { > > 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_kprobe_override_function(struct pt_regs *regs) > > -{ > > - regs->ip = (unsigned long)&override_func; > > -} > > -NOKPROBE_SYMBOL(arch_kprobe_override_function); > > diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile > > index 7b181b61170e..171377b83be1 100644 > > --- a/arch/x86/lib/Makefile > > +++ b/arch/x86/lib/Makefile > > @@ -26,6 +26,7 @@ lib-y += memcpy_$(BITS).o > > lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o > > lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o insn-eval.o > > lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o > > +lib-$(CONFIG_FUNCTION_ERROR_INJECTION) += error-inject.o > > > > obj-y += msr.o msr-reg.o msr-reg-export.o hweight.o > > > > diff --git a/arch/x86/lib/error-inject.c b/arch/x86/lib/error-inject.c > > new file mode 100644 > > index 000000000000..7b881d03d0dd > > --- /dev/null > > +++ b/arch/x86/lib/error-inject.c > > @@ -0,0 +1,19 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#include > > +#include > > + > > +asmlinkage void just_return_func(void); > > + > > +asm( > > + ".type just_return_func, @function\n" > > + "just_return_func:\n" > > + " ret\n" > > + ".size just_return_func, .-just_return_func\n" > > +); > > + > > +void override_function_with_return(struct pt_regs *regs) > > +{ > > + regs->ip = (unsigned long)&just_return_func; > > +} > > +NOKPROBE_SYMBOL(override_function_with_return); > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index 5da18ebc9222..5c540129ad81 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -3124,7 +3124,7 @@ int open_ctree(struct super_block *sb, > > goto fail_block_groups; > > goto retry_root_backup; > > } > > -BPF_ALLOW_ERROR_INJECTION(open_ctree); > > +ALLOW_ERROR_INJECTION(open_ctree); > > > > static void btrfs_end_buffer_write_sync(struct buffer_head *bh, int uptodate) > > { > > diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c > > index fb1382893bfc..2a75e088b215 100644 > > --- a/fs/btrfs/free-space-cache.c > > +++ b/fs/btrfs/free-space-cache.c > > @@ -333,7 +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); > > +ALLOW_ERROR_INJECTION(io_ctl_init); > > > > static void io_ctl_free(struct btrfs_io_ctl *io_ctl) > > { > > diff --git a/include/asm-generic/error-injection.h b/include/asm-generic/error-injection.h > > new file mode 100644 > > index 000000000000..08352c9d9f97 > > --- /dev/null > > +++ b/include/asm-generic/error-injection.h > > @@ -0,0 +1,20 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _ASM_GENERIC_ERROR_INJECTION_H > > +#define _ASM_GENERIC_ERROR_INJECTION_H > > + > > +#if defined(__KERNEL__) && !defined(__ASSEMBLY__) > > +#ifdef CONFIG_FUNCTION_ERROR_INJECTION > > +/* > > + * Whitelist ganerating macro. Specify functions which can be > > + * error-injectable using this macro. > > + */ > > +#define ALLOW_ERROR_INJECTION(fname) \ > > +static unsigned long __used \ > > + __attribute__((__section__("_error_injection_whitelist"))) \ > > + _eil_addr_##fname = (unsigned long)fname; > > +#else > > +#define ALLOW_ERROR_INJECTION(fname) > > +#endif > > +#endif > > + > > +#endif /* _ASM_GENERIC_ERROR_INJECTION_H */ > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > > index a2e8582d094a..f2068cca5206 100644 > > --- a/include/asm-generic/vmlinux.lds.h > > +++ b/include/asm-generic/vmlinux.lds.h > > @@ -136,13 +136,13 @@ > > #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) = .; > > +#ifdef CONFIG_FUNCTION_ERROR_INJECTION > > +#define ERROR_INJECT_WHITELIST() . = ALIGN(8); \ > > + VMLINUX_SYMBOL(__start_error_injection_whitelist) = .;\ > > + KEEP(*(_error_injection_whitelist)) \ > > + VMLINUX_SYMBOL(__stop_error_injection_whitelist) = .; > > #else > > -#define ERROR_INJECT_LIST() > > +#define ERROR_INJECT_WHITELIST() > > #endif > > > > #ifdef CONFIG_EVENT_TRACING > > @@ -573,7 +573,7 @@ > > FTRACE_EVENTS() \ > > TRACE_SYSCALLS() \ > > KPROBE_BLACKLIST() \ > > - ERROR_INJECT_LIST() \ > > + ERROR_INJECT_WHITELIST() \ > > MEM_DISCARD(init.rodata) \ > > CLK_OF_TABLES() \ > > RESERVEDMEM_OF_TABLES() \ > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index 9e03046d1df2..ea865bb9f676 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -16,6 +16,7 @@ > > #include > > #include > > #include > > +#include > > > > I assume you did this because we include linux/bpf.h for the > BPF_ALLOW_ERROR_INJECTION() stuff in btrfs. Can we just drop this include here, > and change the users of ALLOW_ERROR_INJECTION() to include error-injection.h > instead? Indeed, since the macro is no longer have BPF_* prefix, user source file should include the definition header directly. Thank you, > > > > > +/* > > + * error_injection/whitelist -- shows which functions can be overridden for > > + * error injection. > > + */ > > +static void *ei_seq_start(struct seq_file *m, loff_t *pos) > > +{ > > + mutex_lock(&ei_mutex); > > + return seq_list_start(&error_injection_list, *pos); > > +} > > + > > +static void ei_seq_stop(struct seq_file *m, void *v) > > +{ > > + mutex_unlock(&ei_mutex); > > +} > > + > > +static void *ei_seq_next(struct seq_file *m, void *v, loff_t *pos) > > +{ > > + return seq_list_next(v, &error_injection_list, pos); > > +} > > + > > +static int ei_seq_show(struct seq_file *m, void *v) > > +{ > > + struct ei_entry *ent = list_entry(v, struct ei_entry, list); > > + > > + seq_printf(m, "%pf\n", (void *)ent->start_addr); > > Can we bring back the sprint_symbol() thing I did originally here so it's nice > and easy to sanity check stuff is working? Thanks > > Josef -- Masami Hiramatsu