On Fri, 31 May 2024 11:33:33 +0200
Vlastimil Babka <vba...@suse.cz> wrote:
> Error injectable functions cannot be inlined and since some are called
> from hot paths, this incurrs overhead even if no error injection is
> enabled for them.
>
> To remove this overhead when disabled, allow the callsites of error
> injectable functions to put the calls behind a static key, which the
> framework can control when error injection is enabled or disabled for
> the function.
>
> Introduce a new ALLOW_ERROR_INJECTION_KEY() macro that adds a parameter
> with the static key's address, and store it in struct
> error_injection_entry. This new field has caused a mismatch when
> populating the injection list from the _error_injection_whitelist
> section with the current STRUCT_ALIGN(), so change the alignment to 8.
>
> During the population, copy the key's address also to struct ei_entry,
> and make it possible to retrieve it along with the error type by
> get_injectable_error_type().
>
> Finally, make the processing of writes to the debugfs inject file enable
> the static key when the function is added to the injection list, and
> disable when removed.
>
> Signed-off-by: Vlastimil Babka <vba...@suse.cz>
> ---
> include/asm-generic/error-injection.h | 13 ++++++++++++-
> include/asm-generic/vmlinux.lds.h | 2 +-
> include/linux/error-injection.h | 9 ++++++---
> kernel/fail_function.c | 22 +++++++++++++++++++---
> lib/error-inject.c | 6 +++++-
> 5 files changed, 43 insertions(+), 9 deletions(-)
>
> diff --git a/include/asm-generic/error-injection.h
> b/include/asm-generic/error-injection.h
> index b05253f68eaa..eed2731f3820 100644
> --- a/include/asm-generic/error-injection.h
> +++ b/include/asm-generic/error-injection.h
> @@ -12,6 +12,7 @@ enum {
>
> struct error_injection_entry {
> unsigned long addr;
> + unsigned long static_key_addr;
> int etype;
> };
>
> @@ -25,16 +26,26 @@ struct pt_regs;
> * 'Error Injectable Functions' section.
> */
> #define ALLOW_ERROR_INJECTION(fname, _etype) \
> -static struct error_injection_entry __used \
> +static struct error_injection_entry __used __aligned(8)
> \
> __section("_error_injection_whitelist") \
> _eil_addr_##fname = { \
> .addr = (unsigned long)fname, \
> .etype = EI_ETYPE_##_etype, \
> }
>
> +#define ALLOW_ERROR_INJECTION_KEY(fname, _etype, key)
> \
> +static struct error_injection_entry __used __aligned(8)
> \
> + __section("_error_injection_whitelist") \
> + _eil_addr_##fname = { \
> + .addr = (unsigned long)fname, \
> + .static_key_addr = (unsigned long)key, \
> + .etype = EI_ETYPE_##_etype, \
> + }
> +
> void override_function_with_return(struct pt_regs *regs);
> #else
> #define ALLOW_ERROR_INJECTION(fname, _etype)
> +#define ALLOW_ERROR_INJECTION_KEY(fname, _etype, key)
>
> static inline void override_function_with_return(struct pt_regs *regs) { }
> #endif
> diff --git a/include/asm-generic/vmlinux.lds.h
> b/include/asm-generic/vmlinux.lds.h
> index 5703526d6ebf..1b15a0af2a00 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -248,7 +248,7 @@
>
> #ifdef CONFIG_FUNCTION_ERROR_INJECTION
> #define ERROR_INJECT_WHITELIST() \
> - STRUCT_ALIGN(); \
> + . = ALIGN(8); \
> BOUNDED_SECTION(_error_injection_whitelist)
> #else
> #define ERROR_INJECT_WHITELIST()
> diff --git a/include/linux/error-injection.h b/include/linux/error-injection.h
> index 20e738f4eae8..bec81b57a9d5 100644
> --- a/include/linux/error-injection.h
> +++ b/include/linux/error-injection.h
> @@ -6,10 +6,12 @@
> #include <linux/errno.h>
> #include <asm-generic/error-injection.h>
>
> +struct static_key;
> +
> #ifdef CONFIG_FUNCTION_ERROR_INJECTION
>
> -extern bool within_error_injection_list(unsigned long addr);
> -extern int get_injectable_error_type(unsigned long addr);
> +bool within_error_injection_list(unsigned long addr);
> +int get_injectable_error_type(unsigned long addr, struct static_key
> **key_addr);
This seems like an add-hoc change. Since this is called in a cold path
(only used when adding new function), can you add new
`struct static_key *get_injection_key(unsigned long addr)`
to find the static_key from the address?
Other part looks good to me.
Thank you,
--
Masami Hiramatsu (Google) <mhira...@kernel.org>
