Messages in this thread Patches in this message | | | Date | Mon, 26 Sep 2022 21:26:50 +0800 | Subject | Re: [PATCH v2] ARM: mm: Provide better fault message for permission fault | From | Kefeng Wang <> |
| |
On 2022/9/26 18:13, Russell King (Oracle) wrote: > On Mon, Sep 19, 2022 at 06:38:45PM +0800, Kefeng Wang wrote: >> If there is a permission fault in __do_kernel_fault(), we only >> print the generic "paging request" message which don't show >> read, write or excute information, let's provide better fault >> message for them. > I don't like this change. With CPUs that do not have the ability to > relocate the vectors to 0xffff0000, the vectors live at address 0, > so NULL pointer dereferences can produce permission faults. The __do_user_fault(), do_DataAbort() and do_PrefetchAbort() shows the FSR when printing, we could do it in die_kernel_fault(), and which will be easy for us to check whether the page fault is permision fault,
--- a/arch/arm/mm/fault.c +++ b/arch/arm/mm/fault.c @@ -111,8 +111,8 @@ static void die_kernel_fault(const char *msg, struct mm_struct *mm, { bust_spinlocks(1); pr_alert("8<--- cut here ---\n"); - pr_alert("Unable to handle kernel %s at virtual address %08lx\n", - msg, addr); + pr_alert("Unable to handle kernel %s (0x%08x) at virtual address %08lx\n", + msg, fsr, addr);
show_pte(KERN_ALERT, mm, addr); die("Oops", regs, fsr);
or,
> I would much rather we did something similar to what x86 does: > > pr_alert("#PF: %s %s in %s mode\n", > (error_code & X86_PF_USER) ? "user" : "supervisor", > (error_code & X86_PF_INSTR) ? "instruction fetch" : > (error_code & X86_PF_WRITE) ? "write access" : > "read access", > user_mode(regs) ? "user" : "kernel"); > > As we already print whether we're in user or kernel mode in the > register dump, there's no need to repeat that. I think we just > need an extra line to decode the FSR PF and write bits.
We could decode the FSR register,
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c index 46cccd6bf705..406e0210c3c5 100644 --- a/arch/arm/mm/fault.c +++ b/arch/arm/mm/fault.c @@ -113,6 +113,10 @@ static void die_kernel_fault(const char *msg, struct mm_struct *mm, pr_alert("8<--- cut here ---\n"); pr_alert("Unable to handle kernel %s at virtual address %08lx\n", msg, addr); + pr_alert("FSR: 0x%08x, LNX_PF = %u, CM = %u, WnR = %u\n", fsr, + (fsr & FSR_LNX_PF) >> FSR_LNX_PF_SHIFT, + (fsr & FSR_CM) >> FSR_CM_SHIFT, + (fsr & FSR_WRITE) >> FSR_WRITE_SHIFT);
show_pte(KERN_ALERT, mm, addr); die("Oops", regs, fsr); diff --git a/arch/arm/mm/fault.h b/arch/arm/mm/fault.h index 83b5ab32d7a4..18f882aa2b32 100644 --- a/arch/arm/mm/fault.h +++ b/arch/arm/mm/fault.h @@ -5,9 +5,12 @@ /* * Fault status register encodings. We steal bit 31 for our own purposes. */ -#define FSR_LNX_PF (1 << 31) -#define FSR_CM (1 << 13) -#define FSR_WRITE (1 << 11) +#define FSR_LNX_PF_SHIFT (31) +#define FSR_LNX_PF (1 << FSR_LNX_PF_SHIFT) +#define FSR_CM_SHIFT (13) +#define FSR_CM (1 << FSR_CM_SHIFT) +#define FSR_WRITE_SHIFT (11) +#define FSR_WRITE (1 << FSR_WRITE_SHIFT) #define FSR_FS4 (1 << 10) #define FSR_FS3_0 (15) #define FSR_FS5_0 (0x3f)
What's your option ?
>
| |