Re: [PATCH bpf-next v3 3/5] error-injection: Separate error-injection from kprobe

[Josef Bacik <josef@toxicpanda.com>]

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 <mhiramat@kernel.org>
> ---
>   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 <linux/compiler.h>
> +#include <linux/linkage.h>
> +#include <asm/ptrace.h>
> +#include <asm-generic/error-injection.h>
> +
> +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 <linux/error-injection.h>
> +#include <linux/kprobes.h>
> +
> +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 <linux/rbtree_latch.h>
>  #include <linux/numa.h>
>  #include <linux/wait.h>
> +#include <linux/error-injection.h>
>  

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?

<snip>

> +/*
> + * 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