From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:16
This will allow simplifying the returns from do_page_fault
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
@@ -195,10 +195,9 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)*Thereturnvalueis0ifthefaultwashandled,orthesignal*numberifthisisakernelfaultthatcan'tbehandledhere.*/-intdo_page_fault(structpt_regs*regs,unsignedlongaddress,-unsignedlongerror_code)+staticint__do_page_fault(structpt_regs*regs,unsignedlongaddress,+unsignedlongerror_code){-enumctx_stateprev_state=exception_enter();structvm_area_struct*vma;structmm_struct*mm=current->mm;unsignedintflags=FAULT_FLAG_ALLOW_RETRY|FAULT_FLAG_KILLABLE;
@@ -523,6 +522,15 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,rc=SIGSEGV;bail:+returnrc;+}+NOKPROBE_SYMBOL(__do_page_fault);++intdo_page_fault(structpt_regs*regs,unsignedlongaddress,+unsignedlongerror_code)+{+enumctx_stateprev_state=exception_enter();+intrc=__do_page_fault(regs,address,error_code);exception_exit(prev_state);returnrc;}
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:22
By filtering the relevant SRR1 bits in the assembly rather than
in do_page_fault() itself, we avoid a conditional branch (since we
already come from different path for data and instruction faults).
This will allow more simplifications later
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/head_32.S | 2 +-
arch/powerpc/kernel/head_8xx.S | 4 ++--
arch/powerpc/mm/fault.c | 14 ++------------
3 files changed, 5 insertions(+), 15 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:25
We test a number of bits from DSISR/SRR1 before deciding
to call hash_page(). If any of these is set, we go directly
to do_page_fault() as the bit indicate a fault that needs
to be handled there (no hashing needed).
This updates the current open-coded masks to use the new
DSISR definitions.
This *does* change the masks actually used in two ways:
- We used to test various bits that were defined as "always 0"
in the architecture and could be repurposed for something
else. From now on, we just ignore such bits.
- We were missing some new bits defined on P9
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/exceptions-64s.S | 6 ++++--
arch/powerpc/kernel/head_32.S | 2 +-
2 files changed, 5 insertions(+), 3 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:28
On legacy 6xx 32-bit procesors, we checked for the DABR match bit
in DSISR from do_page_fault(), in the middle of a pile of ifdef's
because all other CPU types do it in assembly prior to calling
do_page_fault. Fix that.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/entry_32.S | 11 +++++++++++
arch/powerpc/mm/fault.c | 9 ---------
2 files changed, 11 insertions(+), 9 deletions(-)
@@ -242,15 +242,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,gotobail;}-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE) || \-defined(CONFIG_PPC_BOOK3S_64)||defined(CONFIG_PPC_8xx))-if(error_code&DSISR_DABRMATCH){-/* breakpoint match */-do_break(regs,address,error_code);-gotobail;-}-#endif-/* We restore the interrupt state now */if(!arch_irq_disabled_regs(regs))local_irq_enable();
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:32
This updates the definitions for the various DSISR bits to
match both some historical stuff and to match new bits on
POWER9.
In addition, we define some masks corresponding to the "bad"
faults on Book3S, and some masks corresponding to the bits
that match between DSISR and SRR1 for a DSI and an ISI.
This comes with a small code update to change the definition
of DSISR_PGDIRFAULT which becomes DSISR_PRTABLE_FAULT to
match architecture 3.0B
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/include/asm/reg.h | 69 +++++++++++++++++++++++++++++-----
arch/powerpc/kvm/book3s_64_mmu_radix.c | 4 +-
2 files changed, 61 insertions(+), 12 deletions(-)
@@ -322,13 +322,13 @@ int kvmppc_book3s_radix_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,gpa=vcpu->arch.fault_gpa&~0xfffUL;gpa&=~0xF000000000000000ul;gfn=gpa>>PAGE_SHIFT;-if(!(dsisr&DSISR_PGDIRFAULT))+if(!(dsisr&DSISR_PRTABLE_FAULT))gpa|=ea&0xfff;memslot=gfn_to_memslot(kvm,gfn);/* No memslot means it's an emulated MMIO region */if(!memslot||(memslot->flags&KVM_MEMSLOT_INVALID)){-if(dsisr&(DSISR_PGDIRFAULT|DSISR_BADACCESS|+if(dsisr&(DSISR_PRTABLE_FAULT|DSISR_BADACCESS|DSISR_SET_RC)){/**Badaddressinguestpagetabletree,orother
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:37
This uses the newly defined constants for this rather than open-coded
numbers. There is a side effect on 64-bit which is to pass through
some of the new P9 bits which we didn't before.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/exceptions-64s.S | 2 +-
arch/powerpc/kernel/head_32.S | 4 ++--
arch/powerpc/kernel/head_8xx.S | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:40
Define a common page_fault_is_write() helper and use it
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:44
There's no point looking for the VMA etc.. when we already know
we are going to fail.
This adds some code to set "code" for the si_code but that will
be gone in subsequent patches.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
@@ -237,6 +237,26 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(unlikely(debugger_fault_handler(regs)))gotobail;+#if defined(CONFIG_6xx)+if(error_code&0x95700000){+/* an error such as lwarx to I/O controller space,+addressmatchingDABR,eciwx,etc.*/+code=SEGV_ACCERR;+gotobad_area_nosemaphore;+}+#endif /* CONFIG_6xx */+#if defined(CONFIG_8xx)+/* The MPC8xx seems to always set 0x80000000, which is+*"undefined".Ofthosethatcanbeset,thisistheonly+*onewhichseemsbad.+*/+if(error_code&0x10000000){+/* Guarded storage error. */+code=SEGV_ACCERR;+gotobad_area_nosemaphore;+}+#endif /* CONFIG_8xx */+/**Thekernelshouldnevertakeanexecutefaultnorshouldit*takeapagefaulttoakerneladdress.
@@ -351,21 +371,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,good_area:code=SEGV_ACCERR;-#if defined(CONFIG_6xx)-if(error_code&0x95700000)-/* an error such as lwarx to I/O controller space,-addressmatchingDABR,eciwx,etc.*/-gotobad_area;-#endif /* CONFIG_6xx */-#if defined(CONFIG_8xx)-/* The MPC8xx seems to always set 0x80000000, which is-*"undefined".Ofthosethatcanbeset,thisistheonly-*onewhichseemsbad.-*/-if(error_code&0x10000000)-/* Guarded storage error. */-gotobad_area;-#endif /* CONFIG_8xx */if(is_exec){/*
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:46
A bad page fault is when the HW signals an error such as a bad
copy/paste, an AMO error, or some other type of error that will
not be fixed by updating the PTE.
Use a helper page_fault_is_bad() to check for bad page faults thus
removing the per-processor family open-coding in __do_page_fault()
and trigger a SIGBUS rather than a SIGSEGV which is more appropriate.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 32 ++++++++++++++------------------
1 file changed, 14 insertions(+), 18 deletions(-)
@@ -237,25 +245,13 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(unlikely(debugger_fault_handler(regs)))gotobail;-#if defined(CONFIG_6xx)-if(error_code&0x95700000){-/* an error such as lwarx to I/O controller space,-addressmatchingDABR,eciwx,etc.*/-code=SEGV_ACCERR;-gotobad_area_nosemaphore;-}-#endif /* CONFIG_6xx */-#if defined(CONFIG_8xx)-/* The MPC8xx seems to always set 0x80000000, which is-*"undefined".Ofthosethatcanbeset,thisistheonly-*onewhichseemsbad.-*/-if(error_code&0x10000000){-/* Guarded storage error. */-code=SEGV_ACCERR;-gotobad_area_nosemaphore;+if(unlikely(page_fault_is_bad(error_code))){+if(is_user)+_exception(SIGBUS,regs,BUS_OBJERR,address);+else+rc=SIGBUS;+gotobail;}-#endif /* CONFIG_8xx *//**Thekernelshouldnevertakeanexecutefaultnorshouldit
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:50:49
Now that we moved the exception state handling to a wrapper, we can
just directly return rather than "goto bail"
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 39 ++++++++++++++++-----------------------
1 file changed, 16 insertions(+), 23 deletions(-)
@@ -233,39 +233,36 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(error_code&ICSWX_DSI_UCT){rc=acop_handle_fault(regs,address,error_code);if(rc)-gotobail;+returnrc;}#endif /* CONFIG_PPC_ICSWX */if(notify_page_fault(regs))-gotobail;+return0;if(unlikely(page_fault_is_bad(error_code))){-if(is_user)+if(is_user){_exception(SIGBUS,regs,BUS_OBJERR,address);-else-rc=SIGBUS;-gotobail;+return0;+}+returnSIGBUS;}/**Thekernelshouldnevertakeanexecutefaultnorshouldit*takeapagefaulttoakerneladdress.*/-if(!is_user&&(is_exec||(address>=TASK_SIZE))){-rc=SIGSEGV;-gotobail;-}+if(!is_user&&(is_exec||(address>=TASK_SIZE)))+returnSIGSEGV;/* We restore the interrupt state now */if(!arch_irq_disabled_regs(regs))local_irq_enable();if(faulthandler_disabled()||mm==NULL){-if(!is_user){-rc=SIGSEGV;-gotobail;-}+if(!is_user)+returnSIGSEGV;+/* faulthandler_disabled() in user mode is really bad,asiscurrent->mm==NULL.*/printk(KERN_EMERG"Page fault in user mode with "
@@ -454,9 +451,8 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,gotobad_area_nosemaphore;rc=mm_fault_error(regs,address,fault);if(rc>=MM_FAULT_RETURN)-gotobail;-else-rc=0;+returnrc;+rc=0;}/*
@@ -483,7 +479,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,regs,address);}-gotobail;+returnrc;bad_area:up_read(&mm->mmap_sem);
@@ -492,7 +488,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,/* User mode accesses cause a SIGSEGV */if(is_user){_exception(SIGSEGV,regs,code,address);-gotobail;+return0;}if(is_exec&&(error_code&DSISR_PROTFAULT))
@@ -500,10 +496,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address," page (%lx) - exploit attempt? (uid: %d)\n",address,from_kuid(&init_user_ns,current_uid()));-rc=SIGSEGV;--bail:-returnrc;+returnSIGSEGV;}NOKPROBE_SYMBOL(__do_page_fault);
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:09
Do the check before we re-enable interrupts and clean the code
up a bit.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
@@ -355,24 +355,23 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(unlikely(!is_user&&bad_kernel_fault(is_exec,error_code,address)))returnSIGSEGV;+/*+*Ifwe'reinaninterrupt,havenousercontextorarerunning+*inaregionwithpagefaultsdisabledthenwemustnottakethefault+*/+if(unlikely(faulthandler_disabled()||!mm)){+if(is_user)+printk_ratelimited(KERN_ERR"Page fault in user mode"+" with faulthandler_disabled()=%d"+" mm=%=p\n",+faulthandler_disabled(),mm);+returnbad_area_nosemaphore(regs,address);+}+/* We restore the interrupt state now */if(!arch_irq_disabled_regs(regs))local_irq_enable();-if(faulthandler_disabled()||mm==NULL){-if(!is_user)-returnSIGSEGV;--/* faulthandler_disabled() in user mode is really bad,-asiscurrent->mm==NULL.*/-printk(KERN_EMERG"Page fault in user mode with "-"faulthandler_disabled() = %d mm = %p\n",-faulthandler_disabled(),mm);-printk(KERN_EMERG"NIP = %lx MSR = %lx\n",-regs->nip,regs->msr);-die("Weird page fault",regs,SIGSEGV);-}-perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS,1,regs,address);/*
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:13
Instead of goto labels, instead call those functions and return.
This gets us closer to x86 and allows us to shring do_page_fault()
even more.
The main difference with x86 is that those function return a value
which we then return from do_page_fault(). That value is our
return value from do_page_fault() which we use to generate
kernel faults.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 78 +++++++++++++++++++++++++++++++------------------
1 file changed, 50 insertions(+), 28 deletions(-)
@@ -231,7 +270,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,structvm_area_struct*vma;structmm_struct*mm=current->mm;unsignedintflags=FAULT_FLAG_ALLOW_RETRY|FAULT_FLAG_KILLABLE;-intcode=SEGV_MAPERR;intis_exec=TRAP(regs)==0x400;intis_user=user_mode(regs);intis_write=page_fault_is_write(error_code);
@@ -317,7 +355,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,*/if(!down_read_trylock(&mm->mmap_sem)){if(!is_user&&!search_exception_tables(regs->nip))-gotobad_area_nosemaphore;+returnbad_area_nosemaphore(regs,address);retry:down_read(&mm->mmap_sem);
@@ -332,11 +370,11 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,vma=find_vma(mm,address);if(!vma)-gotobad_area;+returnbad_area(regs,address);if(vma->vm_start<=address)gotogood_area;if(!(vma->vm_flags&VM_GROWSDOWN))-gotobad_area;+returnbad_area(regs,address);/**N.B.ThePOWER/OpenABIallowsprogramstoaccessupto
@@ -351,7 +389,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,/* get user regs even if this fault is in kernel mode */structpt_regs*uregs=current->thread.regs;if(uregs==NULL)-gotobad_area;+returnbad_area(regs,address);/**Auser-modeaccesstoanaddressalongwaybelow
@@ -366,14 +404,12 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,*expandthestackratherthansegfaulting.*/if(address+2048<uregs->gpr[1]&&!store_update_sp)-gotobad_area;+returnbad_area(regs,address);}if(expand_stack(vma,address))-gotobad_area;+returnbad_area(regs,address);good_area:-code=SEGV_ACCERR;-if(is_exec){/**AllowexecutionfromreadableareasiftheMMUdoesnot
@@ -388,16 +424,16 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(!(vma->vm_flags&VM_EXEC)&&(cpu_has_feature(CPU_FTR_NOEXECUTE)||!(vma->vm_flags&(VM_READ|VM_WRITE))))-gotobad_area;+returnbad_area(regs,address);/* a write */}elseif(is_write){if(!(vma->vm_flags&VM_WRITE))-gotobad_area;+returnbad_area(regs,address);flags|=FAULT_FLAG_WRITE;/* a read */}else{if(!(vma->vm_flags&(VM_READ|VM_EXEC|VM_WRITE)))-gotobad_area;+returnbad_area(regs,address);}#ifdef CONFIG_PPC_STD_MMU/*
@@ -462,11 +498,10 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(unlikely(fault&(VM_FAULT_RETRY|VM_FAULT_ERROR))){if(fault&VM_FAULT_SIGSEGV)-gotobad_area_nosemaphore;+returnbad_area_nosemaphore(regs,address);rc=mm_fault_error(regs,address,fault);if(rc>=MM_FAULT_RETURN)returnrc;-rc=0;}/*
@@ -492,20 +527,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN,1,regs,address);}--returnrc;--bad_area:-up_read(&mm->mmap_sem);--bad_area_nosemaphore:-/* User mode accesses cause a SIGSEGV */-if(is_user){-_exception(SIGSEGV,regs,code,address);-return0;-}--returnSIGSEGV;+return0;}NOKPROBE_SYMBOL(__do_page_fault);
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:13
First, handle the normal retry failure in do_page_fault itself,
since it's a simple return statement. That allows us to remove
the "continue" special return code from mm_fault_error().
Once that's done, we can have an implementation much closer to
x86 where we only call mm_fault_error() if VM_FAULT_ERROR is set
and directly return.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 66 +++++++++++++++++++++----------------------------
1 file changed, 28 insertions(+), 38 deletions(-)
@@ -147,10 +147,6 @@ static noinline int bad_area(struct pt_regs *regs, unsigned long address)return__bad_area(regs,address,SEGV_MAPERR);}-#define MM_FAULT_RETURN 0-#define MM_FAULT_CONTINUE -1-#define MM_FAULT_ERR(sig) (sig)-staticintdo_sigbus(structpt_regs*regs,unsignedlongaddress,unsignedintfault){
@@ -158,7 +154,7 @@ static int do_sigbus(struct pt_regs *regs, unsigned long address,unsignedintlsb=0;if(!user_mode(regs))-returnMM_FAULT_ERR(SIGBUS);+returnSIGBUS;current->thread.trap_nr=BUS_ADRERR;info.si_signo=SIGBUS;
@@ -179,25 +175,17 @@ static int do_sigbus(struct pt_regs *regs, unsigned long address,#endifinfo.si_addr_lsb=lsb;force_sig_info(SIGBUS,&info,current);-returnMM_FAULT_RETURN;+return0;}staticintmm_fault_error(structpt_regs*regs,unsignedlongaddr,intfault){/*-*PagefaultwasinterruptedbySIGKILL.Wehavenoreasonto-*continuethepagefault.+*KernelpagefaultinterruptedbySIGKILL.Wehavenoreasonto+*continueprocessing.*/-if(fatal_signal_pending(current)){-/* Coming from kernel, we need to deal with uaccess fixups */-if(user_mode(regs))-returnMM_FAULT_RETURN;-returnMM_FAULT_ERR(SIGKILL);-}--/* No fault: be happy */-if(!(fault&VM_FAULT_ERROR))-returnMM_FAULT_CONTINUE;+if(fatal_signal_pending(current)&&!user_mode(regs))+returnSIGKILL;/* Out of memory */if(fault&VM_FAULT_OOM){
@@ -206,17 +194,18 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)*madeusunabletohandlethepagefaultgracefully.*/if(!user_mode(regs))-returnMM_FAULT_ERR(SIGKILL);+returnSIGSEGV;pagefault_out_of_memory();-returnMM_FAULT_RETURN;+}else{+if(fault&(VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|+VM_FAULT_HWPOISON_LARGE))+returndo_sigbus(regs,addr,fault);+elseif(fault&VM_FAULT_SIGSEGV)+returnbad_area_nosemaphore(regs,addr);+else+BUG();}--if(fault&(VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|VM_FAULT_HWPOISON_LARGE))-returndo_sigbus(regs,addr,fault);--/* We don't understand the fault code, this is fatal */-BUG();-returnMM_FAULT_CONTINUE;+return0;}/* Is this a bad kernel fault ? */
@@ -274,7 +263,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,intis_user=user_mode(regs);intis_write=page_fault_is_write(error_code);intfault;-intrc=0,store_update_sp=0;+intstore_update_sp=0;#ifdef CONFIG_PPC_ICSWX/*
@@ -283,7 +272,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,*lookatit*/if(error_code&ICSWX_DSI_UCT){-rc=acop_handle_fault(regs,address,error_code);+intrc=acop_handle_fault(regs,address,error_code);if(rc)returnrc;}
@@ -492,18 +481,19 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(!fatal_signal_pending(current))gotoretry;}-/* We will enter mm_fault_error() below */-}else-up_read(¤t->mm->mmap_sem);-if(unlikely(fault&(VM_FAULT_RETRY|VM_FAULT_ERROR))){-if(fault&VM_FAULT_SIGSEGV)-returnbad_area_nosemaphore(regs,address);-rc=mm_fault_error(regs,address,fault);-if(rc>=MM_FAULT_RETURN)-returnrc;+/*+*Usermode?Justreturntohandlethefatalexceptionotherwise+*returntobad_page_fault+*/+returnis_user?0:SIGBUS;}+up_read(¤t->mm->mmap_sem);++if(unlikely(fault&VM_FAULT_ERROR))+returnmm_fault_error(regs,address,fault);+/**Major/minorpagefaultaccounting.*/
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:19
It makes do_page_fault() more readable. No functional change.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
@@ -461,30 +492,8 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,returnbad_area(regs,address);good_area:-if(is_exec){-/*-*AllowexecutionfromreadableareasiftheMMUdoesnot-*provideseparatecontrolsoverreadingandexecuting.-*-*Note:Thatcodeusedtonotbeenabledfor4xx/BookE.-*ItisnowasI/Dcachecoherencyfortheseisdoneat-*set_pte_at()timeandIseenoreasonwhythetest-*belowwouldn'tbevalidonthoseprocessors.This-may--*breakprogramscompiledwithareallyoldABIthough.-*/-if(unlikely(!(vma->vm_flags&VM_EXEC)&&-(cpu_has_feature(CPU_FTR_NOEXECUTE)||-!(vma->vm_flags&(VM_READ|VM_WRITE)))))-returnbad_area(regs,address);-/* a write */-}elseif(is_write){-if(unlikely(!(vma->vm_flags&VM_WRITE)))-returnbad_area(regs,address);-/* a read */-}else{-if(unlikely(!(vma->vm_flags&(VM_READ|VM_EXEC|VM_WRITE))))-returnbad_area(regs,address);-}+if(unlikely(access_error(is_write,is_exec,vma)))+returnbad_area(regs,address);/**Ifforanyreasonatallwecouldn'thandlethefault,
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:23
When hitting below a VM_GROWSDOWN vma (typically growing the stack),
we check whether it's a valid stack-growing instruction and we
check the distance to GPR1. This is largely open coded with lots
of comments, so move it out to a helper.
While at it, make store_update_sp a boolean.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 84 ++++++++++++++++++++++++++++---------------------
1 file changed, 48 insertions(+), 36 deletions(-)
@@ -71,15 +71,15 @@ static inline bool notify_page_fault(struct pt_regs *regs)*Checkwhethertheinstructionatregs->nipisastoreusing*anupdateaddressingformwhichwillupdater1.*/-staticintstore_updates_sp(structpt_regs*regs)+staticboolstore_updates_sp(structpt_regs*regs){unsignedintinst;if(get_user(inst,(unsignedint__user*)regs->nip))-return0;+returnfalse;/* check for 1 in the rA field */if(((inst>>16)&0x1f)!=1)-return0;+returnfalse;/* check major opcode */switch(inst>>26){case37:/* stwu */
@@ -87,7 +87,7 @@ static int store_updates_sp(struct pt_regs *regs)case45:/* sthu */case53:/* stfsu */case55:/* stfdu */-return1;+returntrue;case62:/* std or stdu */return(inst&3)==1;case31:
@@ -222,6 +222,43 @@ static bool bad_kernel_fault(bool is_exec, unsigned long error_code,returnis_exec||(address>=TASK_SIZE);}+staticboolbad_stack_expansion(structpt_regs*regs,unsignedlongaddress,+structvm_area_struct*vma,+boolstore_update_sp)+{+/*+*N.B.ThePOWER/OpenABIallowsprogramstoaccessupto+*288bytesbelowthestackpointer.+*Thekernelsignaldeliverycodewritesuptoabout1.5kB+*belowthestackpointer(r1)beforedecrementingit.+*Theexeccodecanwriteslightlyover640kBtothestack+*beforesettingtheuserr1.Thusweallowthestackto+*expandto1MBwithoutfurtherchecks.+*/+if(address+0x100000<vma->vm_end){+/* get user regs even if this fault is in kernel mode */+structpt_regs*uregs=current->thread.regs;+if(uregs==NULL)+returntrue;++/*+*Auser-modeaccesstoanaddressalongwaybelow+*thestackpointerisonlyvalidiftheinstruction+*isonewhichwouldupdatethestackpointertothe+*addressaccessediftheinstructioncompleted,+*i.e.eitherstwurs,n(r1)orstwuxrs,r1,rb+*(orthebyte,halfword,floatordoubleforms).+*+*Ifwedon'tcheckthisthenanywritetothearea+*betweenthelastmappedregionandthestackwill+*expandthestackratherthansegfaulting.+*/+if(address+2048<uregs->gpr[1]&&!store_update_sp)+returntrue;+}+returnfalse;+}+staticboolaccess_error(boolis_write,boolis_exec,structvm_area_struct*vma){
@@ -350,7 +387,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,intis_user=user_mode(regs);intis_write=page_fault_is_write(error_code);intfault,major=0;-intstore_update_sp=0;+boolstore_update_sp=false;#ifdef CONFIG_PPC_ICSWX/*
@@ -458,36 +495,11 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(unlikely(!(vma->vm_flags&VM_GROWSDOWN)))returnbad_area(regs,address);-/*-*N.B.ThePOWER/OpenABIallowsprogramstoaccessupto-*288bytesbelowthestackpointer.-*Thekernelsignaldeliverycodewritesuptoabout1.5kB-*belowthestackpointer(r1)beforedecrementingit.-*Theexeccodecanwriteslightlyover640kBtothestack-*beforesettingtheuserr1.Thusweallowthestackto-*expandto1MBwithoutfurtherchecks.-*/-if(address+0x100000<vma->vm_end){-/* get user regs even if this fault is in kernel mode */-structpt_regs*uregs=current->thread.regs;-if(uregs==NULL)-returnbad_area(regs,address);+/* The stack is being expanded, check if it's valid */+if(unlikely(bad_stack_expansion(regs,address,vma,store_update_sp)))+returnbad_area(regs,address);-/*-*Auser-modeaccesstoanaddressalongwaybelow-*thestackpointerisonlyvalidiftheinstruction-*isonewhichwouldupdatethestackpointertothe-*addressaccessediftheinstructioncompleted,-*i.e.eitherstwurs,n(r1)orstwuxrs,r1,rb-*(orthebyte,halfword,floatordoubleforms).-*-*Ifwedon'tcheckthisthenanywritetothearea-*betweenthelastmappedregionandthestackwill-*expandthestackratherthansegfaulting.-*/-if(address+2048<uregs->gpr[1]&&!store_update_sp)-returnbad_area(regs,address);-}+/* Try to expand it */if(unlikely(expand_stack(vma,address)))returnbad_area(regs,address);
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:23
Move out the code that sets FAULT_FLAG_WRITE so the block that check
access permissions can be extracted. While at it also set
FAULT_FLAG_INSTRUCTION which will be used for protection keys.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -384,6 +384,10 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(is_user)flags|=FAULT_FLAG_USER;+if(is_write)+flags|=FAULT_FLAG_WRITE;+if(is_exec)+flags|=FAULT_FLAG_INSTRUCTION;/* When running in the kernel we expect faults to occur only to*addressesinuserspace.Allotherfaultsrepresenterrorsinthe
@@ -476,7 +480,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,}elseif(is_write){if(unlikely(!(vma->vm_flags&VM_WRITE)))returnbad_area(regs,address);-flags|=FAULT_FLAG_WRITE;/* a read */}else{if(unlikely(!(vma->vm_flags&(VM_READ|VM_EXEC|VM_WRITE))))
@@ -400,7 +400,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,*source.Ifthisisinvalidwecanskiptheaddressspacecheck,*thusavoidingthedeadlock.*/-if(!down_read_trylock(&mm->mmap_sem)){+if(unlikely(!down_read_trylock(&mm->mmap_sem))){if(!is_user&&!search_exception_tables(regs->nip))returnbad_area_nosemaphore(regs,address);
@@ -416,11 +416,11 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,}vma=find_vma(mm,address);-if(!vma)+if(unlikely(!vma))returnbad_area(regs,address);-if(vma->vm_start<=address)+if(likely(vma->vm_start<=address))gotogood_area;-if(!(vma->vm_flags&VM_GROWSDOWN))+if(unlikely(!(vma->vm_flags&VM_GROWSDOWN)))returnbad_area(regs,address);/*
@@ -453,7 +453,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,if(address+2048<uregs->gpr[1]&&!store_update_sp)returnbad_area(regs,address);}-if(expand_stack(vma,address))+if(unlikely(expand_stack(vma,address)))returnbad_area(regs,address);good_area:
@@ -468,18 +468,18 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,*belowwouldn'tbevalidonthoseprocessors.This-may-*breakprogramscompiledwithareallyoldABIthough.*/-if(!(vma->vm_flags&VM_EXEC)&&-(cpu_has_feature(CPU_FTR_NOEXECUTE)||-!(vma->vm_flags&(VM_READ|VM_WRITE))))+if(unlikely(!(vma->vm_flags&VM_EXEC)&&+(cpu_has_feature(CPU_FTR_NOEXECUTE)||+!(vma->vm_flags&(VM_READ|VM_WRITE)))))returnbad_area(regs,address);/* a write */}elseif(is_write){-if(!(vma->vm_flags&VM_WRITE))+if(unlikely(!(vma->vm_flags&VM_WRITE)))returnbad_area(regs,address);flags|=FAULT_FLAG_WRITE;/* a read */}else{-if(!(vma->vm_flags&(VM_READ|VM_EXEC|VM_WRITE)))+if(unlikely(!(vma->vm_flags&(VM_READ|VM_EXEC|VM_WRITE))))returnbad_area(regs,address);}
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:28
We have a whole pile of unused code to maintain the ACOP register,
allocate coprocessor PIDs and handle ACOP faults. This mechanism
was used for the HFI adapter on POWER7 which is dead and gone and
whose driver never went upstream. It was used on some A2 core based
stuff that also never saw the light of day.
Take out all that code.
There is still some POWER8 coprocessor code that uses icswx but it's
kernel only and thus doesn't use any of that infrastructure.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 5 -
arch/powerpc/include/asm/mmu_context.h | 6 -
arch/powerpc/include/asm/reg_booke.h | 3 -
arch/powerpc/mm/Makefile | 2 -
arch/powerpc/mm/fault.c | 15 --
arch/powerpc/mm/icswx.c | 292 -------------------------------
arch/powerpc/mm/icswx.h | 68 -------
arch/powerpc/mm/icswx_pid.c | 87 ---------
arch/powerpc/mm/mmu_context_book3s64.c | 18 --
arch/powerpc/platforms/Kconfig.cputype | 38 ----
10 files changed, 534 deletions(-)
delete mode 100644 arch/powerpc/mm/icswx.c
delete mode 100644 arch/powerpc/mm/icswx.h
delete mode 100644 arch/powerpc/mm/icswx_pid.c
@@ -97,11 +97,6 @@ typedef struct {#ifdef CONFIG_PPC_SUBPAGE_PROTstructsubpage_prot_tablespt;#endif /* CONFIG_PPC_SUBPAGE_PROT */-#ifdef CONFIG_PPC_ICSWX-structspinlock*cop_lockp;/* guard acop and cop_pid */-unsignedlongacop;/* mask of enabled coprocessor types */-unsignedintcop_pid;/* pid value used with coprocessors */-#endif /* CONFIG_PPC_ICSWX */#ifdef CONFIG_PPC_64K_PAGES/* for 4K PTE fragment support */void*pte_frag;
@@ -107,12 +107,6 @@ static inline void switch_mm_irqs_off(struct mm_struct *prev,if(prev==next)return;-#ifdef CONFIG_PPC_ICSWX-/* Switch coprocessor context only if prev or next uses a coprocessor */-if(prev->context.acop||next->context.acop)-switch_cop(next);-#endif /* CONFIG_PPC_ICSWX */-/* We must stop all altivec streams before changing the HW*context*/
@@ -389,19 +387,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,intfault,major=0;boolstore_update_sp=false;-#ifdef CONFIG_PPC_ICSWX-/*-*weneedtodothisearlybecausethis"data storage-*interrupt" does not update the DAR/DEAR so we don't want to-*lookatit-*/-if(error_code&ICSWX_DSI_UCT){-intrc=acop_handle_fault(regs,address,error_code);-if(rc)-returnrc;-}-#endif /* CONFIG_PPC_ICSWX */-if(notify_page_fault(regs))return0;
@@ -1,292 +0,0 @@-/*- * ICSWX and ACOP Management- *- * Copyright (C) 2011 Anton Blanchard, IBM Corp. <anton@samba.org>- *- * This program is free software; you can redistribute it and/or- * modify it under the terms of the GNU General Public License- * as published by the Free Software Foundation; either version- * 2 of the License, or (at your option) any later version.- *- */--#include <linux/sched.h>-#include <linux/kernel.h>-#include <linux/errno.h>-#include <linux/types.h>-#include <linux/mm.h>-#include <linux/spinlock.h>-#include <linux/module.h>-#include <linux/uaccess.h>--#include "icswx.h"--/*- * The processor and its L2 cache cause the icswx instruction to- * generate a COP_REQ transaction on PowerBus. The transaction has no- * address, and the processor does not perform an MMU access to- * authenticate the transaction. The command portion of the PowerBus- * COP_REQ transaction includes the LPAR_ID (LPID) and the coprocessor- * Process ID (PID), which the coprocessor compares to the authorized- * LPID and PID held in the coprocessor, to determine if the process- * is authorized to generate the transaction. The data of the COP_REQ- * transaction is 128-byte or less in size and is placed in cacheable- * memory on a 128-byte cache line boundary.- *- * The task to use a coprocessor should use use_cop() to mark the use- * of the Coprocessor Type (CT) and context switching. On a server- * class processor, the PID register is used only for coprocessor- * management + * and so a coprocessor PID is allocated before- * executing icswx + * instruction. Drop_cop() is used to free the- * coprocessor PID.- *- * Example:- * Host Fabric Interface (HFI) is a PowerPC network coprocessor.- * Each HFI have multiple windows. Each HFI window serves as a- * network device sending to and receiving from HFI network.- * HFI immediate send function uses icswx instruction. The immediate- * send function allows small (single cache-line) packets be sent- * without using the regular HFI send FIFO and doorbell, which are- * much slower than immediate send.- *- * For each task intending to use HFI immediate send, the HFI driver- * calls use_cop() to obtain a coprocessor PID for the task.- * The HFI driver then allocate a free HFI window and save the- * coprocessor PID to the HFI window to allow the task to use the- * HFI window.- *- * The HFI driver repeatedly creates immediate send packets and- * issues icswx instruction to send data through the HFI window.- * The HFI compares the coprocessor PID in the CPU PID register- * to the PID held in the HFI window to determine if the transaction- * is allowed.- *- * When the task to release the HFI window, the HFI driver calls- * drop_cop() to release the coprocessor PID.- */--void switch_cop(struct mm_struct *next)-{-#ifdef CONFIG_PPC_ICSWX_PID- mtspr(SPRN_PID, next->context.cop_pid);-#endif- mtspr(SPRN_ACOP, next->context.acop);-}--/**- * Start using a coprocessor.- * @acop: mask of coprocessor to be used.- * @mm: The mm the coprocessor to associate with. Most likely current mm.- *- * Return a positive PID if successful. Negative errno otherwise.- * The returned PID will be fed to the coprocessor to determine if an- * icswx transaction is authenticated.- */-int use_cop(unsigned long acop, struct mm_struct *mm)-{- int ret;-- if (!cpu_has_feature(CPU_FTR_ICSWX))- return -ENODEV;-- if (!mm || !acop)- return -EINVAL;-- /* The page_table_lock ensures mm_users won't change under us */- spin_lock(&mm->page_table_lock);- spin_lock(mm->context.cop_lockp);-- ret = get_cop_pid(mm);- if (ret < 0)- goto out;-- /* update acop */- mm->context.acop |= acop;-- sync_cop(mm);-- /*- * If this is a threaded process then there might be other threads- * running. We need to send an IPI to force them to pick up any- * change in PID and ACOP.- */- if (atomic_read(&mm->mm_users) > 1)- smp_call_function(sync_cop, mm, 1);--out:- spin_unlock(mm->context.cop_lockp);- spin_unlock(&mm->page_table_lock);-- return ret;-}-EXPORT_SYMBOL_GPL(use_cop);--/**- * Stop using a coprocessor.- * @acop: mask of coprocessor to be stopped.- * @mm: The mm the coprocessor associated with.- */-void drop_cop(unsigned long acop, struct mm_struct *mm)-{- int free_pid;-- if (!cpu_has_feature(CPU_FTR_ICSWX))- return;-- if (WARN_ON_ONCE(!mm))- return;-- /* The page_table_lock ensures mm_users won't change under us */- spin_lock(&mm->page_table_lock);- spin_lock(mm->context.cop_lockp);-- mm->context.acop &= ~acop;-- free_pid = disable_cop_pid(mm);- sync_cop(mm);-- /*- * If this is a threaded process then there might be other threads- * running. We need to send an IPI to force them to pick up any- * change in PID and ACOP.- */- if (atomic_read(&mm->mm_users) > 1)- smp_call_function(sync_cop, mm, 1);-- if (free_pid != COP_PID_NONE)- free_cop_pid(free_pid);-- spin_unlock(mm->context.cop_lockp);- spin_unlock(&mm->page_table_lock);-}-EXPORT_SYMBOL_GPL(drop_cop);--static int acop_use_cop(int ct)-{- /* There is no alternate policy, yet */- return -1;-}--/*- * Get the instruction word at the NIP- */-static u32 acop_get_inst(struct pt_regs *regs)-{- u32 inst;- u32 __user *p;-- p = (u32 __user *)regs->nip;- if (!access_ok(VERIFY_READ, p, sizeof(*p)))- return 0;-- if (__get_user(inst, p))- return 0;-- return inst;-}--/**- * @regs: registers at time of interrupt- * @address: storage address- * @error_code: Fault code, usually the DSISR or ESR depending on- * processor type- *- * Return 0 if we are able to resolve the data storage fault that- * results from a CT miss in the ACOP register.- */-int acop_handle_fault(struct pt_regs *regs, unsigned long address,- unsigned long error_code)-{- int ct;- u32 inst = 0;-- if (!cpu_has_feature(CPU_FTR_ICSWX)) {- pr_info("No coprocessors available");- _exception(SIGILL, regs, ILL_ILLOPN, address);- }-- if (!user_mode(regs)) {- /* this could happen if the HV denies the- * kernel access, for now we just die */- die("ICSWX from kernel failed", regs, SIGSEGV);- }-- /* Some implementations leave us a hint for the CT */- ct = ICSWX_GET_CT_HINT(error_code);- if (ct < 0) {- /* we have to peek at the instruction word to figure out CT */- u32 ccw;- u32 rs;-- inst = acop_get_inst(regs);- if (inst == 0)- return -1;-- rs = (inst >> (31 - 10)) & 0x1f;- ccw = regs->gpr[rs];- ct = (ccw >> 16) & 0x3f;- }-- /*- * We could be here because another thread has enabled acop- * but the ACOP register has yet to be updated.- *- * This should have been taken care of by the IPI to sync all- * the threads (see smp_call_function(sync_cop, mm, 1)), but- * that could take forever if there are a significant amount- * of threads.- *- * Given the number of threads on some of these systems,- * perhaps this is the best way to sync ACOP rather than whack- * every thread with an IPI.- */- if ((acop_copro_type_bit(ct) & current->active_mm->context.acop) != 0) {- sync_cop(current->active_mm);- return 0;- }-- /* check for alternate policy */- if (!acop_use_cop(ct))- return 0;-- /* at this point the CT is unknown to the system */- pr_warn("%s[%d]: Coprocessor %d is unavailable\n",- current->comm, current->pid, ct);-- /* get inst if we don't already have it */- if (inst == 0) {- inst = acop_get_inst(regs);- if (inst == 0)- return -1;- }-- /* Check if the instruction is the "record form" */- if (inst & 1) {- /*- * the instruction is "record" form so we can reject- * using CR0- */- regs->ccr &= ~(0xful << 28);- regs->ccr |= ICSWX_RC_NOT_FOUND << 28;-- /* Move on to the next instruction */- regs->nip += 4;- } else {- /*- * There is no architected mechanism to report a bad- * CT so we could either SIGILL or report nothing.- * Since the non-record version should only bu used- * for "hints" or "don't care" we should probably do- * nothing. However, I could see how some people- * might want an SIGILL so it here if you want it.- */-#ifdef CONFIG_PPC_ICSWX_USE_SIGILL- _exception(SIGILL, regs, ILL_ILLOPN, address);-#else- regs->nip += 4;-#endif- }-- return 0;-}-EXPORT_SYMBOL_GPL(acop_handle_fault);
@@ -1,68 +0,0 @@-#ifndef _ARCH_POWERPC_MM_ICSWX_H_-#define _ARCH_POWERPC_MM_ICSWX_H_--/*- * ICSWX and ACOP Management- *- * Copyright (C) 2011 Anton Blanchard, IBM Corp. <anton@samba.org>- *- * This program is free software; you can redistribute it and/or- * modify it under the terms of the GNU General Public License- * as published by the Free Software Foundation; either version- * 2 of the License, or (at your option) any later version.- *- */--#include <asm/mmu_context.h>--/* also used to denote that PIDs are not used */-#define COP_PID_NONE 0--static inline void sync_cop(void *arg)-{- struct mm_struct *mm = arg;-- if (mm == current->active_mm)- switch_cop(current->active_mm);-}--#ifdef CONFIG_PPC_ICSWX_PID-extern int get_cop_pid(struct mm_struct *mm);-extern int disable_cop_pid(struct mm_struct *mm);-extern void free_cop_pid(int free_pid);-#else-#define get_cop_pid(m) (COP_PID_NONE)-#define disable_cop_pid(m) (COP_PID_NONE)-#define free_cop_pid(p)-#endif--/*- * These are implementation bits for architected registers. If this- * ever becomes architecture the should be moved to reg.h et. al.- */-/* UCT is the same bit for Server and Embedded */-#define ICSWX_DSI_UCT 0x00004000 /* Unavailable Coprocessor Type */--#ifdef CONFIG_PPC_BOOK3E-/* Embedded implementation gives us no hints as to what the CT is */-#define ICSWX_GET_CT_HINT(x) (-1)-#else-/* Server implementation contains the CT value in the DSISR */-#define ICSWX_DSISR_CTMASK 0x00003f00-#define ICSWX_GET_CT_HINT(x) (((x) & ICSWX_DSISR_CTMASK) >> 8)-#endif--#define ICSWX_RC_STARTED 0x8 /* The request has been started */-#define ICSWX_RC_NOT_IDLE 0x4 /* No coprocessor found idle */-#define ICSWX_RC_NOT_FOUND 0x2 /* No coprocessor found */-#define ICSWX_RC_UNDEFINED 0x1 /* Reserved */--extern int acop_handle_fault(struct pt_regs *regs, unsigned long address,- unsigned long error_code);--static inline u64 acop_copro_type_bit(unsigned int type)-{- return 1ULL << (63 - type);-}--#endif /* !_ARCH_POWERPC_MM_ICSWX_H_ */
@@ -1,87 +0,0 @@-/*- * ICSWX and ACOP/PID Management- *- * Copyright (C) 2011 Anton Blanchard, IBM Corp. <anton@samba.org>- *- * This program is free software; you can redistribute it and/or- * modify it under the terms of the GNU General Public License- * as published by the Free Software Foundation; either version- * 2 of the License, or (at your option) any later version.- *- */--#include <linux/sched.h>-#include <linux/kernel.h>-#include <linux/errno.h>-#include <linux/types.h>-#include <linux/mm.h>-#include <linux/spinlock.h>-#include <linux/idr.h>-#include <linux/module.h>-#include "icswx.h"--#define COP_PID_MIN (COP_PID_NONE + 1)-#define COP_PID_MAX (0xFFFF)--static DEFINE_SPINLOCK(mmu_context_acop_lock);-static DEFINE_IDA(cop_ida);--static int new_cop_pid(struct ida *ida, int min_id, int max_id,- spinlock_t *lock)-{- int index;- int err;--again:- if (!ida_pre_get(ida, GFP_KERNEL))- return -ENOMEM;-- spin_lock(lock);- err = ida_get_new_above(ida, min_id, &index);- spin_unlock(lock);-- if (err == -EAGAIN)- goto again;- else if (err)- return err;-- if (index > max_id) {- spin_lock(lock);- ida_remove(ida, index);- spin_unlock(lock);- return -ENOMEM;- }-- return index;-}--int get_cop_pid(struct mm_struct *mm)-{- int pid;-- if (mm->context.cop_pid == COP_PID_NONE) {- pid = new_cop_pid(&cop_ida, COP_PID_MIN, COP_PID_MAX,- &mmu_context_acop_lock);- if (pid >= 0)- mm->context.cop_pid = pid;- }- return mm->context.cop_pid;-}--int disable_cop_pid(struct mm_struct *mm)-{- int free_pid = COP_PID_NONE;-- if ((!mm->context.acop) && (mm->context.cop_pid != COP_PID_NONE)) {- free_pid = mm->context.cop_pid;- mm->context.cop_pid = COP_PID_NONE;- }- return free_pid;-}--void free_cop_pid(int free_pid)-{- spin_lock(&mmu_context_acop_lock);- ida_remove(&cop_ida, free_pid);- spin_unlock(&mmu_context_acop_lock);-}
@@ -271,44 +271,6 @@ config VSXIfindoubt,sayYhere.-configPPC_ICSWX-bool"Support for PowerPC icswx coprocessor instruction"-depends onPPC_BOOK3S_64-defaultn----help-----ThisoptionenableskernelsupportforthePowerPCInitiate-CoprocessorStoreWord(icswx)coprocessorinstructiononPOWER7-andPOWER8processors.POWER9usesnewcopy/pasteinstructions-toinvokethecoprocessor.--Thisoptionisonlyusefulifyouhaveaprocessorthatsupports-theicswxcoprocessorinstruction.Itdoesnothaveanyeffect-onprocessorswithouttheicswxcoprocessorinstruction.--Thisoptionslightlyincreaseskernelmemoryusage.--Ifindoubt,sayNhere.--configPPC_ICSWX_PID-bool"icswx requires direct PID management"-depends onPPC_ICSWX-defaulty----help----ThePIDregisterinserverisusedexplicitlyforICSWX.In-embeddedsystemsPIDmanagementisdonebythesystem.--configPPC_ICSWX_USE_SIGILL-bool"Should a bad CT cause a SIGILL?"-depends onPPC_ICSWX-defaultn----help----ShouldabadCTusedfor"non-record form ICSWX"causean-illegalinstructionsignalorshoulditbesilentas-architected.--Ifindoubt,sayNhere.-configSPE_POSSIBLEdef_boolydepends onE200||(E500&&!PPC_E500MC)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:31
This has a page of comment explaining what's going on right in
the middle of do_page_fault() which makes things a bit hard to
follow. Move it to a helper instead. Also do the test earlier
as there's no point waiting until after we found the VMA.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 75 +++++++++++++++++++++++++++----------------------
1 file changed, 42 insertions(+), 33 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:37
If the first iteration returns VM_FAULT_MAJOR but the second
one doesn't, we fail to account the fault as a major fault.
This fixes it and brings the code in line with x86.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:57:41
No need to break those lines, they aren't that long
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-19 04:58:01
We currently test for is_exec and DSISR_PROTFAULT but that doesn't
make sense as this is the wrong error bit to test for an execute
permission failure.
In fact, we had code that would return early if we had an exec
fault in kernel mode so I think that was just dead code anyway.
Finally the location of that test is awkward and prevents further
simplifications.
So instead move that test into a helper along with the existing
early test for kernel exec faults and out of range accesses,
and put it all in a "bad_kernel_fault()" helper. While at it
test the correct error bits.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
@@ -180,6 +180,20 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)returnMM_FAULT_CONTINUE;}+/* Is this a bad kernel fault ? */+staticboolbad_kernel_fault(boolis_exec,unsignedlongerror_code,+unsignedlongaddress)+{+if(is_exec&&(error_code&(DSISR_NOEXEC_OR_G|DSISR_KEYFAULT))){+printk_ratelimited(KERN_CRIT"kernel tried to execute"+" exec-protected page (%lx) -"+"exploit attempt? (uid: %d)\n",+address,from_kuid(&init_user_ns,+current_uid()));+}+returnis_exec||(address>=TASK_SIZE);+}+/**Definethecorrect"is_write"bitinerror_codebased*ontheprocessorfamily
@@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,*Thekernelshouldnevertakeanexecutefaultnorshouldit*takeapagefaulttoakerneladdress.*/-if(!is_user&&(is_exec||(address>=TASK_SIZE)))+if(unlikely(!is_user&&bad_kernel_fault(is_exec,error_code,address)))returnSIGSEGV;/* We restore the interrupt state now */
@@ -491,11 +505,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,return0;}-if(is_exec&&(error_code&DSISR_PROTFAULT))-printk_ratelimited(KERN_CRIT"kernel tried to execute NX-protected"-" page (%lx) - exploit attempt? (uid: %d)\n",-address,from_kuid(&init_user_ns,current_uid()));-returnSIGSEGV;}NOKPROBE_SYMBOL(__do_page_fault);
Benjamin Herrenschmidt [off-list ref] a =C3=A9crit=C2=A0:
When hitting below a VM_GROWSDOWN vma (typically growing the stack),
we check whether it's a valid stack-growing instruction and we
check the distance to GPR1. This is largely open coded with lots
of comments, so move it out to a helper.
Did you have a look at the following patch ? It's been waiting for=20=20
application=20for some weeks now.=20=20
https://patchwork.ozlabs.org/patch/771869
It=20limits number of calls to get_user()
Can you have a look and merge it with your serie ?
=20pt_regs *regs)
* Check whether the instruction at regs->nip is a store using
* an update addressing form which will update r1.
*/
-static int store_updates_sp(struct pt_regs *regs)
+static bool store_updates_sp(struct pt_regs *regs)
{
unsigned int inst;
if (get_user(inst, (unsigned int __user *)regs->nip))
- return 0;
+ return false;
/* check for 1 in the rA field */
if (((inst >> 16) & 0x1f) !=3D 1)
- return 0;
+ return false;
/* check major opcode */
switch (inst >> 26) {
case 37: /* stwu */
@@ -87,7 +87,7 @@ static int store_updates_sp(struct pt_regs *regs) case 45: /* sthu */ case 53: /* stfsu */ case 55: /* stfdu */- return 1;+ return true; case 62: /* std or stdu */ return (inst & 3) =3D=3D 1; case 31:
=20unsigned long error_code,
return is_exec || (address >=3D TASK_SIZE);
}
+static bool bad_stack_expansion(struct pt_regs *regs, unsigned long addr=
ess,
quoted hunk
+ struct vm_area_struct *vma,
+ bool store_update_sp)
+{
+ /*
+ * N.B. The POWER/Open ABI allows programs to access up to
+ * 288 bytes below the stack pointer.
+ * The kernel signal delivery code writes up to about 1.5kB
+ * below the stack pointer (r1) before decrementing it.
+ * The exec code can write slightly over 640kB to the stack
+ * before setting the user r1. Thus we allow the stack to
+ * expand to 1MB without further checks.
+ */
+ if (address + 0x100000 < vma->vm_end) {
+ /* get user regs even if this fault is in kernel mode */
+ struct pt_regs *uregs =3D current->thread.regs;
+ if (uregs =3D=3D NULL)
+ return true;
+
+ /*
+ * A user-mode access to an address a long way below
+ * the stack pointer is only valid if the instruction
+ * is one which would update the stack pointer to the
+ * address accessed if the instruction completed,
+ * i.e. either stwu rs,n(r1) or stwux rs,r1,rb
+ * (or the byte, halfword, float or double forms).
+ *
+ * If we don't check this then any write to the area
+ * between the last mapped region and the stack will
+ * expand the stack rather than segfaulting.
+ */
+ if (address + 2048 < uregs->gpr[1] && !store_update_sp)
+ return true;
+ }
+ return false;
+}
+
static bool access_error(bool is_write, bool is_exec,
struct vm_area_struct *vma)
{
@@ -350,7 +387,7 @@ static int __do_page_fault(struct pt_regs *regs,=20=
=20
quoted hunk
=20unsigned long address,
int is_user =3D user_mode(regs);
int is_write =3D page_fault_is_write(error_code);
int fault, major =3D 0;
- int store_update_sp =3D 0;
+ bool store_update_sp =3D false;
#ifdef CONFIG_PPC_ICSWX
/*
@@ -458,36 +495,11 @@ static int __do_page_fault(struct pt_regs=20=20
=20*regs, unsigned long address,
if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)))
return bad_area(regs, address);
- /*
- * N.B. The POWER/Open ABI allows programs to access up to
- * 288 bytes below the stack pointer.
- * The kernel signal delivery code writes up to about 1.5kB
- * below the stack pointer (r1) before decrementing it.
- * The exec code can write slightly over 640kB to the stack
- * before setting the user r1. Thus we allow the stack to
- * expand to 1MB without further checks.
- */
- if (address + 0x100000 < vma->vm_end) {
- /* get user regs even if this fault is in kernel mode */
- struct pt_regs *uregs =3D current->thread.regs;
- if (uregs =3D=3D NULL)
- return bad_area(regs, address);
+ /* The stack is being expanded, check if it's valid */
+ if (unlikely(bad_stack_expansion(regs, address, vma, store_update_sp)))
+ return bad_area(regs, address);
- /*
- * A user-mode access to an address a long way below
- * the stack pointer is only valid if the instruction
- * is one which would update the stack pointer to the
- * address accessed if the instruction completed,
- * i.e. either stwu rs,n(r1) or stwux rs,r1,rb
- * (or the byte, halfword, float or double forms).
- *
- * If we don't check this then any write to the area
- * between the last mapped region and the stack will
- * expand the stack rather than segfaulting.
- */
- if (address + 2048 < uregs->gpr[1] && !store_update_sp)
- return bad_area(regs, address);
- }
+ /* Try to expand it */
if (unlikely(expand_stack(vma, address)))
return bad_area(regs, address);
--
2.13.3
Benjamin Herrenschmidt [off-list ref] a =C3=A9crit=C2=A0:
quoted hunk
Define a common page_fault_is_write() helper and use it
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
@@ -183,6 +183,16 @@ static int mm_fault_error(struct pt_regs *regs,=20=
=20
=20unsigned long addr, int fault)
}
/*
+ * Define the correct "is_write" bit in error_code based
+ * on the processor family
+ */
+#if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
+#define page_fault_is_write(__err) ((__err) & ESR_DST)
+#else
+#define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
+#endif
Doesn't linux kernel codying style make preference to static inline=20=20
functions=20instead of macros ?
+
+/*
* For 600- and 800-family processors, the error_code parameter is DSISR
* for a data fault, SRR1 for an instruction fault. For 400-family=20=20
=20processors
* the error_code parameter is ESR for a data fault, 0 for an instructio=
n
quoted hunk
@@ -202,18 +212,12 @@ static int __do_page_fault(struct pt_regs=20=20
=20*regs, unsigned long address,
struct mm_struct *mm =3D current->mm;
unsigned int flags =3D FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
int code =3D SEGV_MAPERR;
- int is_write =3D 0;
int is_exec =3D TRAP(regs) =3D=3D 0x400;
int is_user =3D user_mode(regs);
+ int is_write =3D page_fault_is_write(error_code);
int fault;
int rc =3D 0, store_update_sp =3D 0;
-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
- is_write =3D error_code & DSISR_ISSTORE;
-#else
- is_write =3D error_code & ESR_DST;
-#endif /* CONFIG_4xx || CONFIG_BOOKE */
-
#ifdef CONFIG_PPC_ICSWX
/*
* we need to do this early because this "data storage
--
2.13.3
Benjamin Herrenschmidt [off-list ref] a =C3=A9crit=C2=A0:
quoted hunk
By filtering the relevant SRR1 bits in the assembly rather than
in do_page_fault() itself, we avoid a conditional branch (since we
already come from different path for data and instruction faults).
This will allow more simplifications later
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/head_32.S | 2 +-
arch/powerpc/kernel/head_8xx.S | 4 ++--
arch/powerpc/mm/fault.c | 14 ++------------
3 files changed, 5 insertions(+), 15 deletions(-)
@@ -203,23 +203,13 @@ static int __do_page_fault(struct pt_regs=20=20
=20*regs, unsigned long address,
unsigned int flags =3D FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
int code =3D SEGV_MAPERR;
int is_write =3D 0;
- int trap =3D TRAP(regs);
- int is_exec =3D trap =3D=3D 0x400;
+ int is_exec =3D TRAP(regs) =3D=3D 0x400;
Don't we have a tab/space issue here ?
int is_user =3D user_mode(regs);
int fault;
int rc =3D 0, store_update_sp =3D 0;
#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
- /*
- * Fortunately the bit assignments in SRR1 for an instruction
- * fault and DSISR for a data fault are mostly the same for the
- * bits we are interested in. But there are some bits which
- * indicate errors in DSISR but can validly be set in SRR1.
- */
- if (is_exec)
- error_code &=3D 0x48200000;
- else
- is_write =3D error_code & DSISR_ISSTORE;
+ is_write =3D error_code & DSISR_ISSTORE;
#else
is_write =3D error_code & ESR_DST;
#endif /* CONFIG_4xx || CONFIG_BOOKE */
--
2.13.3
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-23 01:07:07
On Sat, 2017-07-22 at 18:40 +0200, LEROY Christophe wrote:
Benjamin Herrenschmidt [off-list ref] a écrit :
quoted
Define a common page_fault_is_write() helper and use it
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
@@ -183,6 +183,16 @@ static int mm_fault_error(struct pt_regs *regs,
unsigned long addr, int fault)
}
/*
+ * Define the correct "is_write" bit in error_code based
+ * on the processor family
+ */
+#if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
+#define page_fault_is_write(__err) ((__err) & ESR_DST)
+#else
+#define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
+#endif
Doesn't linux kernel codying style make preference to static inline
functions instead of macros ?
Doesn't really matter, yes that could be, but then I would have
to add a !! to make them proper bools I think unless I keep things
as int, etc... not sure how well gcc will do with it.
quoted
+
+/*
* For 600- and 800-family processors, the error_code parameter is DSISR
* for a data fault, SRR1 for an instruction fault. For 400-family
processors
* the error_code parameter is ESR for a data fault, 0 for an instruction
@@ -202,18 +212,12 @@ static int __do_page_fault(struct pt_regs
*regs, unsigned long address,
struct mm_struct *mm = current->mm;
unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
int code = SEGV_MAPERR;
- int is_write = 0;
int is_exec = TRAP(regs) == 0x400;
int is_user = user_mode(regs);
+ int is_write = page_fault_is_write(error_code);
int fault;
int rc = 0, store_update_sp = 0;
-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
- is_write = error_code & DSISR_ISSTORE;
-#else
- is_write = error_code & ESR_DST;
-#endif /* CONFIG_4xx || CONFIG_BOOKE */
-
#ifdef CONFIG_PPC_ICSWX
/*
* we need to do this early because this "data storage
--
2.13.3
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-07-23 01:10:32
On Sat, 2017-07-22 at 18:43 +0200, LEROY Christophe wrote:
quoted
@@ -203,23 +203,13 @@ static int __do_page_fault(struct pt_regs
*regs, unsigned long address,
unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
int code = SEGV_MAPERR;
int is_write = 0;
- int trap = TRAP(regs);
- int is_exec = trap == 0x400;
+ int is_exec = TRAP(regs) == 0x400;
Don't we have a tab/space issue here ?
There seem to be indeed an extra space before the tab at the beginning
of the line, though it looks like it was already there in the orignal
code :-) I didn't notice it and thus didn't fix it. If I respin I'll
take care of it.
quoted
int is_user = user_mode(regs);
int fault;
int rc = 0, store_update_sp = 0;
#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
- /*
- * Fortunately the bit assignments in SRR1 for an instruction
- * fault and DSISR for a data fault are mostly the same for the
- * bits we are interested in. But there are some bits which
- * indicate errors in DSISR but can validly be set in SRR1.
- */
- if (is_exec)
- error_code &= 0x48200000;
- else
- is_write = error_code & DSISR_ISSTORE;
+ is_write = error_code & DSISR_ISSTORE;
#else
is_write = error_code & ESR_DST;
#endif /* CONFIG_4xx || CONFIG_BOOKE */
--
2.13.3
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-07-24 10:47:04
LEROY Christophe [off-list ref] writes:
Benjamin Herrenschmidt [off-list ref] a =C3=A9crit=C2=A0:
quoted
When hitting below a VM_GROWSDOWN vma (typically growing the stack),
we check whether it's a valid stack-growing instruction and we
check the distance to GPR1. This is largely open coded with lots
of comments, so move it out to a helper.
I actually merged it last merge window, but found I had no good way to
test it, so I took it out again until I can write a test case for it.
The way I realised it wasn't being tested was by removing all the
store_updates_sp logic entirely and having my system run happily for
several days :}
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-07-24 11:58:06
LEROY Christophe [off-list ref] writes:
Benjamin Herrenschmidt [off-list ref] a =C3=A9crit=C2=A0:
quoted
Define a common page_fault_is_write() helper and use it
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
@@ -183,6 +183,16 @@ static int mm_fault_error(struct pt_regs *regs,=20=
=20
quoted
unsigned long addr, int fault)
}
/*
+ * Define the correct "is_write" bit in error_code based
+ * on the processor family
+ */
+#if (defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
+#define page_fault_is_write(__err) ((__err) & ESR_DST)
+#else
+#define page_fault_is_write(__err) ((__err) & DSISR_ISSTORE)
+#endif
Doesn't linux kernel codying style make preference to static inline=20=20
functions instead of macros ?
In general yes. Especially for things that look like functions.
Ben was worried the code gen would be worse if it was a static inline
returning bool, I'll apply it and have a look.
cheers
@@ -203,23 +203,13 @@ static int __do_page_fault(struct pt_regs=20=20
*regs, unsigned long address,
unsigned int flags =3D FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
int code =3D SEGV_MAPERR;
int is_write =3D 0;
- int trap =3D TRAP(regs);
- int is_exec =3D trap =3D=3D 0x400;
+ int is_exec =3D TRAP(regs) =3D=3D 0x400;
Don't we have a tab/space issue here ?
We do:
Warnings for commit 0f9b89d759ba 'powerpc/mm: Pre-filter SRR1 bits before=
do_page_fault()'
Lines beginning with space in arch/powerpc/mm/fault.c
+ int is_exec =3D TRAP(regs) =3D=3D 0x400;
I can fix it up.
cheers
Michael Ellerman [off-list ref] a =C3=A9crit=C2=A0:
LEROY Christophe [off-list ref] writes:
quoted
Benjamin Herrenschmidt [off-list ref] a =C3=A9crit=C2=A0:
quoted
When hitting below a VM_GROWSDOWN vma (typically growing the stack),
we check whether it's a valid stack-growing instruction and we
check the distance to GPR1. This is largely open coded with lots
of comments, so move it out to a helper.
I actually merged it last merge window, but found I had no good way to
test it, so I took it out again until I can write a test case for it.
The way I realised it wasn't being tested was by removing all the
store_updates_sp logic entirely and having my system run happily for
several days :}
Which demonstrates how unlikely this is, hence doing that get_user()=20=20
at=20every fault is waste of time.
How do you plan to handle that in parralele to ben's serie ?
I'll be back from vacation next week and may help finding a way to=20=20
test=20that. (A test program using alloca() ?)
Christophe
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-07-25 11:19:50
LEROY Christophe [off-list ref] writes:
Michael Ellerman [off-list ref] a =C3=A9crit=C2=A0:
quoted
LEROY Christophe [off-list ref] writes:
quoted
Benjamin Herrenschmidt [off-list ref] a =C3=A9crit=C2=A0:
quoted
When hitting below a VM_GROWSDOWN vma (typically growing the stack),
we check whether it's a valid stack-growing instruction and we
check the distance to GPR1. This is largely open coded with lots
of comments, so move it out to a helper.
I actually merged it last merge window, but found I had no good way to
test it, so I took it out again until I can write a test case for it.
The way I realised it wasn't being tested was by removing all the
store_updates_sp logic entirely and having my system run happily for
several days :}
Which demonstrates how unlikely this is, hence doing that get_user()=20=20
at every fault is waste of time.
Yes I agree.
How do you plan to handle that in parralele to ben's serie ?
Not sure :)
I'll be back from vacation next week and may help finding a way to=20=20
test that. (A test program using alloca() ?)
I was thinking hand-crafted asm, but that might be a pain to get working
for 32 & 64-bit, in which case alloca() might work.
cheers
When hitting below a VM_GROWSDOWN vma (typically growing the stack),
we check whether it's a valid stack-growing instruction and we
check the distance to GPR1. This is largely open coded with lots
of comments, so move it out to a helper.
I actually merged it last merge window, but found I had no good way to
test it, so I took it out again until I can write a test case for it.
The way I realised it wasn't being tested was by removing all the
store_updates_sp logic entirely and having my system run happily for
several days :}
Which demonstrates how unlikely this is, hence doing that get_user()
at every fault is waste of time.
Yes I agree.
quoted
How do you plan to handle that in parralele to ben's serie ?
Not sure :)
quoted
I'll be back from vacation next week and may help finding a way to
test that. (A test program using alloca() ?)
I was thinking hand-crafted asm, but that might be a pain to get working
for 32 & 64-bit, in which case alloca() might work.
No need of very sofisticated thing indeed.
The following app makes the trick. If I modify store_updates_sp() to
always return 0, the app gets a SIGSEGV.
#include <stdlib.h>
#include <stdio.h>
int main(int argc, char **argv)
{
char buf[1024 * 1025];
sprintf(buf, "Hello world !\n");
printf(buf);
exit(0);
}
Christophe
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2017-08-03 00:19:12
Benjamin Herrenschmidt [off-list ref] writes:
quoted hunk
On legacy 6xx 32-bit procesors, we checked for the DABR match bit
in DSISR from do_page_fault(), in the middle of a pile of ifdef's
because all other CPU types do it in assembly prior to calling
do_page_fault. Fix that.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/entry_32.S | 11 +++++++++++
arch/powerpc/mm/fault.c | 9 ---------
2 files changed, 11 insertions(+), 9 deletions(-)
This breaks ~35% of the defconfigs.
eg. ppc44x_defconfig
http://kisskb.ellerman.id.au/kisskb/buildresult/13113408/
arch/powerpc/kernel/entry_32.S:(.text+0x7ac): undefined reference to `do_break'
For now I've just wrapped the asm above in:
+#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE) || defined(CONFIG_PPC_8xx))
But would be happy if there was something less gross.
cheers
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2017-08-03 01:51:11
On Thu, 2017-08-03 at 10:19 +1000, Michael Ellerman wrote:
arch/powerpc/kernel/entry_32.S:(.text+0x7ac): undefined reference to `do_break'
For now I've just wrapped the asm above in:
+#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE) || defined(CONFIG_PPC_8xx))
But would be happy if there was something less gross.
Boot failure on the 8xx:
[ 6.029556] Failed to execute /init (error -14)
[ 6.034623] Starting init: /bin/sh exists but couldn't execute it
(error -14)
[ 6.041489] Kernel panic - not syncing: No working init found. Try
passing init= option to kernel. See Linux
Documentation/admin-guide/init.rst for guidance.
[ 6.055518] CPU: 0 PID: 1 Comm: init Not tainted
4.13.0-rc3-s3k-dev-00143-g7aa62e972a56 #56
[ 6.063745] Call Trace:
[ 6.066224] [c60f1ed0] [c001a624] panic+0x108/0x250 (unreliable)
[ 6.072140] [c60f1f30] [c0002640] rootfs_mount+0x0/0x58
[ 6.077311] [c60f1f40] [c000cb80] ret_from_kernel_thread+0x5c/0x64
[ 6.083405] Rebooting in 180 seconds..
Bisected to c433ec0455f921eaf8dd0262a718ce6f8ad62ea2 ("powerpc/mm:
Pre-filter SRR1 bits before do_page_fault()")
Sorry about that. I don't have a way to test 8xx.
Looks like I concluded too quickly yesterday night, indeed the above
commit is the last good one. The faulty one is
d300627c6a53693fb01479b59b0cdd293761b1fa("powerpc/6xx: Handle DABR match
before calling do_page_fault")
Boot failure on the 8xx:
[ 6.029556] Failed to execute /init (error -14)
[ 6.034623] Starting init: /bin/sh exists but couldn't execute it
(error -14)
[ 6.041489] Kernel panic - not syncing: No working init found. Try
passing init=3D option to kernel. See Linux
Documentation/admin-guide/init.rst for guidance.
[ 6.055518] CPU: 0 PID: 1 Comm: init Not tainted
4.13.0-rc3-s3k-dev-00143-g7aa62e972a56 #56
[ 6.063745] Call Trace:
[ 6.066224] [c60f1ed0] [c001a624] panic+0x108/0x250 (unreliable)
[ 6.072140] [c60f1f30] [c0002640] rootfs_mount+0x0/0x58
[ 6.077311] [c60f1f40] [c000cb80] ret_from_kernel_thread+0x5c/0x64
[ 6.083405] Rebooting in 180 seconds..
Bisected to c433ec0455f921eaf8dd0262a718ce6f8ad62ea2 ("powerpc/mm:
Pre-filter SRR1 bits before do_page_fault()")
=20
Sorry about that. I don't have a way to test 8xx.
Looks like I concluded too quickly yesterday night, indeed the above=20
commit is the last good one. The faulty one is=20
d300627c6a53693fb01479b59b0cdd293761b1fa("powerpc/6xx: Handle DABR match=
Hi Ben,
I have an issue on the 8xx with this change
Le 19/07/2017 à 06:49, Benjamin Herrenschmidt a écrit :
We currently test for is_exec and DSISR_PROTFAULT but that doesn't
make sense as this is the wrong error bit to test for an execute
permission failure.
On the 8xx, on an exec permission failure, this is the correct BIT, see
below extract from reference manual:
Note that only one of bits 1, 3, and 4 will be set.
1 1 if the translation of an attempted access is not in the translation
tables. Otherwise 0
3 1 if the fetch access was to guarded memory when MSR[IR] = 1. Otherwise 0
4 1 if the access is not permitted by the protection mechanism; otherwise 0.
So on the 8xx, bit 3 is not DSISR_NOEXEC_OR_G but only DSISR_G.
When the PPP bits are set to No-Execute, we really get bit 4 that is
DSISR_PROTFAULT.
quoted hunk
In fact, we had code that would return early if we had an exec
fault in kernel mode so I think that was just dead code anyway.
Finally the location of that test is awkward and prevents further
simplifications.
So instead move that test into a helper along with the existing
early test for kernel exec faults and out of range accesses,
and put it all in a "bad_kernel_fault()" helper. While at it
test the correct error bits.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/mm/fault.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
@@ -180,6 +180,20 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr, int fault)returnMM_FAULT_CONTINUE;}+/* Is this a bad kernel fault ? */+staticboolbad_kernel_fault(boolis_exec,unsignedlongerror_code,+unsignedlongaddress)+{+if(is_exec&&(error_code&(DSISR_NOEXEC_OR_G|DSISR_KEYFAULT))){
Do you mind if we had DSISR_PROTFAULT here as well ?
Christophe
quoted hunk
+ printk_ratelimited(KERN_CRIT "kernel tried to execute"
+ " exec-protected page (%lx) -"
+ "exploit attempt? (uid: %d)\n",
+ address, from_kuid(&init_user_ns,
+ current_uid()));
+ }
+ return is_exec || (address >= TASK_SIZE);
+}
+
/*
* Define the correct "is_write" bit in error_code based
* on the processor family
@@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, * The kernel should never take an execute fault nor should it * take a page fault to a kernel address. */- if (!is_user && (is_exec || (address >= TASK_SIZE)))+ if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address))) return SIGSEGV; /* We restore the interrupt state now */
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2018-11-07 10:42:03
On Wed, 2018-11-07 at 09:35 +0100, Christophe LEROY wrote:
Hi Ben,
I have an issue on the 8xx with this change
Ah ouch...
.../...
quoted
+/* Is this a bad kernel fault ? */
+static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
+ unsigned long address)
+{
+ if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT))) {
Do you mind if we had DSISR_PROTFAULT here as well ?
Off the top of my mind, I don't see a problem with that... but it would
definitely require an explanation comment.
quoted
+ printk_ratelimited(KERN_CRIT "kernel tried to execute"
+ " exec-protected page (%lx) -"
+ "exploit attempt? (uid: %d)\n",
+ address, from_kuid(&init_user_ns,
+ current_uid()));
+ }
+ return is_exec || (address >= TASK_SIZE);
+}
+
/*
* Define the correct "is_write" bit in error_code based
* on the processor family
@@ -252,7 +266,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, * The kernel should never take an execute fault nor should it * take a page fault to a kernel address. */- if (!is_user && (is_exec || (address >= TASK_SIZE)))+ if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address))) return SIGSEGV; /* We restore the interrupt state now */