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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 2 Jun 2024 23:19:50 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Akinobu Mita <akinobu.mita@...il.com>, Christoph Lameter <cl@...ux.com>,
 David Rientjes <rientjes@...gle.com>, Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko
 <andrii@...nel.org>, "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>, Anil S
 Keshavamurthy <anil.s.keshavamurthy@...el.com>, "David S. Miller"
 <davem@...emloft.net>, Steven Rostedt <rostedt@...dmis.org>, Mark Rutland
 <mark.rutland@....com>, Jiri Olsa <jolsa@...nel.org>, Roman Gushchin
 <roman.gushchin@...ux.dev>, Hyeonggon Yoo <42.hyeyoo@...il.com>,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org, bpf@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 2/4] error-injection: support static keys around
 injectable functions

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