[PATCH] arm64: entry: remove redundant IRQ flag tracing

All EL0 returns go via ret_to_user(), which masks IRQs and notifies
lockdep and tracing before calling into do_notify_resume(). Therefore,
there's no need for do_notify_resume() to call trace_hardirqs_off(), and
the comment is stale. The call is simply redundant.

In ret_to_user() we call exit_to_user_mode(), which notifies lockdep and
tracing the IRQs will be enabled in userspace, so there's no need for
el0_svc_common() to call trace_hardirqs_on() before returning. Further,
at the start of ret_to_user() we call trace_hardirqs_off(), so not only
is this redundant, but it is immediately undone.

In addition to being redundant, the trace_hardirqs_on() in
trace_hardirqs_on() isn't consistent with the HW state, and is liable to
cause issues for any C code or instrumentation invoked before this is
undone in ret_to_user(). While we appear to get away with this in
practice, it's very fragile and liable to break if we make changes in
this area.

This patch removes the redundant tracing calls and associated stale
comments.

Fixes: 23529049c6842382 ("arm64: entry: fix non-NMI user<->kernel transitions")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/signal.c  | 7 -------
 arch/arm64/kernel/syscall.c | 9 +--------
 2 files changed, 1 insertion(+), 15 deletions(-)

Catalin, Will, would you be happy to take this as a fix for v5.11? It addresses
an oversight in the entry fixes merged in v5.10, and the bit in
el0_svc_common() is very fragile in the presence of instrumentation or new C
code, so I'd like to get that fixed and backported before I make further
changes to the entry code.

Thanks,
Mark.

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index f71d6ce4673f..6237486ff6bb 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c

@@ -914,13 +914,6 @@ static void do_signal(struct pt_regs *regs)
 asmlinkage void do_notify_resume(struct pt_regs *regs,
 				 unsigned long thread_flags)
 {
-	/*
-	 * The assembly code enters us with IRQs off, but it hasn't
-	 * informed the tracing code of that for efficiency reasons.
-	 * Update the trace code with the current status.
-	 */
-	trace_hardirqs_off();
-
 	do {
 		if (thread_flags & _TIF_NEED_RESCHED) {
 			/* Unmask Debug and SError for the next task */

diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index f61e9d8cc55a..0bfac95fe464 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c

@@ -165,15 +165,8 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
 	if (!has_syscall_work(flags) && !IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
 		local_daif_mask();
 		flags = current_thread_info()->flags;
-		if (!has_syscall_work(flags) && !(flags & _TIF_SINGLESTEP)) {
-			/*
-			 * We're off to userspace, where interrupts are
-			 * always enabled after we restore the flags from
-			 * the SPSR.
-			 */
-			trace_hardirqs_on();
+		if (!has_syscall_work(flags) && !(flags & _TIF_SINGLESTEP))
 			return;
-		}
 		local_daif_restore(DAIF_PROCCTX);
 	}
 

-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jan 07, 2021 at 02:53:10PM +0000, Mark Rutland wrote:

All EL0 returns go via ret_to_user(), which masks IRQs and notifies
lockdep and tracing before calling into do_notify_resume(). Therefore,
there's no need for do_notify_resume() to call trace_hardirqs_off(), and
the comment is stale. The call is simply redundant.

In ret_to_user() we call exit_to_user_mode(), which notifies lockdep and
tracing the IRQs will be enabled in userspace, so there's no need for
el0_svc_common() to call trace_hardirqs_on() before returning. Further,
at the start of ret_to_user() we call trace_hardirqs_off(), so not only
is this redundant, but it is immediately undone.

In addition to being redundant, the trace_hardirqs_on() in
trace_hardirqs_on() isn't consistent with the HW state, and is liable to
cause issues for any C code or instrumentation invoked before this is
undone in ret_to_user().

I can't parse this final paragraph, but it seems to be the part which
justifies this as a fix. Please can you reword?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jan 12, 2021 at 02:25:27PM +0000, Will Deacon wrote:

On Thu, Jan 07, 2021 at 02:53:10PM +0000, Mark Rutland wrote:

quoted

All EL0 returns go via ret_to_user(), which masks IRQs and notifies
lockdep and tracing before calling into do_notify_resume(). Therefore,
there's no need for do_notify_resume() to call trace_hardirqs_off(), and
the comment is stale. The call is simply redundant.

In ret_to_user() we call exit_to_user_mode(), which notifies lockdep and
tracing the IRQs will be enabled in userspace, so there's no need for
el0_svc_common() to call trace_hardirqs_on() before returning. Further,
at the start of ret_to_user() we call trace_hardirqs_off(), so not only
is this redundant, but it is immediately undone.

In addition to being redundant, the trace_hardirqs_on() in
trace_hardirqs_on() isn't consistent with the HW state, and is liable to
cause issues for any C code or instrumentation invoked before this is
undone in ret_to_user().

I can't parse this final paragraph, but it seems to be the part which
justifies this as a fix. Please can you reword?

Oh whoops, I messed that up. Does the following make more sense to you?

| In addition to being redundant, the trace_hardirqs_on() in
| el0_svc_common() leaves lockdep inconsistent with the hardware state,
| and is liable to cause issues for any C code or instrumentation
| between this and the call to trace_hardirqs_off() which undoes it in
| ret_to_user().

... if so, I can resend with that folded in, unless you'd prefer to fix
it up locally.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Jan 12, 2021 at 02:39:49PM +0000, Mark Rutland wrote:

On Tue, Jan 12, 2021 at 02:25:27PM +0000, Will Deacon wrote:

quoted

On Thu, Jan 07, 2021 at 02:53:10PM +0000, Mark Rutland wrote:

quoted

All EL0 returns go via ret_to_user(), which masks IRQs and notifies
lockdep and tracing before calling into do_notify_resume(). Therefore,
there's no need for do_notify_resume() to call trace_hardirqs_off(), and
the comment is stale. The call is simply redundant.

In ret_to_user() we call exit_to_user_mode(), which notifies lockdep and
tracing the IRQs will be enabled in userspace, so there's no need for
el0_svc_common() to call trace_hardirqs_on() before returning. Further,
at the start of ret_to_user() we call trace_hardirqs_off(), so not only
is this redundant, but it is immediately undone.

In addition to being redundant, the trace_hardirqs_on() in
trace_hardirqs_on() isn't consistent with the HW state, and is liable to
cause issues for any C code or instrumentation invoked before this is
undone in ret_to_user().

I can't parse this final paragraph, but it seems to be the part which
justifies this as a fix. Please can you reword?

Oh whoops, I messed that up. Does the following make more sense to you?

| In addition to being redundant, the trace_hardirqs_on() in
| el0_svc_common() leaves lockdep inconsistent with the hardware state,
| and is liable to cause issues for any C code or instrumentation
| between this and the call to trace_hardirqs_off() which undoes it in
| ret_to_user().

... if so, I can resend with that folded in, unless you'd prefer to fix
it up locally.

With that change:

Acked-by: Will Deacon <will@kernel.org>

Catalin's looking after fixes, so I'll leave it up to him.

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, 7 Jan 2021 14:53:10 +0000, Mark Rutland wrote:

All EL0 returns go via ret_to_user(), which masks IRQs and notifies
lockdep and tracing before calling into do_notify_resume(). Therefore,
there's no need for do_notify_resume() to call trace_hardirqs_off(), and
the comment is stale. The call is simply redundant.

In ret_to_user() we call exit_to_user_mode(), which notifies lockdep and
tracing the IRQs will be enabled in userspace, so there's no need for
el0_svc_common() to call trace_hardirqs_on() before returning. Further,
at the start of ret_to_user() we call trace_hardirqs_off(), so not only
is this redundant, but it is immediately undone.

[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: entry: remove redundant IRQ flag tracing
      https://git.kernel.org/arm64/c/df06824767cc

-- 
Catalin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel