[v1,1/8] powerpc/lib/code-patching: Enhance code patching

Le 25/05/2017 à 05:36, Balbir Singh a écrit :
> Today our patching happens via direct copy and
> patch_instruction. The patching code is well
> contained in the sense that copying bits are limited.
>
> While considering implementation of CONFIG_STRICT_RWX,
> the first requirement is to a create another mapping
> that will allow for patching. We create the window using
> text_poke_area, allocated via get_vm_area(), which might
> be an overkill. We can do per-cpu stuff as well. The
> downside of these patches that patch_instruction is
> now synchornized using a lock. Other arches do similar
> things, but use fixmaps. The reason for not using
> fixmaps is to make use of any randomization in the
> future. The code also relies on set_pte_at and pte_clear
> to do the appropriate tlb flushing.
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>

[...]

> +static int kernel_map_addr(void *addr)
> +{
> +	unsigned long pfn;
>  	int err;
>
> -	__put_user_size(instr, addr, 4, err);
> +	if (is_vmalloc_addr(addr))
> +		pfn = vmalloc_to_pfn(addr);
> +	else
> +		pfn = __pa_symbol(addr) >> PAGE_SHIFT;
> +
> +	err = map_kernel_page((unsigned long)text_poke_area->addr,
> +			(pfn << PAGE_SHIFT), _PAGE_KERNEL_RW | _PAGE_PRESENT);

map_kernel_page() doesn't exist on powerpc32, so compilation fails.

However a similar function exists and is called map_page()

Maybe the below modification could help (not tested yet)

Christophe



---
  arch/powerpc/include/asm/book3s/32/pgtable.h | 2 ++
  arch/powerpc/include/asm/nohash/32/pgtable.h | 2 ++
  arch/powerpc/mm/8xx_mmu.c                    | 2 +-
  arch/powerpc/mm/dma-noncoherent.c            | 2 +-
  arch/powerpc/mm/mem.c                        | 4 ++--
  arch/powerpc/mm/mmu_decl.h                   | 1 -
  arch/powerpc/mm/pgtable_32.c                 | 8 ++++----
  7 files changed, 12 insertions(+), 9 deletions(-)

  			vunmap((void *)v);
@@ -215,7 +215,7 @@ void iounmap(volatile void __iomem *addr)
  }
  EXPORT_SYMBOL(iounmap);

-int map_page(unsigned long va, phys_addr_t pa, int flags)
+int map_kernel_page(unsigned long va, phys_addr_t pa, int flags)
  {
  	pmd_t *pd;
  	pte_t *pg;
@@ -255,7 +255,7 @@ void __init __mapin_ram_chunk(unsigned long offset, 
unsigned long top)
  		ktext = ((char *)v >= _stext && (char *)v < etext) ||
  			((char *)v >= _sinittext && (char *)v < _einittext);
  		f = ktext ? pgprot_val(PAGE_KERNEL_TEXT) : pgprot_val(PAGE_KERNEL);
-		map_page(v, p, f);
+		map_kernel_page(v, p, f);
  #ifdef CONFIG_PPC_STD_MMU_32
  		if (ktext)
  			hash_preload(&init_mm, v, 0, 0x300);
@@ -387,7 +387,7 @@ void __set_fixmap (enum fixed_addresses idx, 
phys_addr_t phys, pgprot_t flags)
  		return;
  	}

-	map_page(address, phys, pgprot_val(flags));
+	map_kernel_page(address, phys, pgprot_val(flags));
  	fixmaps++;
  }

On Sun, 2017-05-28 at 16:29 +0200, christophe leroy wrote:
> 
> Le 25/05/2017 à 05:36, Balbir Singh a écrit :
> > Today our patching happens via direct copy and
> > patch_instruction. The patching code is well
> > contained in the sense that copying bits are limited.
> > 
> > While considering implementation of CONFIG_STRICT_RWX,
> > the first requirement is to a create another mapping
> > that will allow for patching. We create the window using
> > text_poke_area, allocated via get_vm_area(), which might
> > be an overkill. We can do per-cpu stuff as well. The
> > downside of these patches that patch_instruction is
> > now synchornized using a lock. Other arches do similar
> > things, but use fixmaps. The reason for not using
> > fixmaps is to make use of any randomization in the
> > future. The code also relies on set_pte_at and pte_clear
> > to do the appropriate tlb flushing.
> > 
> > Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> 
> [...]
> 
> > +static int kernel_map_addr(void *addr)
> > +{
> > +	unsigned long pfn;
> >  	int err;
> > 
> > -	__put_user_size(instr, addr, 4, err);
> > +	if (is_vmalloc_addr(addr))
> > +		pfn = vmalloc_to_pfn(addr);
> > +	else
> > +		pfn = __pa_symbol(addr) >> PAGE_SHIFT;
> > +
> > +	err = map_kernel_page((unsigned long)text_poke_area->addr,
> > +			(pfn << PAGE_SHIFT), _PAGE_KERNEL_RW | _PAGE_PRESENT);
> 
> map_kernel_page() doesn't exist on powerpc32, so compilation fails.
> 
> However a similar function exists and is called map_page()
> 
> Maybe the below modification could help (not tested yet)
> 
> Christophe
>

Thanks, I'll try and get a compile, as an alternative how about

#ifdef CONFIG_PPC32
#define map_kernel_page map_page
#endif

Balbir Singh.

Le 29/05/2017 à 00:58, Balbir Singh a écrit :
> On Sun, 2017-05-28 at 16:29 +0200, christophe leroy wrote:
>>
>> Le 25/05/2017 à 05:36, Balbir Singh a écrit :
>>> Today our patching happens via direct copy and
>>> patch_instruction. The patching code is well
>>> contained in the sense that copying bits are limited.
>>>
>>> While considering implementation of CONFIG_STRICT_RWX,
>>> the first requirement is to a create another mapping
>>> that will allow for patching. We create the window using
>>> text_poke_area, allocated via get_vm_area(), which might
>>> be an overkill. We can do per-cpu stuff as well. The
>>> downside of these patches that patch_instruction is
>>> now synchornized using a lock. Other arches do similar
>>> things, but use fixmaps. The reason for not using
>>> fixmaps is to make use of any randomization in the
>>> future. The code also relies on set_pte_at and pte_clear
>>> to do the appropriate tlb flushing.
>>>
>>> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
>>
>> [...]
>>
>>> +static int kernel_map_addr(void *addr)
>>> +{
>>> +	unsigned long pfn;
>>>   	int err;
>>>
>>> -	__put_user_size(instr, addr, 4, err);
>>> +	if (is_vmalloc_addr(addr))
>>> +		pfn = vmalloc_to_pfn(addr);
>>> +	else
>>> +		pfn = __pa_symbol(addr) >> PAGE_SHIFT;
>>> +
>>> +	err = map_kernel_page((unsigned long)text_poke_area->addr,
>>> +			(pfn << PAGE_SHIFT), _PAGE_KERNEL_RW | _PAGE_PRESENT);
>>
>> map_kernel_page() doesn't exist on powerpc32, so compilation fails.
>>
>> However a similar function exists and is called map_page()
>>
>> Maybe the below modification could help (not tested yet)
>>
>> Christophe
>>
> 
> Thanks, I'll try and get a compile, as an alternative how about
> 
> #ifdef CONFIG_PPC32
> #define map_kernel_page map_page
> #endif
> 

My preference goes to renaming the PPC32 function, first because the 
PPC64 name fits better, second because too many defines kills 
readability, third because two functions doing the same thing are worth 
being called the same, and fourth because we surely have opportunity to 
merge both functions on day.

Christophe