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 <mhira...@kernel.org>
> ---
> 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 <asm/error-injection.h>
>
> 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 <linux/jump_label.h>
> #include <linux/export.h>
> #include <linux/rbtree_latch.h>
> +#include <linux/error-injection.h>
>
> #include <linux/percpu.h>
> #include <asm/module.h>
> @@ -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 <jba...@fb.com>
Thanks,
Josef
