Re: [PATCH v10 2/9] powerpc/code-patching: Use WARN_ON and fix check in poking_init

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 09/11/2022 à 05:51, Benjamin Gray a écrit :
> BUG_ON() when failing to initialise the code patching window is
> unnecessary, and use of BUG_ON is discouraged. We don't set
> poking_init_done in this case, so failure to init the boot CPU will
> result in a strict RWX error when a following patch_instruction uses
> raw_patch_instruction. If it only fails for later CPUs, they won't be
> onlined in the first place.
> 
> The return value of cpuhp_setup_state() is also >= 0 on success,
> so check for < 0.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
> v10:	* Don't mention caller recovery
> 	* Don't set poking_init_done on failure
> 	* Add setup return code comment from later patch
> 	* Reviewed-by not applied because of above changes
> v9:	* Reword commit message to explain why init failure is not fatal
> ---
>   arch/powerpc/lib/code-patching.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index ad0cf3108dd0..3055eef7dcdc 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -81,16 +81,17 @@ static int text_area_cpu_down(unsigned int cpu)
>   
>   static __ro_after_init DEFINE_STATIC_KEY_FALSE(poking_init_done);
>   
> -/*
> - * Although BUG_ON() is rude, in this case it should only happen if ENOMEM, and
> - * we judge it as being preferable to a kernel that will crash later when
> - * someone tries to use patch_instruction().
> - */
>   void __init poking_init(void)
>   {
> -	BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> -		"powerpc/text_poke:online", text_area_cpu_up,
> -		text_area_cpu_down));
> +	int ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				    "powerpc/text_poke:online",
> +				    text_area_cpu_up,
> +				    text_area_cpu_down);
> +
> +	/* cpuhp_setup_state returns >= 0 on success */
> +	if (WARN_ON(ret < 0))
> +		return;
> +
>   	static_branch_enable(&poking_init_done);
>   }
>