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 |
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 ? > } >
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 --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); }