From d6d810141e841632ce5372d757c5a6dd93801319 Mon Sep 17 00:00:00 2001
From: riastradh <riastradh@NetBSD.org>
Date: Wed, 28 Feb 2024 04:12:59 +0000
Subject: [PATCH] heartbeat(9): No kpreempt_disable/enable in
 heartbeat_suspend/resume.

This causes a leak of l_nopreempt in xc_thread when a CPU is offlined
and onlined again, because the offlining heartbeat_suspend and the
onlining heartbeat_resume happen in separate xcalls.

No change to callers because they are already bound to the CPU:

1. cnpollc does kpreempt_disable/enable itself around the calls to
   heartbeat_suspend/resume anyway

2. cpu_xc_offline/online run in the xcall thread, which is always
   bound to the CPU that is being offlined or onlined
---
 sys/kern/kern_heartbeat.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/sys/kern/kern_heartbeat.c b/sys/kern/kern_heartbeat.c
index 8d4ea01880d1..20d769637158 100644
--- a/sys/kern/kern_heartbeat.c
+++ b/sys/kern/kern_heartbeat.c
@@ -1,4 +1,4 @@
-/*	$NetBSD: kern_heartbeat.c,v 1.10 2023/09/06 12:29:14 riastradh Exp $	*/
+/*	$NetBSD: kern_heartbeat.c,v 1.11 2024/02/28 04:12:59 riastradh Exp $	*/
 
 /*-
  * Copyright (c) 2023 The NetBSD Foundation, Inc.
@@ -82,7 +82,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_heartbeat.c,v 1.10 2023/09/06 12:29:14 riastradh Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_heartbeat.c,v 1.11 2024/02/28 04:12:59 riastradh Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_ddb.h"
@@ -132,24 +132,20 @@ void *heartbeat_sih			__read_mostly;
  *
  *	Called after the current CPU has been marked offline but before
  *	it has stopped running, or after IPL has been raised for
- *	polling-mode console input.  Binds to the current CPU as a side
- *	effect.  Nestable (but only up to 2^32 times, so don't do this
- *	in a loop).  Reversed by heartbeat_resume.
+ *	polling-mode console input.  Nestable.  Reversed by
+ *	heartbeat_resume.
+ *
+ *	Caller must be bound to the CPU, i.e., curcpu_stable() must be
+ *	true.  This function does not assert curcpu_stable() since it
+ *	is used in the ddb entry path, where any assertions risk
+ *	infinite regress into undebuggable chaos, so callers must be
+ *	careful.
  */
 void
 heartbeat_suspend(void)
 {
 	unsigned *p;
 
-	/*
-	 * We could use curlwp_bind, but we'd have to record whether we
-	 * were already bound or not to pass to curlwp_bindx in
-	 * heartbeat_resume.  Using kpreempt_disable is simpler and
-	 * unlikely to have any adverse consequences, since this only
-	 * happens when we're about to go into a tight polling loop at
-	 * raised IPL anyway.
-	 */
-	kpreempt_disable();
 	p = &curcpu()->ci_heartbeat_suspend;
 	atomic_store_relaxed(p, *p + 1);
 }
@@ -186,6 +182,9 @@ heartbeat_resume_cpu(struct cpu_info *ci)
  *	Called after the current CPU has started running but before it
  *	has been marked online, or when ending polling-mode input
  *	before IPL is restored.  Reverses heartbeat_suspend.
+ *
+ *	Caller must be bound to the CPU, i.e., curcpu_stable() must be
+ *	true.
  */
 void
 heartbeat_resume(void)
@@ -194,6 +193,8 @@ heartbeat_resume(void)
 	unsigned *p;
 	int s;
 
+	KASSERT(curcpu_stable());
+
 	/*
 	 * Reset the state so nobody spuriously thinks we had a heart
 	 * attack as soon as the heartbeat checks resume.
@@ -204,7 +205,6 @@ heartbeat_resume(void)
 
 	p = &ci->ci_heartbeat_suspend;
 	atomic_store_relaxed(p, *p - 1);
-	kpreempt_enable();
 }
 
 /*