Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934316AbeAJPi2 (ORCPT + 1 other); Wed, 10 Jan 2018 10:38:28 -0500 Received: from mail-qt0-f194.google.com ([209.85.216.194]:37190 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753825AbeAJPiZ (ORCPT ); Wed, 10 Jan 2018 10:38:25 -0500 X-Google-Smtp-Source: ACJfBovy04rCo0PCxm3g239d1crFVW+9pDqVHg8hyaNDWL/JVEw7igedt1FJXoxwjHzmoMY9ccCOkw== Date: Wed, 10 Jan 2018 10:38:23 -0500 From: Josef Bacik To: Masami Hiramatsu Cc: 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, Josef Bacik , Akinobu Mita Subject: Re: [PATCH bpf-next v3 4/5] error-injection: Add injectable error types Message-ID: <20180110153822.qbqpnkoxy6key25q@destiny> References: <151557939382.6629.18074658376309258555.stgit@devbox> <151557951552.6629.9986988497471849240.stgit@devbox> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <151557951552.6629.9986988497471849240.stgit@devbox> User-Agent: NeoMutt/20170714 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Wed, Jan 10, 2018 at 07:18:35PM +0900, Masami Hiramatsu wrote: > Add injectable error types for each error-injectable function. > > One motivation of error injection test is to find software flaws, > mistakes or mis-handlings of expectable errors. If we find such > flaws by the test, that is a program bug, so we need to fix it. > > But if the tester miss input the error (e.g. just return success > code without processing anything), it causes unexpected behavior > even if the caller is correctly programmed to handle any errors. > That is not what we want to test by error injection. > > To clarify what type of errors the caller must expect for each > injectable function, this introduces injectable error types: > > - EI_ETYPE_NULL : means the function will return NULL if it > fails. No ERR_PTR, just a NULL. > - EI_ETYPE_ERRNO : means the function will return -ERRNO > if it fails. > - EI_ETYPE_ERRNO_NULL : means the function will return -ERRNO > (ERR_PTR) or NULL. > > ALLOW_ERROR_INJECTION() macro is expanded to get one of > NULL, ERRNO, ERRNO_NULL to record the error type for > each function. e.g. > > ALLOW_ERROR_INJECTION(open_ctree, ERRNO) > > This error types are shown in debugfs as below. > > ==== > / # cat /sys/kernel/debug/error_injection/list > open_ctree [btrfs] ERRNO > io_ctl_init [btrfs] ERRNO > ==== > > Signed-off-by: Masami Hiramatsu > --- > fs/btrfs/disk-io.c | 2 +- > fs/btrfs/free-space-cache.c | 2 +- > include/asm-generic/error-injection.h | 23 +++++++++++++++--- > include/asm-generic/vmlinux.lds.h | 2 +- > include/linux/error-injection.h | 6 +++++ > include/linux/module.h | 3 ++ > lib/error-inject.c | 43 ++++++++++++++++++++++++++++----- > 7 files changed, 66 insertions(+), 15 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 5c540129ad81..9269fc23f490 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; > } > -ALLOW_ERROR_INJECTION(open_ctree); > +ALLOW_ERROR_INJECTION(open_ctree, ERRNO); > > 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 2a75e088b215..9eb45f77e381 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; > } > -ALLOW_ERROR_INJECTION(io_ctl_init); > +ALLOW_ERROR_INJECTION(io_ctl_init, ERRNO); > > 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 > index 08352c9d9f97..296c65442f00 100644 > --- a/include/asm-generic/error-injection.h > +++ b/include/asm-generic/error-injection.h > @@ -3,17 +3,32 @@ > #define _ASM_GENERIC_ERROR_INJECTION_H > > #if defined(__KERNEL__) && !defined(__ASSEMBLY__) > +enum { > + EI_ETYPE_NONE, /* Dummy value for undefined case */ > + EI_ETYPE_NULL, /* Return NULL if failure */ > + EI_ETYPE_ERRNO, /* Return -ERRNO if failure */ > + EI_ETYPE_ERRNO_NULL, /* Return -ERRNO or NULL if failure */ > +}; > + > +struct error_injection_entry { > + unsigned long addr; > + int etype; > +}; > + > #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 \ > +#define ALLOW_ERROR_INJECTION(fname, _etype) \ > +static struct error_injection_entry __used \ > __attribute__((__section__("_error_injection_whitelist"))) \ > - _eil_addr_##fname = (unsigned long)fname; > + _eil_addr_##fname = { \ > + .addr = (unsigned long)fname, \ > + .etype = EI_ETYPE_##_etype, \ > + }; > #else > -#define ALLOW_ERROR_INJECTION(fname) > +#define ALLOW_ERROR_INJECTION(fname, _etype) > #endif > #endif > > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h > index f2068cca5206..ebe544e048cd 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -137,7 +137,7 @@ > #endif > > #ifdef CONFIG_FUNCTION_ERROR_INJECTION > -#define ERROR_INJECT_WHITELIST() . = ALIGN(8); \ > +#define ERROR_INJECT_WHITELIST() STRUCT_ALIGN(); \ > VMLINUX_SYMBOL(__start_error_injection_whitelist) = .;\ > KEEP(*(_error_injection_whitelist)) \ > VMLINUX_SYMBOL(__stop_error_injection_whitelist) = .; > diff --git a/include/linux/error-injection.h b/include/linux/error-injection.h > index 130a67c50dac..280c61ecbf20 100644 > --- a/include/linux/error-injection.h > +++ b/include/linux/error-injection.h > @@ -7,6 +7,7 @@ > #include > > extern bool within_error_injection_list(unsigned long addr); > +extern int get_injectable_error_type(unsigned long addr); > > #else /* !CONFIG_FUNCTION_ERROR_INJECTION */ > > @@ -16,6 +17,11 @@ static inline bool within_error_injection_list(unsigned long addr) > return false; > } > > +static inline int get_injectable_error_type(unsigned long addr) > +{ > + return EI_ETYPE_NONE; > +} > + > #endif > > #endif /* _LINUX_ERROR_INJECTION_H */ > diff --git a/include/linux/module.h b/include/linux/module.h > index 792e51d83bda..9642d3116718 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -477,8 +478,8 @@ struct module { > #endif > > #ifdef CONFIG_FUNCTION_ERROR_INJECTION > + struct error_injection_entry *ei_funcs; > unsigned int num_ei_funcs; > - unsigned long *ei_funcs; > #endif > } ____cacheline_aligned __randomize_layout; > #ifndef MODULE_ARCH_INIT > diff --git a/lib/error-inject.c b/lib/error-inject.c > index ac9a371af719..1c303881ba5c 100644 > --- a/lib/error-inject.c > +++ b/lib/error-inject.c > @@ -16,6 +16,7 @@ struct ei_entry { > struct list_head list; > unsigned long start_addr; > unsigned long end_addr; > + int etype; > void *priv; > }; > > @@ -35,6 +36,17 @@ bool within_error_injection_list(unsigned long addr) > return false; > } > > +int get_injectable_error_type(unsigned long addr) > +{ > + struct ei_entry *ent; > + > + list_for_each_entry(ent, &error_injection_list, list) { > + if (addr >= ent->start_addr && addr < ent->end_addr) > + return ent->etype; > + } > + return EI_ETYPE_NONE; > +} > + > /* > * Lookup and populate the error_injection_list. > * > @@ -42,16 +54,17 @@ bool within_error_injection_list(unsigned long addr) > * bpf_error_injection, so we need to populate the list of the symbols that have > * been marked as safe for overriding. > */ > -static void populate_error_injection_list(unsigned long *start, > - unsigned long *end, void *priv) > +static void populate_error_injection_list(struct error_injection_entry *start, > + struct error_injection_entry *end, > + void *priv) > { > - unsigned long *iter; > + struct error_injection_entry *iter; > struct ei_entry *ent; > unsigned long entry, offset = 0, size = 0; > > mutex_lock(&ei_mutex); > for (iter = start; iter < end; iter++) { > - entry = arch_deref_entry_point((void *)*iter); > + entry = arch_deref_entry_point((void *)iter->addr); > > if (!kernel_text_address(entry) || > !kallsyms_lookup_size_offset(entry, &size, &offset)) { > @@ -65,6 +78,7 @@ static void populate_error_injection_list(unsigned long *start, > break; > ent->start_addr = entry; > ent->end_addr = entry + size; > + ent->etype = iter->etype; > ent->priv = priv; > INIT_LIST_HEAD(&ent->list); > list_add_tail(&ent->list, &error_injection_list); > @@ -73,8 +87,8 @@ static void populate_error_injection_list(unsigned long *start, > } > > /* Markers of the _error_inject_whitelist section */ > -extern unsigned long __start_error_injection_whitelist[]; > -extern unsigned long __stop_error_injection_whitelist[]; > +extern struct error_injection_entry __start_error_injection_whitelist[]; > +extern struct error_injection_entry __stop_error_injection_whitelist[]; > > static void __init populate_kernel_ei_list(void) > { > @@ -157,11 +171,26 @@ static void *ei_seq_next(struct seq_file *m, void *v, loff_t *pos) > return seq_list_next(v, &error_injection_list, pos); > } > > +static const char *error_type_string(int etype) > +{ > + switch (etype) { > + case EI_ETYPE_NULL: > + return "NULL"; > + case EI_ETYPE_ERRNO: > + return "ERRNO"; > + case EI_ETYPE_ERRNO_NULL: > + return "ERRNO_NULL"; > + default: > + return "(unknown)"; > + } > +} > + > 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); > + seq_printf(m, "%pf\t%s\n", (void *)ent->start_addr, > + error_type_string(ent->etype)); > return 0; > } Lol ignore the comment in my previous review about this part. Also I love this, Reviewed-by: Josef Bacik Thanks, Josef