Re: [PATCH] arm64: Expose original FAR_EL1 value in sigcontext
From: Catalin Marinas <catalin.marinas@arm.com>
Date: 2020-03-25 13:10:34
Hi Peter, On Thu, Mar 12, 2020 at 10:17:55AM -0700, Peter Collingbourne wrote:
quoted hunk ↗ jump to hunk
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c index fde59981445ca..290ea59c68b85 100644 --- a/arch/arm64/kernel/entry-common.c +++ b/arch/arm64/kernel/entry-common.c@@ -22,7 +22,6 @@ static void notrace el1_abort(struct pt_regs *regs, unsigned long esr) unsigned long far = read_sysreg(far_el1); local_daif_inherit(regs); - far = untagged_addr(far); do_mem_abort(far, esr, regs); } NOKPROBE_SYMBOL(el1_abort);
Would we get a signal on faults triggered by the kernel? Anyway, I'm fine with this change for consistency and may help with the fault information printed by the kernel with khwasan or (later) MTE.
quoted hunk ↗ jump to hunk
@@ -104,7 +103,6 @@ static void notrace el0_da(struct pt_regs *regs, unsigned long esr) user_exit_irqoff(); local_daif_restore(DAIF_PROCCTX); - far = untagged_addr(far); do_mem_abort(far, esr, regs); } NOKPROBE_SYMBOL(el0_da);diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index 0b727edf41046..985cd44decf62 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c@@ -730,7 +730,7 @@ static u64 get_distance_from_watchpoint(unsigned long addr, u64 val, return 0; } -static int watchpoint_handler(unsigned long addr, unsigned int esr, +static int watchpoint_handler(unsigned long far, unsigned int esr, struct pt_regs *regs) { int i, step = 0, *kernel_step, access, closest_match = 0;@@ -741,6 +741,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr, struct debug_info *debug_info; struct arch_hw_breakpoint *info; struct arch_hw_breakpoint_ctrl ctrl; + unsigned long addr = untagged_addr(far); slots = this_cpu_ptr(wp_on_reg); debug_info = ¤t->thread.debug;
Why do we need to untag this here? Have you hit any bug? This function gets the original FAR_EL1 value, untagged (via elX_dbg()), and we clear the tag further down in get_distance_from_watchpoint().
quoted hunk ↗ jump to hunk
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 339882db5a915..48e8b6c7b5369 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c@@ -55,6 +55,7 @@ struct rt_sigframe_user_layout { unsigned long fpsimd_offset; unsigned long esr_offset; + unsigned long far_offset; unsigned long sve_offset; unsigned long extra_offset; unsigned long end_offset;@@ -383,6 +384,7 @@ static int parse_user_sigframe(struct user_ctxs *user, break; case ESR_MAGIC: + case FAR_MAGIC: /* ignore */ break;@@ -581,6 +583,11 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user, sizeof(struct esr_context)); if (err) return err; + + err = sigframe_alloc(user, &user->far_offset, + sizeof(struct far_context)); + if (err) + return err;
It looks fine, I think it makes sense to only expose the raw FAR_EL1 when we also expose the ESR_EL1 (via set_thread_esr()).
quoted hunk ↗ jump to hunk
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 85566d32958f5..2ca2de1ff43be 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c@@ -41,7 +41,7 @@ #include <asm/traps.h> struct fault_info { - int (*fn)(unsigned long addr, unsigned int esr, + int (*fn)(unsigned long far, unsigned int esr, struct pt_regs *regs); int sig; int code;@@ -320,9 +320,11 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr, die_kernel_fault(msg, addr, esr, regs); } -static void set_thread_esr(unsigned long address, unsigned int esr) +static void set_thread_esr(unsigned long far, unsigned int esr)
We might as well rename this to set_thread_far_esr(). Thanks. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel