| Message ID | 20210316031741.1004850-2-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v9,1/8] powerpc/mm: Implement set_memory() routines | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (0512161accb8b6f6dacc85d165350b1812ddcc33) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (fbda7904302499dd7ffc073a3c84eb7c9275db0a) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (1a4431a5db2bf800c647ee0ed87f2727b8d6c29c) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (eed5fae00593ab9d261a0c1ffc1bdb786a87a55a) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (1e28eed17697bcf343c6743f0028cc3b5dd88bf0) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On Tue, 2021-03-16 at 14:17 +1100, Jordan Niethe wrote: > setup_text_poke_area() is a late init call so it runs before > mark_rodata_ro() and after the init calls. This lets all the init > code > patching simply write to their locations. In the future, kprobes is > going to allocate its instruction pages RO which means they will need > setup_text__poke_area() to have been already called for their code > patching. However, init_kprobes() (which allocates and patches some > instruction pages) is an early init call so it happens before > setup_text__poke_area(). > > start_kernel() calls poking_init() before any of the init calls. On > powerpc, poking_init() is currently a nop. setup_text_poke_area() > relies > on kernel virtual memory, cpu hotplug and per_cpu_areas being setup. > setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are > called > before poking_init(). > > Turn setup_text_poke_area() into poking_init(). > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> Good job finding & fixing this bug! Reviewed-by: Russell Currey <ruscur@russell.cc> > --- > v9: New to series > --- > arch/powerpc/lib/code-patching.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/arch/powerpc/lib/code-patching.c > b/arch/powerpc/lib/code-patching.c > index 2333625b5e31..b28afa1133db 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -65,14 +65,11 @@ static int text_area_cpu_down(unsigned int cpu) > } > > /* > - * Run as a late init call. This allows all the boot time patching > to be done > - * simply by patching the code, and then we're called here prior to > - * mark_rodata_ro(), which happens after all init calls are run. > 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(). > + * 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(). > */ > -static int __init setup_text_poke_area(void) > +int __init poking_init(void) > { > BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > "powerpc/text_poke:online", text_area_cpu_up, > @@ -80,7 +77,6 @@ static int __init setup_text_poke_area(void) > > return 0; > } > -late_initcall(setup_text_poke_area); > > /* > * This can be called for kernel text or a module.
Le 16/03/2021 à 04:17, Jordan Niethe a écrit : > setup_text_poke_area() is a late init call so it runs before > mark_rodata_ro() and after the init calls. This lets all the init code > patching simply write to their locations. In the future, kprobes is > going to allocate its instruction pages RO which means they will need > setup_text__poke_area() to have been already called for their code > patching. However, init_kprobes() (which allocates and patches some > instruction pages) is an early init call so it happens before > setup_text__poke_area(). > > start_kernel() calls poking_init() before any of the init calls. On > powerpc, poking_init() is currently a nop. setup_text_poke_area() relies > on kernel virtual memory, cpu hotplug and per_cpu_areas being setup. > setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are called > before poking_init(). > > Turn setup_text_poke_area() into poking_init(). > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v9: New to series > --- > arch/powerpc/lib/code-patching.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index 2333625b5e31..b28afa1133db 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -65,14 +65,11 @@ static int text_area_cpu_down(unsigned int cpu) > } > > /* > - * Run as a late init call. This allows all the boot time patching to be done > - * simply by patching the code, and then we're called here prior to > - * mark_rodata_ro(), which happens after all init calls are run. 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(). > + * 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(). Please use WARN_ON(), see why at https://www.kernel.org/doc/html/latest/process/deprecated.html > */ > -static int __init setup_text_poke_area(void) > +int __init poking_init(void) > { > BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > "powerpc/text_poke:online", text_area_cpu_up, > @@ -80,7 +77,6 @@ static int __init setup_text_poke_area(void) > > return 0; > } > -late_initcall(setup_text_poke_area); > > /* > * This can be called for kernel text or a module. >
On Tue, Mar 16, 2021 at 5:32 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 16/03/2021 à 04:17, Jordan Niethe a écrit : > > setup_text_poke_area() is a late init call so it runs before > > mark_rodata_ro() and after the init calls. This lets all the init code > > patching simply write to their locations. In the future, kprobes is > > going to allocate its instruction pages RO which means they will need > > setup_text__poke_area() to have been already called for their code > > patching. However, init_kprobes() (which allocates and patches some > > instruction pages) is an early init call so it happens before > > setup_text__poke_area(). > > > > start_kernel() calls poking_init() before any of the init calls. On > > powerpc, poking_init() is currently a nop. setup_text_poke_area() relies > > on kernel virtual memory, cpu hotplug and per_cpu_areas being setup. > > setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are called > > before poking_init(). > > > > Turn setup_text_poke_area() into poking_init(). > > > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > v9: New to series > > --- > > arch/powerpc/lib/code-patching.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > > index 2333625b5e31..b28afa1133db 100644 > > --- a/arch/powerpc/lib/code-patching.c > > +++ b/arch/powerpc/lib/code-patching.c > > @@ -65,14 +65,11 @@ static int text_area_cpu_down(unsigned int cpu) > > } > > > > /* > > - * Run as a late init call. This allows all the boot time patching to be done > > - * simply by patching the code, and then we're called here prior to > > - * mark_rodata_ro(), which happens after all init calls are run. 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(). > > + * 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(). > > Please use WARN_ON(), see why at https://www.kernel.org/doc/html/latest/process/deprecated.html Ok I can include a change to WARN_ON() as a separate patch. > > > */ > > -static int __init setup_text_poke_area(void) > > +int __init poking_init(void) > > { > > BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, > > "powerpc/text_poke:online", text_area_cpu_up, > > @@ -80,7 +77,6 @@ static int __init setup_text_poke_area(void) > > > > return 0; > > } > > -late_initcall(setup_text_poke_area); > > > > /* > > * This can be called for kernel text or a module. > >
Jordan Niethe <jniethe5@gmail.com> writes: > On Tue, Mar 16, 2021 at 5:32 PM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> Le 16/03/2021 à 04:17, Jordan Niethe a écrit : >> > setup_text_poke_area() is a late init call so it runs before >> > mark_rodata_ro() and after the init calls. This lets all the init code >> > patching simply write to their locations. In the future, kprobes is >> > going to allocate its instruction pages RO which means they will need >> > setup_text__poke_area() to have been already called for their code >> > patching. However, init_kprobes() (which allocates and patches some >> > instruction pages) is an early init call so it happens before >> > setup_text__poke_area(). >> > >> > start_kernel() calls poking_init() before any of the init calls. On >> > powerpc, poking_init() is currently a nop. setup_text_poke_area() relies >> > on kernel virtual memory, cpu hotplug and per_cpu_areas being setup. >> > setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are called >> > before poking_init(). >> > >> > Turn setup_text_poke_area() into poking_init(). >> > >> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> >> > --- >> > v9: New to series >> > --- >> > arch/powerpc/lib/code-patching.c | 12 ++++-------- >> > 1 file changed, 4 insertions(+), 8 deletions(-) >> > >> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c >> > index 2333625b5e31..b28afa1133db 100644 >> > --- a/arch/powerpc/lib/code-patching.c >> > +++ b/arch/powerpc/lib/code-patching.c >> > @@ -65,14 +65,11 @@ static int text_area_cpu_down(unsigned int cpu) >> > } >> > >> > /* >> > - * Run as a late init call. This allows all the boot time patching to be done >> > - * simply by patching the code, and then we're called here prior to >> > - * mark_rodata_ro(), which happens after all init calls are run. 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(). >> > + * 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(). >> >> Please use WARN_ON(), see why at https://www.kernel.org/doc/html/latest/process/deprecated.html > Ok I can include a change to WARN_ON() as a separate patch. I'm not convinced we should change this to a WARN_ON. Being able to patch the kernel text is not optional. Patching jump labels has no ability to return an error, and the code that uses them has no concept of the jump label not taking the correct polarity. Silently failing the patch is like randomly flipping an if condition somewhere in the kernel and hoping that everything will continue working. cheers
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index 2333625b5e31..b28afa1133db 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -65,14 +65,11 @@ static int text_area_cpu_down(unsigned int cpu) } /* - * Run as a late init call. This allows all the boot time patching to be done - * simply by patching the code, and then we're called here prior to - * mark_rodata_ro(), which happens after all init calls are run. 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(). + * 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(). */ -static int __init setup_text_poke_area(void) +int __init poking_init(void) { BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "powerpc/text_poke:online", text_area_cpu_up, @@ -80,7 +77,6 @@ static int __init setup_text_poke_area(void) return 0; } -late_initcall(setup_text_poke_area); /* * This can be called for kernel text or a module.
setup_text_poke_area() is a late init call so it runs before mark_rodata_ro() and after the init calls. This lets all the init code patching simply write to their locations. In the future, kprobes is going to allocate its instruction pages RO which means they will need setup_text__poke_area() to have been already called for their code patching. However, init_kprobes() (which allocates and patches some instruction pages) is an early init call so it happens before setup_text__poke_area(). start_kernel() calls poking_init() before any of the init calls. On powerpc, poking_init() is currently a nop. setup_text_poke_area() relies on kernel virtual memory, cpu hotplug and per_cpu_areas being setup. setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are called before poking_init(). Turn setup_text_poke_area() into poking_init(). Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v9: New to series --- arch/powerpc/lib/code-patching.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)