Re: Perf: WARNING: arch/x86/entry/common.c:624 idtentry_exit_cond_rcu+0x92/0xc0

[Peter Zijlstra]

On Thu, Jul 02, 2020 at 07:57:54PM +0530, Naresh Kamboju wrote:
> I have reported this warning on linux-next and now it is happening on
> linux mainline tree.
> May I know , are we missing a fix patch on linus 's tree ?
> 
> - Naresh
> ---
> While running selftest x86 single_step_syscall_32 on
> i386 kernel linux 5.8.0-rc3 kernel warning noticed.
> 
> steps to reproduce:
> --------------------------
> perf test
> and
> cd /opt/kselftests/default-in-kernel/x86
> ./single_step_syscall_32
> 
> crash dump,
> [ 1324.774385] kselftest: Running tests in x86
> [ 1324.830187] ------------[ cut here ]------------
> [ 1324.834820] IRQs not disabled as expected
> [ 1324.838838] WARNING: CPU: 2 PID: 5365 at
> /usr/src/kernel/arch/x86/entry/common.c:645
> idtentry_exit_cond_rcu+0x92/0xc0

How's this?

DEFINE_IDTENTRY_DEBUG() is DEFINE_IDTENTRY_RAW on x86_64
                           DEFINE_IDTENTRY on i386

calling exc_debug_*() from DEFINE_IDTENTRY() does a double layer of
idtentry_{enter,exit}*() functions.

Also, handle_debug() is still a trainwreck, we should never get to
cond_local_irq_enable() when !user.

Completely untested...

---
 arch/x86/kernel/traps.c | 49 +++++++++++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index b0195771c932..47d6b46e1564 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -806,8 +806,17 @@ static void handle_debug(struct pt_regs *regs, unsigned long dr6, bool user)
 	 * If DR6 is zero, no point in trying to handle it. The kernel is
 	 * not using INT1.
 	 */
-	if (!user && !dr6)
-		return;
+	if (!user) {
+		/*
+		 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
+		 * watchpoint at the same time then that will still be handled.
+		 */
+		if ((dr6 & DR_STEP) && is_sysenter_singlestep(regs))
+			dr6 &= ~DR_STEP;
+
+		if (!dr6)
+			return;
+	}
 
 	/*
 	 * If dr6 has no reason to give us about the origin of this trap,
@@ -859,25 +868,29 @@ static void handle_debug(struct pt_regs *regs, unsigned long dr6, bool user)
 	cond_local_irq_disable(regs);
 }
 
+#ifdef CONFIG_X86_64
 static __always_inline void exc_debug_kernel(struct pt_regs *regs,
 					     unsigned long dr6)
 {
 	bool irq_state = idtentry_enter_nmi(regs);
 	instrumentation_begin();
 
-	/*
-	 * Catch SYSENTER with TF set and clear DR_STEP. If this hit a
-	 * watchpoint at the same time then that will still be handled.
-	 */
-	if ((dr6 & DR_STEP) && is_sysenter_singlestep(regs))
-		dr6 &= ~DR_STEP;
-
 	handle_debug(regs, dr6, false);
 
 	instrumentation_end();
 	idtentry_exit_nmi(regs, irq_state);
 }
 
+/* IST stack entry */
+DEFINE_IDTENTRY_DEBUG(exc_debug)
+{
+	unsigned long dr6, dr7;
+
+	debug_enter(&dr6, &dr7);
+	exc_debug_kernel(regs, dr6);
+	debug_exit(dr7);
+}
+
 static __always_inline void exc_debug_user(struct pt_regs *regs,
 					   unsigned long dr6)
 {
@@ -890,17 +903,6 @@ static __always_inline void exc_debug_user(struct pt_regs *regs,
 	idtentry_exit_user(regs);
 }
 
-#ifdef CONFIG_X86_64
-/* IST stack entry */
-DEFINE_IDTENTRY_DEBUG(exc_debug)
-{
-	unsigned long dr6, dr7;
-
-	debug_enter(&dr6, &dr7);
-	exc_debug_kernel(regs, dr6);
-	debug_exit(dr7);
-}
-
 /* User entry, runs on regular task stack */
 DEFINE_IDTENTRY_DEBUG_USER(exc_debug)
 {
@@ -917,12 +919,7 @@ DEFINE_IDTENTRY_DEBUG(exc_debug)
 	unsigned long dr6, dr7;
 
 	debug_enter(&dr6, &dr7);
-
-	if (user_mode(regs))
-		exc_debug_user(regs, dr6);
-	else
-		exc_debug_kernel(regs, dr6);
-
+	handle_debug(regs, dr6, user_mode(regs));
 	debug_exit(dr7);
 }
 #endif