| Message ID | 20170217011959.26754-1-stephen.boyd@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Stephen, On 17/02/17 01:19, Stephen Boyd wrote: > If a page is marked read only we should print out that fact, > instead of printing out that there was a page fault. Right now we > get a cryptic error message that something went wrong with an > unhandled fault, but we don't evaluate the esr to figure out that > it was a read/write permission fault. > > Instead of seeing: > > Unable to handle kernel paging request at virtual address ffff000008e460d8 > pgd = ffff800003504000 > [ffff000008e460d8] *pgd=0000000083473003, *pud=0000000083503003, *pmd=0000000000000000 > Internal error: Oops: 9600004f [#1] PREEMPT SMP > > we'll see: > > Unable to handle kernel write to read-only memory at virtual address ffff000008e760d8 > pgd = ffff80003d3de000 > [ffff000008e760d8] *pgd=0000000083472003, *pud=0000000083435003, *pmd=0000000000000000 > Internal error: Oops: 9600004f [#1] PREEMPT SMP This looks like a good idea.. > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 156169c6981b..8bd4e7f11c70 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > /* > * The kernel tried to access some page that wasn't present. > */ > static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, > unsigned int esr, struct pt_regs *regs) > { > + const char *msg; > /* > * Are we prepared to handle this kernel fault? > * We are almost certainly not prepared to handle instruction faults. > @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, > * No handler, we'll have to terminate things with extreme prejudice. > */ > bust_spinlocks(1); > - pr_alert("Unable to handle kernel %s at virtual address %08lx\n", > - (addr < PAGE_SIZE) ? "NULL pointer dereference" : > - "paging request", addr); > + > + if (is_permission_fault(esr, regs)) { is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this is because it assumes software-PAN is relevant. The corner case is when the kernel accesses TTBR1-mapped memory while software-PAN happens to have swivelled TTBR0. Translation faults will be matched by is_permission_fault(), but permission faults won't. Juggling is_permission_fault() to look something like: ---%<--- if (fsc_type == ESR_ELx_FSC_PERM) return true; if (addr < USER_DS && system_uses_ttbr0_pan()) return fsc_type == ESR_ELx_FSC_FAULT && (regs->pstate & PSR_PAN_BIT); return false; ---%<--- ... should fix this. > + if (esr & ESR_ELx_WNR) > + msg = "write to read-only memory"; > + else > + msg = "read from unreadable memory"; > + } else if (addr < PAGE_SIZE) > + msg = "NULL pointer dereference"; > + else > + msg = "paging request"; Nit: {} all the way down! > + > + pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg, > + addr); > > show_pte(mm, addr); > die("Oops", regs, esr); > @@ -269,21 +295,6 @@ static int __do_page_fault(struct mm_struct *mm, unsigned long addr, > return fault; > } > -static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs) > -{ > - unsigned int ec = ESR_ELx_EC(esr); > - unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE; > - > - if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR) > - return false; > - > - if (system_uses_ttbr0_pan()) > - return fsc_type == ESR_ELx_FSC_FAULT && > - (regs->pstate & PSR_PAN_BIT); > - else > - return fsc_type == ESR_ELx_FSC_PERM; > -} Thanks! James
Hi Stephen, On 17/02/17 15:53, Stephen Boyd wrote: > Quoting James Morse (2017-02-17 03:00:39) >> On 17/02/17 01:19, Stephen Boyd wrote: >>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >>> index 156169c6981b..8bd4e7f11c70 100644 >>> --- a/arch/arm64/mm/fault.c >>> +++ b/arch/arm64/mm/fault.c >>> @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, >>> * No handler, we'll have to terminate things with extreme prejudice. >>> */ >>> bust_spinlocks(1); >>> - pr_alert("Unable to handle kernel %s at virtual address %08lx\n", >>> - (addr < PAGE_SIZE) ? "NULL pointer dereference" : >>> - "paging request", addr); >>> + >>> + if (is_permission_fault(esr, regs)) { >> >> is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this >> is because it assumes software-PAN is relevant. >> >> The corner case is when the kernel accesses TTBR1-mapped memory while >> software-PAN happens to have swivelled TTBR0. Translation faults will be matched >> by is_permission_fault(), but permission faults won't. > > If I understand correctly, and I most definitely don't because there are > quite a few combinations, you're saying that __do_kernel_fault() could > be called if the kernel attempts to access some userspace address with > software PAN? That won't be caught in do_page_fault() with the previous > is_permission_fault() check? You're right the user-address side of things will get caught in do_page_fault(). I was trying to badly-explain 'is_permission_fault(esr)' isn't as general purpose as its name and prototype suggest, it only gives the results that the PAN checks expect when called with a user address. >> Juggling is_permission_fault() to look something like: >> ---%<--- >> if (fsc_type == ESR_ELx_FSC_PERM) >> return true; >> >> if (addr < USER_DS && system_uses_ttbr0_pan()) >> return fsc_type == ESR_ELx_FSC_FAULT && >> (regs->pstate & PSR_PAN_BIT); >> >> return false; >> ---%<--- >> ... should fix this. > > But we don't need to check ec anymore? Sorry, I was being sloppy, something like the above could replace the if/else block at the end of is_permission_fault(). You're right we still need the ec check! Thanks, James
Quoting James Morse (2017-02-20 03:10:10) > Hi Stephen, > > On 17/02/17 15:53, Stephen Boyd wrote: > > Quoting James Morse (2017-02-17 03:00:39) > >> On 17/02/17 01:19, Stephen Boyd wrote: > >>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > >>> index 156169c6981b..8bd4e7f11c70 100644 > >>> --- a/arch/arm64/mm/fault.c > >>> +++ b/arch/arm64/mm/fault.c > >>> @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, > >>> * No handler, we'll have to terminate things with extreme prejudice. > >>> */ > >>> bust_spinlocks(1); > >>> - pr_alert("Unable to handle kernel %s at virtual address %08lx\n", > >>> - (addr < PAGE_SIZE) ? "NULL pointer dereference" : > >>> - "paging request", addr); > >>> + > >>> + if (is_permission_fault(esr, regs)) { > >> > >> is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this > >> is because it assumes software-PAN is relevant. > >> > >> The corner case is when the kernel accesses TTBR1-mapped memory while > >> software-PAN happens to have swivelled TTBR0. Translation faults will be matched > >> by is_permission_fault(), but permission faults won't. > > > > If I understand correctly, and I most definitely don't because there are > > quite a few combinations, you're saying that __do_kernel_fault() could > > be called if the kernel attempts to access some userspace address with > > software PAN? That won't be caught in do_page_fault() with the previous > > is_permission_fault() check? > > You're right the user-address side of things will get caught in do_page_fault(). > I was trying to badly-explain 'is_permission_fault(esr)' isn't as general > purpose as its name and prototype suggest, it only gives the results that the > PAN checks expect when called with a user address. Ok. I'd rather not change the function in this patch because I'm only moving the code around to use it higher up in the file. But if you prefer I can combine the code movement with the addition of a new 'addr' argument to this function and rework things based on that.
Hi Stephen, On Fri, Feb 24, 2017 at 05:39:08PM -0800, Stephen Boyd wrote: > Quoting James Morse (2017-02-20 03:10:10) > > On 17/02/17 15:53, Stephen Boyd wrote: > > > Quoting James Morse (2017-02-17 03:00:39) > > >> On 17/02/17 01:19, Stephen Boyd wrote: > > >>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > >>> index 156169c6981b..8bd4e7f11c70 100644 > > >>> --- a/arch/arm64/mm/fault.c > > >>> +++ b/arch/arm64/mm/fault.c > > >>> @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, > > >>> * No handler, we'll have to terminate things with extreme prejudice. > > >>> */ > > >>> bust_spinlocks(1); > > >>> - pr_alert("Unable to handle kernel %s at virtual address %08lx\n", > > >>> - (addr < PAGE_SIZE) ? "NULL pointer dereference" : > > >>> - "paging request", addr); > > >>> + > > >>> + if (is_permission_fault(esr, regs)) { > > >> > > >> is_permission_fault() was previously guarded with a 'addr<USER_DS' check, this > > >> is because it assumes software-PAN is relevant. > > >> > > >> The corner case is when the kernel accesses TTBR1-mapped memory while > > >> software-PAN happens to have swivelled TTBR0. Translation faults will be matched > > >> by is_permission_fault(), but permission faults won't. > > > > > > If I understand correctly, and I most definitely don't because there are > > > quite a few combinations, you're saying that __do_kernel_fault() could > > > be called if the kernel attempts to access some userspace address with > > > software PAN? That won't be caught in do_page_fault() with the previous > > > is_permission_fault() check? > > > > You're right the user-address side of things will get caught in do_page_fault(). > > I was trying to badly-explain 'is_permission_fault(esr)' isn't as general > > purpose as its name and prototype suggest, it only gives the results that the > > PAN checks expect when called with a user address. > > Ok. I'd rather not change the function in this patch because I'm only > moving the code around to use it higher up in the file. But if you > prefer I can combine the code movement with the addition of a new 'addr' > argument to this function and rework things based on that. Are you planning to send a v3 of this? Will
Quoting Will Deacon (2017-03-23 07:22:39) > On Fri, Feb 24, 2017 at 05:39:08PM -0800, Stephen Boyd wrote: > > Quoting James Morse (2017-02-20 03:10:10) > > > > > > You're right the user-address side of things will get caught in do_page_fault(). > > > I was trying to badly-explain 'is_permission_fault(esr)' isn't as general > > > purpose as its name and prototype suggest, it only gives the results that the > > > PAN checks expect when called with a user address. > > > > Ok. I'd rather not change the function in this patch because I'm only > > moving the code around to use it higher up in the file. But if you > > prefer I can combine the code movement with the addition of a new 'addr' > > argument to this function and rework things based on that. > > Are you planning to send a v3 of this? > Sorry for the late reply. I was hoping that James would indicate preference one way or the other. I suppose no reply means "yes" here, so I'll go ahead and fold everything together into one patch and resend.
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 156169c6981b..8bd4e7f11c70 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -160,12 +160,28 @@ static bool is_el1_instruction_abort(unsigned int esr) return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_CUR; } +static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs) +{ + unsigned int ec = ESR_ELx_EC(esr); + unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE; + + if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR) + return false; + + if (system_uses_ttbr0_pan()) + return fsc_type == ESR_ELx_FSC_FAULT && + (regs->pstate & PSR_PAN_BIT); + else + return fsc_type == ESR_ELx_FSC_PERM; +} + /* * The kernel tried to access some page that wasn't present. */ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, unsigned int esr, struct pt_regs *regs) { + const char *msg; /* * Are we prepared to handle this kernel fault? * We are almost certainly not prepared to handle instruction faults. @@ -177,9 +193,19 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, * No handler, we'll have to terminate things with extreme prejudice. */ bust_spinlocks(1); - pr_alert("Unable to handle kernel %s at virtual address %08lx\n", - (addr < PAGE_SIZE) ? "NULL pointer dereference" : - "paging request", addr); + + if (is_permission_fault(esr, regs)) { + if (esr & ESR_ELx_WNR) + msg = "write to read-only memory"; + else + msg = "read from unreadable memory"; + } else if (addr < PAGE_SIZE) + msg = "NULL pointer dereference"; + else + msg = "paging request"; + + pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg, + addr); show_pte(mm, addr); die("Oops", regs, esr); @@ -269,21 +295,6 @@ static int __do_page_fault(struct mm_struct *mm, unsigned long addr, return fault; } -static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs) -{ - unsigned int ec = ESR_ELx_EC(esr); - unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE; - - if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR) - return false; - - if (system_uses_ttbr0_pan()) - return fsc_type == ESR_ELx_FSC_FAULT && - (regs->pstate & PSR_PAN_BIT); - else - return fsc_type == ESR_ELx_FSC_PERM; -} - static bool is_el0_instruction_abort(unsigned int esr) { return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW;
If a page is marked read only we should print out that fact, instead of printing out that there was a page fault. Right now we get a cryptic error message that something went wrong with an unhandled fault, but we don't evaluate the esr to figure out that it was a read/write permission fault. Instead of seeing: Unable to handle kernel paging request at virtual address ffff000008e460d8 pgd = ffff800003504000 [ffff000008e460d8] *pgd=0000000083473003, *pud=0000000083503003, *pmd=0000000000000000 Internal error: Oops: 9600004f [#1] PREEMPT SMP we'll see: Unable to handle kernel write to read-only memory at virtual address ffff000008e760d8 pgd = ffff80003d3de000 [ffff000008e760d8] *pgd=0000000083472003, *pud=0000000083435003, *pmd=0000000000000000 Internal error: Oops: 9600004f [#1] PREEMPT SMP Cc: James Morse <james.morse@arm.com> Cc: Laura Abbott <labbott@redhat.com> Cc: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org> --- Changes from v1: * Move into __do_kernel_fault() (Mark Rutland) arch/arm64/mm/fault.c | 47 +++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) -- 2.10.0.297.gf6727b0