[PATCH 1/2] ipmi:watchdog: Reboot cleanly on BMC reset

[Tony Camuso <tcamuso@redhat.com>]

When the BMC resets while the IPMI watchdog is active, three problems
can occur:

1. The static smi_msg and recv_msg structures remain queued in the IPMI
   layer after a response timeout. If the watchdog daemon retries, the
   code reuses these structures while still on the IPMI layer's internal
   lists, causing:

     list_add double add: new=ffffffffc10063e0, prev=ffffffffc10063e0, ...
     kernel BUG at lib/list_debug.c:29!

2. Both __ipmi_heartbeat() and _ipmi_set_timeout() use
   wait_for_completion() with no timeout, blocking indefinitely if the
   BMC is unresponsive, leaving tasks stuck in D state.

3. When the BMC loses the watchdog timer state, the driver's internal
   state becomes inconsistent, causing subsequent writes to /dev/watchdog
   to return -EINVAL, leaving the system without watchdog protection.

Fix all three issues:

- Add msg_in_flight atomic flag to prevent re-entry into
  __ipmi_heartbeat() and _ipmi_set_timeout() while message structures
  are still queued in the IPMI layer.

- Convert wait_for_completion() to wait_for_completion_timeout() in
  both functions to prevent indefinite blocking.

- Add reinit_completion() before each use to prevent stale completion
  events from allowing premature wakeup.

- Detect BMC communication failure in ipmi_wdog_msg_handler() via
  non-zero completion codes and initiate orderly_reboot() when the
  watchdog is active.  This ensures the system reboots cleanly rather
  than being left without watchdog protection.  Error classification
  distinguishes TIMER_NOT_INIT (0x80), vendor-specific codes
  (0x81-0xBE), and standard IPMI completion codes.

- Guard all BMC communication paths (_ipmi_set_timeout,
  __ipmi_heartbeat, wdog_reboot_handler) with bmc_reset_shutdown flag
  to prevent further IPMI operations during shutdown.

Signed-off-by: Tony Camuso <tcamuso@redhat.com>
---
 drivers/char/ipmi/ipmi_watchdog.c | 101 ++++++++++++++++++++++++------
 1 file changed, 83 insertions(+), 18 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
index a013ddbf1466..1d8277cbe598 100644
--- a/drivers/char/ipmi/ipmi_watchdog.c
+++ b/drivers/char/ipmi/ipmi_watchdog.c
@@ -123,6 +123,16 @@
 
 #define IPMI_WDOG_TIMER_NOT_INIT_RESP	0x80
 
+/* Timeout for waiting for a heartbeat response (in jiffies). */
+#define IPMI_HEARTBEAT_WAIT_TIMEOUT	(HZ * 5)
+
+/*
+ * Set when the BMC becomes unreachable while the watchdog is active.
+ * Once set, all BMC communication is skipped and an orderly reboot
+ * is in progress.
+ */
+static bool bmc_reset_shutdown;
+
 static DEFINE_MUTEX(ipmi_watchdog_mutex);
 static bool nowayout = WATCHDOG_NOWAYOUT;
 
@@ -339,12 +349,14 @@ static int __ipmi_heartbeat(void);
  * and freed when both the send and receive messages are free.
  */
 static atomic_t msg_tofree = ATOMIC_INIT(0);
+static atomic_t msg_in_flight = ATOMIC_INIT(0);
 static DECLARE_COMPLETION(msg_wait);
 static void msg_free_smi(struct ipmi_smi_msg *msg)
 {
 	if (atomic_dec_and_test(&msg_tofree)) {
 		if (!oops_in_progress)
 			complete(&msg_wait);
+		atomic_set(&msg_in_flight, 0);
 	}
 }
 static void msg_free_recv(struct ipmi_recv_msg *msg)
@@ -352,6 +364,7 @@ static void msg_free_recv(struct ipmi_recv_msg *msg)
 	if (atomic_dec_and_test(&msg_tofree)) {
 		if (!oops_in_progress)
 			complete(&msg_wait);
+		atomic_set(&msg_in_flight, 0);
 	}
 }
 static struct ipmi_smi_msg smi_msg = INIT_IPMI_SMI_MSG(msg_free_smi);
@@ -429,19 +442,34 @@ static int _ipmi_set_timeout(int do_heartbeat)
 {
 	int send_heartbeat_now;
 	int rv;
+	unsigned long ret;
 
 	if (!watchdog_user)
 		return -ENODEV;
 
+	if (bmc_reset_shutdown)
+		return -ENODEV;
+
+	if (atomic_read(&msg_in_flight))
+		return -EBUSY;
+
+	reinit_completion(&msg_wait);
+	atomic_set(&msg_in_flight, 1);
 	atomic_set(&msg_tofree, 2);
 
 	rv = __ipmi_set_timeout(&smi_msg, &recv_msg, &send_heartbeat_now);
 	if (rv) {
 		atomic_set(&msg_tofree, 0);
+		atomic_set(&msg_in_flight, 0);
 		return rv;
 	}
 
-	wait_for_completion(&msg_wait);
+	ret = wait_for_completion_timeout(&msg_wait,
+					  IPMI_HEARTBEAT_WAIT_TIMEOUT);
+	if (ret == 0) {
+		atomic_set(&msg_tofree, 0);
+		return -ETIMEDOUT;
+	}
 
 	if ((do_heartbeat == IPMI_SET_TIMEOUT_FORCE_HB)
 		|| ((send_heartbeat_now)
@@ -510,10 +538,17 @@ static int __ipmi_heartbeat(void)
 {
 	struct kernel_ipmi_msg msg;
 	int rv;
+	unsigned long ret;
 	struct ipmi_system_interface_addr addr;
 	int timeout_retries = 0;
 
 restart:
+	if (bmc_reset_shutdown)
+		return -ENODEV;
+
+	if (atomic_read(&msg_in_flight))
+		return -EBUSY;
+
 	/*
 	 * Don't reset the timer if we have the timer turned off, that
 	 * re-enables the watchdog.
@@ -521,6 +556,8 @@ static int __ipmi_heartbeat(void)
 	if (ipmi_watchdog_state == WDOG_TIMEOUT_NONE)
 		return 0;
 
+	reinit_completion(&msg_wait);
+	atomic_set(&msg_in_flight, 1);
 	atomic_set(&msg_tofree, 2);
 
 	addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
@@ -541,14 +578,17 @@ static int __ipmi_heartbeat(void)
 				      1);
 	if (rv) {
 		atomic_set(&msg_tofree, 0);
-		pr_warn("heartbeat send failure: %d\n", rv);
+		atomic_set(&msg_in_flight, 0);
 		return rv;
 	}
 
-	/* Wait for the heartbeat to be sent. */
-	wait_for_completion(&msg_wait);
+	ret = wait_for_completion_timeout(&msg_wait, IPMI_HEARTBEAT_WAIT_TIMEOUT);
+	if (ret == 0) {
+		atomic_set(&msg_tofree, 0);
+		return -ETIMEDOUT;
+	}
 
-	if (recv_msg.msg.data[0] == IPMI_WDOG_TIMER_NOT_INIT_RESP)  {
+	if (recv_msg.msg.data[0] >= 0x80) {
 		timeout_retries++;
 		if (timeout_retries > 3) {
 			pr_err("Unable to restore the IPMI watchdog's settings, giving up\n");
@@ -557,12 +597,11 @@ static int __ipmi_heartbeat(void)
 		}
 
 		/*
-		 * The timer was not initialized, that means the BMC was
-		 * probably reset and lost the watchdog information.  Attempt
-		 * to restore the timer's info.  Note that we still hold
-		 * the heartbeat lock, to keep a heartbeat from happening
-		 * in this process, so must say no heartbeat to avoid a
-		 * deadlock on this mutex
+		 * The BMC was probably reset and lost the watchdog
+		 * information.  Attempt to restore the timer's info.
+		 * Note that we still hold the heartbeat lock, to keep
+		 * a heartbeat from happening in this process, so must
+		 * say no heartbeat to avoid a deadlock on this mutex.
 		 */
 		rv = _ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB);
 		if (rv) {
@@ -876,15 +915,38 @@ static struct miscdevice ipmi_wdog_miscdev = {
 static void ipmi_wdog_msg_handler(struct ipmi_recv_msg *msg,
 				  void                 *handler_data)
 {
-	if (msg->msg.cmd == IPMI_WDOG_RESET_TIMER &&
-			msg->msg.data[0] == IPMI_WDOG_TIMER_NOT_INIT_RESP)
-		pr_info("response: The IPMI controller appears to have been reset, will attempt to reinitialize the watchdog timer\n");
-	else if (msg->msg.data[0] != 0)
-		pr_err("response: Error %x on cmd %x\n",
-		       msg->msg.data[0],
-		       msg->msg.cmd);
+	if (msg->msg.data[0] != 0) {
+		if (msg->msg.data[0] == IPMI_WDOG_TIMER_NOT_INIT_RESP)
+			pr_crit("BMC error: watchdog timer not initialized "
+				"(0x%02x on cmd 0x%02x)\n",
+				msg->msg.data[0], msg->msg.cmd);
+		else if (msg->msg.data[0] > 0x80 &&
+			 msg->msg.data[0] <= 0xBE)
+			pr_crit("BMC error: vendor-specific completion code "
+				"0x%02x on cmd 0x%02x\n",
+				msg->msg.data[0], msg->msg.cmd);
+		else
+			pr_crit("BMC error: completion code 0x%02x "
+				"on cmd 0x%02x\n",
+				msg->msg.data[0], msg->msg.cmd);
+
+		if (ipmi_watchdog_state != WDOG_TIMEOUT_NONE &&
+		    !bmc_reset_shutdown) {
+			bmc_reset_shutdown = true;
+			pr_crit("BMC communication lost with watchdog active, "
+				"initiating system reboot\n");
+			orderly_reboot();
+		}
+	}
 
 	ipmi_free_recv_msg(msg);
+	/*
+	 * Ensure the in-flight flag is cleared after the message is freed.
+	 * In the normal path this is redundant (already cleared by the
+	 * recv_msg destructor). For late responses arriving after a
+	 * completion timeout, this is the only path that clears the flag.
+	 */
+	atomic_set(&msg_in_flight, 0);
 }
 
 static void ipmi_wdog_pretimeout_handler(void *handler_data)
@@ -1106,6 +1168,9 @@ static int wdog_reboot_handler(struct notifier_block *this,
 		/* Make sure we only do this once. */
 		reboot_event_handled = 1;
 
+		if (bmc_reset_shutdown)
+			return NOTIFY_OK;
+
 		if (code == SYS_POWER_OFF || code == SYS_HALT) {
 			/* Disable the WDT if we are shutting down. */
 			ipmi_watchdog_state = WDOG_TIMEOUT_NONE;
-- 
2.53.0