| Message ID | 1497022554-1451-1-git-send-email-kristina.martsenko@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 09, 2017 at 04:35:52PM +0100, Kristina Martsenko wrote: > /* > - * Dump out the page tables associated with 'addr' in mm 'mm'. > + * Dump out the page tables associated with 'addr' in the currently active mm. > */ > -void show_pte(struct mm_struct *mm, unsigned long addr) > +void show_pte(unsigned long addr) > { > + struct mm_struct *mm; > pgd_t *pgd; > > - if (!mm) > + if (addr < TASK_SIZE) { > + /* TTBR0 */ > + mm = current->active_mm; Can we log something for the active_mm == &init_mm case? e.g. if (addr < TASK_SIZE) { if (current->active_mm == &init_mm) pr_alert("[%016lx] address in unknown TTBR0 range\n", addr); else mm = current->active_mm; } ... > + } else if (addr >= VA_START) { > + /* TTBR1 */ > mm = &init_mm; > + } else { > + pr_alert("[%08lx] address between user and kernel address ranges\n", > + addr); I think that should be %016lx, given the address will be 64-bit. With those: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark.
On Fri, Jun 09, 2017 at 05:04:07PM +0100, Mark Rutland wrote: > On Fri, Jun 09, 2017 at 04:35:52PM +0100, Kristina Martsenko wrote: > > /* > > - * Dump out the page tables associated with 'addr' in mm 'mm'. > > + * Dump out the page tables associated with 'addr' in the currently active mm. > > */ > > -void show_pte(struct mm_struct *mm, unsigned long addr) > > +void show_pte(unsigned long addr) > > { > > + struct mm_struct *mm; > > pgd_t *pgd; > > > > - if (!mm) > > + if (addr < TASK_SIZE) { > > + /* TTBR0 */ > > + mm = current->active_mm; > > Can we log something for the active_mm == &init_mm case? > > e.g. > > if (addr < TASK_SIZE) { > if (current->active_mm == &init_mm) > pr_alert("[%016lx] address in unknown TTBR0 range\n", > addr); I don't understand the case you're trying to highlight here, nor which table you want to walk. Will
On Fri, Jun 09, 2017 at 05:33:29PM +0100, Will Deacon wrote: > On Fri, Jun 09, 2017 at 05:04:07PM +0100, Mark Rutland wrote: > > On Fri, Jun 09, 2017 at 04:35:52PM +0100, Kristina Martsenko wrote: > > > /* > > > - * Dump out the page tables associated with 'addr' in mm 'mm'. > > > + * Dump out the page tables associated with 'addr' in the currently active mm. > > > */ > > > -void show_pte(struct mm_struct *mm, unsigned long addr) > > > +void show_pte(unsigned long addr) > > > { > > > + struct mm_struct *mm; > > > pgd_t *pgd; > > > > > > - if (!mm) > > > + if (addr < TASK_SIZE) { > > > + /* TTBR0 */ > > > + mm = current->active_mm; > > > > Can we log something for the active_mm == &init_mm case? > > > > e.g. > > > > if (addr < TASK_SIZE) { > > if (current->active_mm == &init_mm) > > pr_alert("[%016lx] address in unknown TTBR0 range\n", > > addr); > > I don't understand the case you're trying to highlight here, nor which > table you want to walk. On some threads, current->active_mm is the init_mm, and this doesn't represent TTBR0. In this case, TTBR0 is typically the zero page (or the idmap). If we don't explicitly handle the init_mm case, we'd walk the kernel's TTBR1 page table when reporting such faults, which would be misleading. The easiest thing to do is to bail out, rather than trying to read TTBR0 and walk whatever it points to, since that could have an unusual VA size. Thanks, Mark.
Hi, Kristina, On Fri, Jun 09, 2017 at 04:35:52PM +0100, Kristina Martsenko wrote: > When we take a fault that can't be handled, we print out the page table > entries associated with the faulting address. In some cases we currently > print out the wrong entries. For a faulting TTBR1 address, we sometimes > print out TTBR0 table entries instead, and for a faulting TTBR0 address > we sometimes print out TTBR1 table entries. Fix this by choosing the > tables based on the faulting address. > > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > --- > > v2: > - use TASK_SIZE and VA_START, change printed message > > arch/arm64/include/asm/system_misc.h | 2 +- > arch/arm64/mm/fault.c | 29 +++++++++++++++++++---------- > 2 files changed, 20 insertions(+), 11 deletions(-) [...] > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index c3f2b1048f83..a9dfb37c87a2 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) > #endif > > /* > - * Dump out the page tables associated with 'addr' in mm 'mm'. > + * Dump out the page tables associated with 'addr' in the currently active mm. > */ > -void show_pte(struct mm_struct *mm, unsigned long addr) > +void show_pte(unsigned long addr) > { > + struct mm_struct *mm; > pgd_t *pgd; > > - if (!mm) > + if (addr < TASK_SIZE) { > + /* TTBR0 */ > + mm = current->active_mm; > + } else if (addr >= VA_START) { > + /* TTBR1 */ > mm = &init_mm; > + } else { > + pr_alert("[%08lx] address between user and kernel address ranges\n", > + addr); > + return; > + } > > pr_alert("pgd = %p\n", mm->pgd); > pgd = pgd_offset(mm, addr); Is there any reason to change the prototype of the function? You say nothing about it in patch description. The show_pte() is implemented in several arches, and everywhere it takes mm_struct as 1st argument. For me, if you don't need to change the prototype, you'd better leave things as is. The patch will get much shorter and expressive with it. If you really need to change the prototype, it would be better to do it in separated patch and give clear explanations - what for? Yury
On Fri, Jun 09, 2017 at 11:22:36PM +0300, Yury Norov wrote: > On Fri, Jun 09, 2017 at 04:35:52PM +0100, Kristina Martsenko wrote: > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > index c3f2b1048f83..a9dfb37c87a2 100644 > > --- a/arch/arm64/mm/fault.c > > +++ b/arch/arm64/mm/fault.c > > @@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) > > #endif > > > > /* > > - * Dump out the page tables associated with 'addr' in mm 'mm'. > > + * Dump out the page tables associated with 'addr' in the currently active mm. > > */ > > -void show_pte(struct mm_struct *mm, unsigned long addr) > > +void show_pte(unsigned long addr) > > { > > + struct mm_struct *mm; > > pgd_t *pgd; > > > > - if (!mm) > > + if (addr < TASK_SIZE) { > > + /* TTBR0 */ > > + mm = current->active_mm; > > + } else if (addr >= VA_START) { > > + /* TTBR1 */ > > mm = &init_mm; > > + } else { > > + pr_alert("[%08lx] address between user and kernel address ranges\n", > > + addr); > > + return; > > + } > > > > pr_alert("pgd = %p\n", mm->pgd); > > pgd = pgd_offset(mm, addr); > > Is there any reason to change the prototype of the function? > You say nothing about it in patch description. The show_pte() > is implemented in several arches, and everywhere it takes mm_struct > as 1st argument. For me, if you don't need to change the prototype, > you'd better leave things as is. The patch will get much shorter > and expressive with it. > > If you really need to change the prototype, it would be better to do > it in separated patch and give clear explanations - what for? The mm is unused and this isn't a core interface. In fact, it's only available on architectures that started off by copying from arch/arm/. We can always add the mm back if we need it in future. Will
On Thu, Jun 15, 2017 at 11:00:51AM +0100, Will Deacon wrote: > On Fri, Jun 09, 2017 at 11:22:36PM +0300, Yury Norov wrote: > > On Fri, Jun 09, 2017 at 04:35:52PM +0100, Kristina Martsenko wrote: > > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > > index c3f2b1048f83..a9dfb37c87a2 100644 > > > --- a/arch/arm64/mm/fault.c > > > +++ b/arch/arm64/mm/fault.c > > > @@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) > > > #endif > > > > > > /* > > > - * Dump out the page tables associated with 'addr' in mm 'mm'. > > > + * Dump out the page tables associated with 'addr' in the currently active mm. > > > */ > > > -void show_pte(struct mm_struct *mm, unsigned long addr) > > > +void show_pte(unsigned long addr) > > > { > > > + struct mm_struct *mm; > > > pgd_t *pgd; > > > > > > - if (!mm) > > > + if (addr < TASK_SIZE) { > > > + /* TTBR0 */ > > > + mm = current->active_mm; > > > + } else if (addr >= VA_START) { > > > + /* TTBR1 */ > > > mm = &init_mm; > > > + } else { > > > + pr_alert("[%08lx] address between user and kernel address ranges\n", > > > + addr); > > > + return; > > > + } > > > > > > pr_alert("pgd = %p\n", mm->pgd); > > > pgd = pgd_offset(mm, addr); > > > > Is there any reason to change the prototype of the function? > > You say nothing about it in patch description. The show_pte() > > is implemented in several arches, and everywhere it takes mm_struct > > as 1st argument. For me, if you don't need to change the prototype, > > you'd better leave things as is. The patch will get much shorter > > and expressive with it. > > > > If you really need to change the prototype, it would be better to do > > it in separated patch and give clear explanations - what for? > > The mm is unused and this isn't a core interface. In fact, it's only > available on architectures that started off by copying from arch/arm/. > > We can always add the mm back if we need it in future. OK, understood that. But I still think it should be done in separeted patch, and it would be also good to revise sh and unicore32. It seems that sh may be easily switched to the proposed interface, and the unicore32 most probably too. Yury
On Thu, Jun 15, 2017 at 01:12:30PM +0300, Yury Norov wrote: > On Thu, Jun 15, 2017 at 11:00:51AM +0100, Will Deacon wrote: > > On Fri, Jun 09, 2017 at 11:22:36PM +0300, Yury Norov wrote: > > > On Fri, Jun 09, 2017 at 04:35:52PM +0100, Kristina Martsenko wrote: > > > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > > > index c3f2b1048f83..a9dfb37c87a2 100644 > > > > --- a/arch/arm64/mm/fault.c > > > > +++ b/arch/arm64/mm/fault.c > > > > @@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) > > > > #endif > > > > > > > > /* > > > > - * Dump out the page tables associated with 'addr' in mm 'mm'. > > > > + * Dump out the page tables associated with 'addr' in the currently active mm. > > > > */ > > > > -void show_pte(struct mm_struct *mm, unsigned long addr) > > > > +void show_pte(unsigned long addr) > > > > { > > > > + struct mm_struct *mm; > > > > pgd_t *pgd; > > > > > > > > - if (!mm) > > > > + if (addr < TASK_SIZE) { > > > > + /* TTBR0 */ > > > > + mm = current->active_mm; > > > > + } else if (addr >= VA_START) { > > > > + /* TTBR1 */ > > > > mm = &init_mm; > > > > + } else { > > > > + pr_alert("[%08lx] address between user and kernel address ranges\n", > > > > + addr); > > > > + return; > > > > + } > > > > > > > > pr_alert("pgd = %p\n", mm->pgd); > > > > pgd = pgd_offset(mm, addr); > > > > > > Is there any reason to change the prototype of the function? > > > You say nothing about it in patch description. The show_pte() > > > is implemented in several arches, and everywhere it takes mm_struct > > > as 1st argument. For me, if you don't need to change the prototype, > > > you'd better leave things as is. The patch will get much shorter > > > and expressive with it. > > > > > > If you really need to change the prototype, it would be better to do > > > it in separated patch and give clear explanations - what for? > > > > The mm is unused and this isn't a core interface. In fact, it's only > > available on architectures that started off by copying from arch/arm/. > > > > We can always add the mm back if we need it in future. > > OK, understood that. But I still think it should be done in separeted > patch, and it would be also good to revise sh and unicore32. It seems > that sh may be easily switched to the proposed interface, and the > unicore32 most probably too. Why? This is the patch that removes the usage of the paramater, so it should also remove the parameter. This function isn't called by anybody outside of arm64, so there's no "proposed interface" to speak of. If this was a core function with lots of tree-wide callers, then I'd agree with you: remove use of the thing, fix the callers, then remove the parameter. But that's not the case here. Will
diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h index bc812435bc76..d0beefeb6d25 100644 --- a/arch/arm64/include/asm/system_misc.h +++ b/arch/arm64/include/asm/system_misc.h @@ -40,7 +40,7 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int, int sig, int code, const char *name); struct mm_struct; -extern void show_pte(struct mm_struct *mm, unsigned long addr); +extern void show_pte(unsigned long addr); extern void __show_regs(struct pt_regs *); extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd); diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index c3f2b1048f83..a9dfb37c87a2 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -80,14 +80,24 @@ static inline int notify_page_fault(struct pt_regs *regs, unsigned int esr) #endif /* - * Dump out the page tables associated with 'addr' in mm 'mm'. + * Dump out the page tables associated with 'addr' in the currently active mm. */ -void show_pte(struct mm_struct *mm, unsigned long addr) +void show_pte(unsigned long addr) { + struct mm_struct *mm; pgd_t *pgd; - if (!mm) + if (addr < TASK_SIZE) { + /* TTBR0 */ + mm = current->active_mm; + } else if (addr >= VA_START) { + /* TTBR1 */ mm = &init_mm; + } else { + pr_alert("[%08lx] address between user and kernel address ranges\n", + addr); + return; + } pr_alert("pgd = %p\n", mm->pgd); pgd = pgd_offset(mm, addr); @@ -196,8 +206,8 @@ static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs, /* * 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) +static void __do_kernel_fault(unsigned long addr, unsigned int esr, + struct pt_regs *regs) { const char *msg; @@ -227,7 +237,7 @@ static void __do_kernel_fault(struct mm_struct *mm, unsigned long addr, pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg, addr); - show_pte(mm, addr); + show_pte(addr); die("Oops", regs, esr); bust_spinlocks(0); do_exit(SIGKILL); @@ -249,7 +259,7 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr, pr_info("%s[%d]: unhandled %s (%d) at 0x%08lx, esr 0x%03x\n", tsk->comm, task_pid_nr(tsk), inf->name, sig, addr, esr); - show_pte(tsk->mm, addr); + show_pte(addr); __show_regs(regs); } @@ -265,7 +275,6 @@ static void __do_user_fault(struct task_struct *tsk, unsigned long addr, static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *regs) { struct task_struct *tsk = current; - struct mm_struct *mm = tsk->active_mm; const struct fault_info *inf; /* @@ -276,7 +285,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re inf = esr_to_fault_info(esr); __do_user_fault(tsk, addr, esr, inf->sig, inf->code, regs); } else - __do_kernel_fault(mm, addr, esr, regs); + __do_kernel_fault(addr, esr, regs); } #define VM_FAULT_BADMAP 0x010000 @@ -475,7 +484,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, return 0; no_context: - __do_kernel_fault(mm, addr, esr, regs); + __do_kernel_fault(addr, esr, regs); return 0; }
When we take a fault that can't be handled, we print out the page table entries associated with the faulting address. In some cases we currently print out the wrong entries. For a faulting TTBR1 address, we sometimes print out TTBR0 table entries instead, and for a faulting TTBR0 address we sometimes print out TTBR1 table entries. Fix this by choosing the tables based on the faulting address. Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> --- v2: - use TASK_SIZE and VA_START, change printed message arch/arm64/include/asm/system_misc.h | 2 +- arch/arm64/mm/fault.c | 29 +++++++++++++++++++---------- 2 files changed, 20 insertions(+), 11 deletions(-)