Re: [PATCH] riscv: entry: Fixup do_trap_break from kernel side

[Peter Zijlstra]

On Mon, Jun 22, 2026 at 06:25:13PM +0800, Vivian Wang wrote:

> > I still don't understand it. This cannot fix anything. Consider:
> >
> >  EBREAK
> >  raw_spin_lock_irq(&your_lock)
> >  EBREAK
> >
> > So now the first 'works', but the second will crash. Additionally,
> > having the EBREAK context differ so dramatically between invocations
> > seems like a very bad deal to me.
> 
> To spell it out, the problem that needs fixing is:
> 
> -> BUG()
>    -> ebreak instruction
>       -> Breakpoint exception
>          -> do_trap_break()
>             -> irqentry_nmi_enter()
>             [ now in_nmi() / in_interrupt() ]
>             -> report_bug() returns BUG_TRAP_TYPE_BUG
>             -> die()
>                -> make_task_dead()
>                   -> panic() because we're in_interrupt()
> 
> As such, currently on riscv all BUG() simply completely panic() the
> entire machine, rather than just killing the one task.

Hmm, from reading some of the previous emails this morning, I got the
impression the problem was with kgdb, not BUG().

Anyway, my argument doesn't change, with the proposed patch:

  BUG()

and:

  local_irq_disable();
  BUG();

will behave quite differently, for no sane reason.

Anyway, BUG()/trap is indeed a bit of magic, the x86 code lives in
arch/x86/kernel/traps.c:exc_invalid_op(). And it looks like we do not
indeed use NMI-like for this path, although I cannot remember why.

*however* I see your kgdb thing also uses ebreak, whereas on x86
WARN/BUG and kGDB use different exceptions (#UD for WARN/BUG and #BP for
gdb). And our #BP handler (exc_int3) very much does NMI for from-kernel.

Same for kprobes, we use #BP/int3 for that, you also have that in
EBREAK.

Anyway, you're handling 3 different cases in one exception, which is a
bit of a mess, but something like so perhaps?

---
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 8c62c771a656..41c7faac7eb3 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -264,42 +264,58 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
 	return GET_INSN_LENGTH(insn);
 }
 
-static bool probe_single_step_handler(struct pt_regs *regs)
+static void handle_kernel_die(struct pt_regs *regs)
 {
-	bool user = user_mode(regs);
-
-	return user ? uprobe_single_step_handler(regs) : kprobe_single_step_handler(regs);
+	irqentry_state_t state = irqentry_enter(regs);
+	die(regs, "Kernel BUG");
+	irqentry_exit(regs, state);
 }
 
-static bool probe_breakpoint_handler(struct pt_regs *regs)
+static bool handle_kernel_bug(struct pt_regs *regs)
 {
-	bool user = user_mode(regs);
+	if (report_bug(regs->epc, regs) == BUG_TRAP_TYPE_WARN ||
+	    handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN) {
+		regs->epc += get_break_insn_length(regs->epc);
+		return true;
+	}
 
-	return user ? uprobe_breakpoint_handler(regs) : kprobe_breakpoint_handler(regs);
+	return false;
 }
 
-void handle_break(struct pt_regs *regs)
+static bool  __handle_kernel_break(struct pt_regs *regs)
 {
-	if (probe_single_step_handler(regs))
-		return;
 
-	if (probe_breakpoint_handler(regs))
+	if (kprobe_single_step_handler(regs) ||
+	    kprobe_breakpoint_handler(regs))
+		return true;
+
+	current->thread.bad_cause = regs->cause;
+
+#ifdef CONFIG_KGDB
+	if (notify_die(DIE_TRAP, "EBREAK", regs, 0, regs->cause, SIGTRAP)
+								== NOTIFY_STOP)
+		return true;
+#endif
+	return false;
+}
+
+static bool handle_kernel_break(struct pt_regs *regs)
+{
+	irqentry_state_t state = irqentry_nmi_enter(regs);
+	bool ret = __handle_kernel_break(regs);
+	irqentry_nmi_exit(regs, state);
+	return ret;
+}
+
+static void handle_user_break(struct pt_regs *regs)
+{
+	if (uprobe_single_step_handler(regs) ||
+	    uprobe_breakpoint_handler(regs))
 		return;
 
 	current->thread.bad_cause = regs->cause;
 
-	if (user_mode(regs))
-		force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->epc);
-#ifdef CONFIG_KGDB
-	else if (notify_die(DIE_TRAP, "EBREAK", regs, 0, regs->cause, SIGTRAP)
-								== NOTIFY_STOP)
-		return;
-#endif
-	else if (report_bug(regs->epc, regs) == BUG_TRAP_TYPE_WARN ||
-		 handle_cfi_failure(regs) == BUG_TRAP_TYPE_WARN)
-		regs->epc += get_break_insn_length(regs->epc);
-	else
-		die(regs, "Kernel BUG");
+	force_sig_fault(SIGTRAP, TRAP_BRKPT, (void __user *)regs->epc);
 }
 
 asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
@@ -308,16 +324,18 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
 		irqentry_enter_from_user_mode(regs);
 		local_irq_enable();
 
-		handle_break(regs);
+		handle_user_break(regs);
 
 		local_irq_disable();
 		irqentry_exit_to_user_mode(regs);
 	} else {
-		irqentry_state_t state = irqentry_nmi_enter(regs);
+		if (handle_kernel_bug(regs))
+			return;
 
-		handle_break(regs);
+		if (handle_kernel_break(regs))
+			return;
 
-		irqentry_nmi_exit(regs, state);
+		handle_kernel_die(regs);
 	}
 }