Re: [PATCH RFC 2/4] error-injection: support static keys around injectable functions

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 31 May 2024 11:33:33 +0200
Vlastimil Babka <vbabka@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 <vbabka@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) <mhiramat@kernel.org>