Balbir Singh <bsinghar...@gmail.com> writes:
> 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. text_poke_area is per CPU to avoid locking
> 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.
>
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c
> index 500b0f6..c0a0834 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -12,23 +12,154 @@
> #include <linux/vmalloc.h>
> #include <linux/init.h>
> #include <linux/mm.h>
> +#include <linux/cpuhotplug.h>
> #include <asm/page.h>
> #include <asm/code-patching.h>
> #include <linux/uaccess.h>
> #include <linux/kprobes.h>
> +#include <asm/pgtable.h>
> +#include <asm/tlbflush.h>
>
> +static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> +static unsigned int text_area_patch_avail;
All of this should be under #ifdef STRICT_RWX. So that when STRICT_RWX=n
we basically use the old code.
> -int patch_instruction(unsigned int *addr, unsigned int instr)
> +static int text_area_cpu_up(unsigned int cpu)
> +{
> + struct vm_struct *area;
> +
> + area = get_vm_area(PAGE_SIZE, VM_ALLOC);
> + if (!area) {
> + WARN_ONCE(1, "Failed to create text area for cpu %d\n",
> + cpu);
> + return -1;
This is good, it will block bringing up a CPU if we can't get the VM
area, which is the safe option.
> + }
> + this_cpu_write(text_poke_area, area);
> + return 0;
> +}
> +
> +static int text_area_cpu_down(unsigned int cpu)
> +{
> + free_vm_area(this_cpu_read(text_poke_area));
> + return 0;
> +}
> +
> +/*
> + * This is an early_initcall and early_initcalls happen at the right time
> + * for us, after slab is enabled and before we mark ro pages R/O. In the
> + * future if get_vm_area is randomized, this will be more flexible than
> + * fixmap
> + */
> +static int __init setup_text_poke_area(void)
> {
> + struct vm_struct *area;
> + int cpu;
> +
> + for_each_online_cpu(cpu) {
> + area = get_vm_area(PAGE_SIZE, VM_ALLOC);
> + if (!area) {
> + WARN_ONCE(1, "Failed to create text area for cpu %d\n",
> + cpu);
> + /* Should we disable strict rwx? */
> + continue;
> + }
> + this_cpu_write(text_poke_area, area);
> + }
So I think skip this.
> + cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> + "powerpc/text_poke:online", text_area_cpu_up,
> + text_area_cpu_down);
Use the cpuhp_setup_state() version, which will call it on the boot CPU.
And then just BUG_ON() if it fails.
Also switch this to a late_initcall(), so that if we do BUG_ON() it's
nice and late and the kernel is mostly up.
> + text_area_patch_avail = 1;
Instead of this global flag, we should just check that each CPUs
text_poke_area is non-NULL before using it ...
> + /*
> + * The barrier here ensures the write is visible to
> + * patch_instruction()
> + */
> + smp_wmb();
> + pr_info("text_poke area ready...\n");
> + return 0;
> +}
> +
> +/*
> + * This can be called for kernel text or a module.
> + */
> +static int kernel_map_addr(void *addr)
map_patch_area() ?
> +{
> + 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;
> +
.. in here.
ie. if (!this_cpu_read(text_poke_area))
return -Exxx;
> + err = map_kernel_page(
> + (unsigned long)__this_cpu_read(text_poke_area)->addr,
> + (pfn << PAGE_SHIFT), pgprot_val(PAGE_KERNEL));
Use a local or two to make that less gross.
> + pr_devel("Mapped addr %p with pfn %lx\n",
> + __this_cpu_read(text_poke_area)->addr, pfn);
err?
> if (err)
> - return err;
> - asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" : : "r" (addr));
> + return -1;
> return 0;
> }
>
> +static inline void kernel_unmap_addr(void *addr)
> +{
> + pte_t *pte;
> + unsigned long kaddr = (unsigned long)addr;
> +
> + pte = pte_offset_kernel(pmd_offset(pud_offset(pgd_offset_k(kaddr),
> + kaddr), kaddr), kaddr);
This is pretty fragile, I'd rather you checked each level returned
something sane.
> + pr_devel("clearing mm %p, pte %p, kaddr %lx\n", &init_mm, pte, kaddr);
> + pte_clear(&init_mm, kaddr, pte);
> + flush_tlb_kernel_range(kaddr, kaddr + PAGE_SIZE);
> +}
> +
> +int patch_instruction(unsigned int *addr, unsigned int instr)
> +{
> + int err;
> + unsigned int *dest = NULL;
> + unsigned long flags;
> + unsigned long kaddr = (unsigned long)addr;
> +
> + /*
> + * Make sure we can see any write of text_area_patch_avail
> + */
> + smp_rmb();
> +
> + /*
> + * 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 text_area_patch_avail, since per cpu read
> + * via __this_cpu_read of text_poke_area might not
> + * yet be available.
> + * TODO: Make text_area_patch_avail per cpu?
> + */
> + if (!text_area_patch_avail) {
This should check for text_poke_area != NULL.
> + __put_user_size(instr, addr, 4, err);
> + if (err)
> + return err;
> + asm ("dcbst 0, %0; sync; icbi 0,%0; sync; isync" :: "r" (addr));
> + return 0;
> + }
> +
> + local_irq_save(flags);
> + if (kernel_map_addr(addr)) {
> + err = -1;
> + goto out;
> + }
> +
> + dest = (unsigned int *)(__this_cpu_read(text_poke_area)->addr) +
> + ((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
!!!
> + __put_user_size(instr, dest, 4, err);
Can you just write a comment explaining why we use __put_user() - in
short, so a fault becomes an error return, not a actual crash.
> + if (!err)
> + asm ("dcbst 0, %0; sync; icbi 0,%0; icbi 0,%1; sync; isync"
> + ::"r" (dest), "r"(addr));
> + kernel_unmap_addr(__this_cpu_read(text_poke_area)->addr);
> +out:
> + local_irq_restore(flags);
> + return err;
> +}
> +NOKPROBE_SYMBOL(patch_instruction);
> +
> int patch_branch(unsigned int *addr, unsigned long target, int flags)
> {
> return patch_instruction(addr, create_branch(addr, target, flags));
> @@ -514,3 +645,4 @@ static int __init test_code_patching(void)
> late_initcall(test_code_patching);
>
> #endif /* CONFIG_CODE_PATCHING_SELFTEST */
> +early_initcall(setup_text_poke_area);
As mentioned before, this should be late_initcall().
cheers
