[PATCH v6 06/10] powerpc/lib/code-patching: Use alternate map for patch_instruction()

[Michael Ellerman]

Christophe LEROY <christophe.leroy at c-s.fr> writes:

> Le 03/07/2017 à 15:01, Michael Ellerman a écrit :
>> From: Balbir Singh <bsingharora at gmail.com>
>> 
>> This patch creates the window using text_poke_area, allocated via
>> get_vm_area(). text_poke_area is per CPU to avoid locking.
>> text_poke_area for each cpu is setup using late_initcall, prior to
>> setup of these alternate mapping areas, we continue to use direct
>> write to change/modify kernel text. With the ability to use alternate
>> mappings to write to kernel text, it provides us the freedom to then
>> turn text read-only and implement CONFIG_STRICT_KERNEL_RWX.
>> 
>> This code is CPU hotplug aware to ensure that the we have mappings for
>> any new cpus as they come online and tear down mappings for any CPUs
>> that go offline.
>> 
>> Signed-off-by: Balbir Singh <bsingharora at gmail.com>
>> Signed-off-by: Michael Ellerman <mpe at ellerman.id.au>
>> ---
>>   arch/powerpc/lib/code-patching.c | 171 ++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 167 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>> index 500b0f6a0b64..c9de03e0c1f1 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -12,23 +12,186 @@
>>   #include <linux/vmalloc.h>
>>   #include <linux/init.h>
>>   #include <linux/mm.h>
>> -#include <asm/page.h>
>> -#include <asm/code-patching.h>
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/slab.h>
>>   #include <linux/uaccess.h>
>>   #include <linux/kprobes.h>
>>   
>> +#include <asm/pgtable.h>
>> +#include <asm/tlbflush.h>
>> +#include <asm/page.h>
>> +#include <asm/code-patching.h>
>>   
>> -int patch_instruction(unsigned int *addr, unsigned int instr)
>> +static int __patch_instruction(unsigned int *addr, unsigned int instr)
>>   {
>>   	int err;
>>   
>>   	__put_user_size(instr, addr, 4, err);
>>   	if (err)
>>   		return err;
>> -	asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (addr));
>> +
>> +	asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" :: "r" (addr));
>> +
>> +	return 0;
>> +}
>> +
>
> [...]
>
>> +int patch_instruction(unsigned int *addr, unsigned int instr)
>> +{
>> +	int err;
>> +	unsigned int *dest = NULL;
>> +	unsigned long flags;
>> +	unsigned long text_poke_addr;
>> +	unsigned long kaddr = (unsigned long)addr;
>> +
>> +	/*
>> +	 * During early early boot patch_instruction is called
>> +	 * when text_poke_area is not ready, but we still need
>> +	 * to allow patching. We just do the plain old patching
>> +	 * We use slab_is_available and per cpu read * via this_cpu_read
>> +	 * of text_poke_area. Per-CPU areas might not be up early
>> +	 * this can create problems with just using this_cpu_read()
>> +	 */
>> +	if (!slab_is_available() || !this_cpu_read(text_poke_area))
>> +		return __patch_instruction(addr, instr);
>> +
>> +	local_irq_save(flags);
>> +
>> +	text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
>> +	if (map_patch_area(addr, text_poke_addr)) {
>> +		err = -1;
>> +		goto out;
>> +	}
>> +
>> +	dest = (unsigned int *)(text_poke_addr) +
>> +			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>> +
>> +	/*
>> +	 * We use __put_user_size so that we can handle faults while
>> +	 * writing to dest and return err to handle faults gracefully
>> +	 */
>> +	__put_user_size(instr, dest, 4, err);
>> +	if (!err)
>> +		asm ("dcbst 0, %0; sync; icbi 0,%0; icbi 0,%1; sync; isync"
>> +			::"r" (dest), "r"(addr));
>
> Is the second icbi really needed since the alternative area is mapped 
> with PAGE_KERNEL which is not executable ?

The use of dest vs addr is hard to follow. The second icbi is of addr,
which is the executable mapping, so yes we need that icbi.

But the first icbi could be skipped, it's to dest which is not
executable. Is that what you meant? Or am I missing the point entirely.

> If we could avoid that, we could refactor this part as follows:

Yeah that would be a good clean up I think.

> diff --git a/arch/powerpc/lib/code-patching.c 
> b/arch/powerpc/lib/code-patching.c
> index d469224c4ada..85031de43bb9 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -23,15 +23,17 @@
>   #include <asm/code-patching.h>
>   #include <asm/setup.h>
>
> -static int __patch_instruction(unsigned int *addr, unsigned int instr)
> +static int __patch_instruction(unsigned int *addr, unsigned int instr,
> +			       unsigned int *dest)

Renaming addr vs dest would help I think.

ie. maybe:

static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
			       unsigned int *patch_addr)

Or something.

cheers

>   {
>   	int err;
>
> -	__put_user_size(instr, addr, 4, err);
> +	__put_user_size(instr, dest, 4, err);
>   	if (err)
>   		return err;
>
> -	asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" :: "r" (addr));
> +	asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (dest),
> +							    "r" (addr));
>
>   	return 0;
>   }
> @@ -149,7 +151,7 @@ int patch_instruction(unsigned int *addr, unsigned 
> int instr)
>   	 * to allow patching. We just do the plain old patching
>   	 */
>   	if (!this_cpu_read(*PTRRELOC(&text_poke_area)))
> -		return __patch_instruction(addr, instr);
> +		return __patch_instruction(addr, instr, addr);
>
>   	local_irq_save(flags);
>
> @@ -162,14 +164,7 @@ int patch_instruction(unsigned int *addr, unsigned 
> int instr)
>   	dest = (unsigned int *)(text_poke_addr) +
>   			((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>
> -	/*
> -	 * We use __put_user_size so that we can handle faults while
> -	 * writing to dest and return err to handle faults gracefully
> -	 */
> -	__put_user_size(instr, dest, 4, err);
> -	if (!err)
> -		asm ("dcbst 0, %0; sync; icbi 0,%0; icbi 0,%1; sync; isync"
> -			::"r" (dest), "r"(addr));
> +	__patch_instruction(addr, instr, dest);
>
>   	err = unmap_patch_area(text_poke_addr);
>   	if (err)
> @@ -184,7 +179,7 @@ int patch_instruction(unsigned int *addr, unsigned 
> int instr)
>
>   int patch_instruction(unsigned int *addr, unsigned int instr)
>   {
> -	return __patch_instruction(addr, instr);
> +	return __patch_instruction(addr, instr, addr);
>   }
>
>   #endif /* CONFIG_STRICT_KERNEL_RWX */
>
>
>> +
>> +	err = unmap_patch_area(text_poke_addr);
>> +	if (err)
>> +		pr_warn("failed to unmap %lx\n", text_poke_addr);
>> +
>> +out:
>> +	local_irq_restore(flags);
>> +
>> +	return err;
>> +}
>> +#else /* !CONFIG_STRICT_KERNEL_RWX */
>> +
>> +int patch_instruction(unsigned int *addr, unsigned int instr)
>> +{
>> +	return __patch_instruction(addr, instr);
>> +}
>> +
>> +#endif /* CONFIG_STRICT_KERNEL_RWX */
>> +NOKPROBE_SYMBOL(patch_instruction);
>> +
>>   int patch_branch(unsigned int *addr, unsigned long target, int flags)
>>   {
>>   	return patch_instruction(addr, create_branch(addr, target, flags));
>>