Re: [PATCH v2] ARM: mm: Provide better fault message for permission fault

Kefeng Wang
2022-09-26T14:56:45Z

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 ?

>