Re: [PATCH v4] powerpc: Avoid code patching freed init sections

Re: [PATCH v4] powerpc: Avoid code patching freed init sections

[Andreas Schwab]

On Sep 14 2018, Michael Neuling <mikey@neuling.org> wrote:

> This stops us from doing code patching in init sections after they've
> been freed.

This breaks booting on PowerBook6,7, crashing very early.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH] powerpc: use PTRRELOC during early init

[Andreas Schwab]

This fixes a crash on powerpc32 when using global data during early init
without relocating its address.

Fixes: 51c3c62b58 (powerpc: Avoid code patching freed init sections)
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
---
 arch/powerpc/lib/code-patching.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 6ae2777c22..6192fdae36 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -29,7 +29,7 @@ static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
 	int err;
 
 	/* Make sure we aren't patching a freed init section */
-	if (init_mem_is_free && init_section_contains(exec_addr, 4)) {
+	if (*PTRRELOC(&init_mem_is_free) && init_section_contains(exec_addr, 4)) {
 		pr_debug("Skipping init section patching addr: 0x%px\n", exec_addr);
 		return 0;
 	}
-- 
2.19.0

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v4] powerpc: Avoid code patching freed init sections

[Michael Neuling]

On Tue, 2018-10-02 at 23:35 +0200, Andreas Schwab wrote:
> On Sep 14 2018, Michael Neuling <mikey@neuling.org> wrote:
> 
> > This stops us from doing code patching in init sections after they've
> > been freed.
> 
> This breaks booting on PowerBook6,7, crashing very early.

Sorry, Can you try?

http://patchwork.ozlabs.org/patch/977195/

Mikey

Re: [PATCH v4] powerpc: Avoid code patching freed init sections

[Michael Ellerman]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Sep 14 2018, Michael Neuling <mikey@neuling.org> wrote:
>
>> This stops us from doing code patching in init sections after they've
>> been freed.
>
> This breaks booting on PowerBook6,7, crashing very early.

Crud, sorry.

My CI setup tests with the mac99 qemu model, but that boots happily, not
sure why.

cheers

Re: [PATCH v4] powerpc: Avoid code patching freed init sections

[Christophe LEROY]

Le 03/10/2018 à 05:20, Michael Ellerman a écrit :
> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
>> On Sep 14 2018, Michael Neuling <mikey@neuling.org> wrote:
>>
>>> This stops us from doing code patching in init sections after they've
>>> been freed.
>>
>> This breaks booting on PowerBook6,7, crashing very early.
> 
> Crud, sorry.
> 
> My CI setup tests with the mac99 qemu model, but that boots happily, not
> sure why.

Maybe mac99 doesn't use relocation ?

The issue is that during early boot, 'init_mem_is_free' is not yet at 
its definitif address.

I sent a fix for it : https://patchwork.ozlabs.org/patch/977195/

Christophe

Re: [PATCH] powerpc: use PTRRELOC during early init

[Christophe LEROY]

Hi Andreas,

Le 03/10/2018 à 00:33, Andreas Schwab a écrit :
> This fixes a crash on powerpc32 when using global data during early init
> without relocating its address.
> 
> Fixes: 51c3c62b58 (powerpc: Avoid code patching freed init sections)
> Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
> ---
>   arch/powerpc/lib/code-patching.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 6ae2777c22..6192fdae36 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -29,7 +29,7 @@ static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
>   	int err;
>   
>   	/* Make sure we aren't patching a freed init section */
> -	if (init_mem_is_free && init_section_contains(exec_addr, 4)) {
> +	if (*PTRRELOC(&init_mem_is_free) && init_section_contains(exec_addr, 4)) {

That's not the best solution. In the past we already did our best to 
separate early use of patch_instruction() , that's how 
raw_patch_instruction() was born, see 
https://patchwork.ozlabs.org/patch/840974/

Here, it idea is similar, this test should not apply to 
raw_patch_instruction()

Did you try my proposed fix https://patchwork.ozlabs.org/patch/977195/ ?

Christophe

>   		pr_debug("Skipping init section patching addr: 0x%px\n", exec_addr);
>   		return 0;
>   	}
> 

Re: [PATCH] powerpc: use PTRRELOC during early init

[Andreas Schwab]

On Okt 03 2018, Christophe LEROY <christophe.leroy@c-s.fr> wrote:

> Did you try my proposed fix https://patchwork.ozlabs.org/patch/977195/ ?

That works as well.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."