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

From: Masami Hiramatsu
Date: Wed Jan 10 2018 - 17:30:03 EST

Next message: Enric Balletbo i Serra: "[PATCH 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye"
Previous message: Tejun Heo: "Re: cgroups(7): documenting cgroups v2 thread mode"
In reply to: Josef Bacik: "Re: [PATCH bpf-next v3 3/5] error-injection: Separate error-injection from kprobe"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 10 Jan 2018 10:36:15 -0500
Josef Bacik <josef@xxxxxxxxxxxxxx> 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 <mhiramat@xxxxxxxxxx>
> > ---
> > 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?

Indeed, since the macro is no longer have BPF_* prefix, user source
file should include the definition header directly.

Thank you,

>
> <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

--
Masami Hiramatsu <mhiramat@xxxxxxxxxx>

Next message: Enric Balletbo i Serra: "[PATCH 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye"
Previous message: Tejun Heo: "Re: cgroups(7): documenting cgroups v2 thread mode"
In reply to: Josef Bacik: "Re: [PATCH bpf-next v3 3/5] error-injection: Separate error-injection from kprobe"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]