diff mbox series

[v9,3/7] powerpc/code-patching: Use WARN_ON and fix check in poking_init

Message ID 20221025044409.448755-4-bgray@linux.ibm.com (mailing list archive)
State Superseded
Headers show
Series Use per-CPU temporary mappings for patching on Radix MMU | expand

Commit Message

Benjamin Gray Oct. 25, 2022, 4:44 a.m. UTC 
BUG_ON() when failing to initialise the code patching window is
excessive, as most critical patching happens during boot before strict
RWX control is enabled. Failure to patch after boot is not inherently
fatal, so aborting the kernel is better determined by the caller.

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: Russell Currey <ruscur@russell.cc>
---
v9:	* Reword commit message to explain why init failure is not fatal
---
 arch/powerpc/lib/code-patching.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Christophe Leroy Nov. 2, 2022, 9:38 a.m. UTC | #1 
Le 25/10/2022 à 06:44, Benjamin Gray a écrit :
> BUG_ON() when failing to initialise the code patching window is
> excessive, as most critical patching happens during boot before strict
> RWX control is enabled. Failure to patch after boot is not inherently
> fatal, so aborting the kernel is better determined by the caller.
> 
> 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: Russell Currey <ruscur@russell.cc>
> ---
> v9:	* Reword commit message to explain why init failure is not fatal
> ---
>   arch/powerpc/lib/code-patching.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 54e145247643..3b3b09d5d2e1 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -82,16 +82,13 @@ 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));
> +	WARN_ON(cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				  "powerpc/text_poke:online",
> +				  text_area_cpu_up,
> +				  text_area_cpu_down) < 0);
> +
>   	static_branch_enable(&poking_init_done);

Wouldn't it be better to not enable the poking_init_done branch if the 
allocation failed ?

>   }
>
Benjamin Gray Nov. 2, 2022, 10:42 p.m. UTC | #2 
On Wed, 2022-11-02 at 09:38 +0000, Christophe Leroy wrote:
> Le 25/10/2022 à 06:44, Benjamin Gray a écrit :
> > diff --git a/arch/powerpc/lib/code-patching.c
> > b/arch/powerpc/lib/code-patching.c
> > index 54e145247643..3b3b09d5d2e1 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -82,16 +82,13 @@ 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));
> > +       WARN_ON(cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > +                                 "powerpc/text_poke:online",
> > +                                 text_area_cpu_up,
> > +                                 text_area_cpu_down) < 0);
> > +
> >         static_branch_enable(&poking_init_done);
> 
> Wouldn't it be better to not enable the poking_init_done branch if
> the 
> allocation failed ?

Yeah that probably works better. If it manages to reach a patch attempt
after that it should fail anyway with strict RWX.
diff mbox series

Patch

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 54e145247643..3b3b09d5d2e1 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -82,16 +82,13 @@  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));
+	WARN_ON(cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+				  "powerpc/text_poke:online",
+				  text_area_cpu_up,
+				  text_area_cpu_down) < 0);
+
 	static_branch_enable(&poking_init_done);
 }