ipmi: Fix a race restarting the timer

With recent changes it is possible for the timer handler to detect an idle interface and not start the timer, but the thread to start an operation at the same time. The thread will not start the timer in that instance, resulting in the timer not running. Instead, move all timer operations under the lock and start the timer in the thread if it detect non-idle and the timer is not already running. Moving under locks allows the last timeout to be set in both the thread and the timer. 'Timer is not running' means that the timer is not pending and smi_timeout() is not running. So we need a flag to detect this correctly. Also fix a few other timeout bugs: setting the last timeout when the interrupt has to be disabled and the timer started, and setting the last timeout in check_start_timer_thread possibly racing with the timer Signed-off-by: Corey Minyard <cminyard@mvista.com> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

ipmi: Fix a race restarting the timer
With recent changes it is possible for the timer handler to detect an idle interface and not start the timer, but the thread to start an operation at the same time. The thread will not start the timer in that instance, resulting in the timer not running. Instead, move all timer operations under the lock and start the timer in the thread if it detect non-idle and the timer is not already running. Moving under locks allows the last timeout to be set in both the thread and the timer. 'Timer is not running' means that the timer is not pending and smi_timeout() is not running. So we need a flag to detect this correctly. Also fix a few other timeout bugs: setting the last timeout when the interrupt has to be disabled and the timer started, and setting the last timeout in check_start_timer_thread possibly racing with the timer Signed-off-by: Corey Minyard <cminyard@mvista.com> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Bodo Stroesser · Linus Torvalds · Eric Lee · Eric Lee · Eric Lee · Eric Lee
1 parent a94cdd1f4d
Showing 1 changed file with 28 additions and 18 deletions Side-by-side Diff
drivers/char/ipmi/ipmi_si_intf.c
@@ -248,6 +248,9 @@
 	/* The timer for this si. */
 	struct timer_list   si_timer;
  
+	/* This flag is set, if the timer is running (timer_pending() isn't enough) */
+	bool		    timer_running;
+
 	/* The time (in jiffies) the last timeout occurred at. */
 	unsigned long       last_timeout_jiffies;
  
@@ -434,6 +437,13 @@
 	smi_info->si_state = SI_CLEARING_FLAGS;
 }
  
+static void smi_mod_timer(struct smi_info *smi_info, unsigned long new_val)
+{
+	smi_info->last_timeout_jiffies = jiffies;
+	mod_timer(&smi_info->si_timer, new_val);
+	smi_info->timer_running = true;
+}
+
 /*
  * When we have a situtaion where we run out of memory and cannot
  * allocate messages, we just leave them in the BMC and run the system
@@ -446,8 +456,7 @@
 		start_disable_irq(smi_info);
 		smi_info->interrupt_disabled = 1;
 		if (!atomic_read(&smi_info->stop_operation))
-			mod_timer(&smi_info->si_timer,
-				  jiffies + SI_TIMEOUT_JIFFIES);
+			smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
 	}
 }
  
  
@@ -907,16 +916,8 @@
 		list_add_tail(&msg->link, &smi_info->xmit_msgs);
  
 	if (smi_info->si_state == SI_NORMAL && smi_info->curr_msg == NULL) {
-		/*
-		 * last_timeout_jiffies is updated here to avoid
-		 * smi_timeout() handler passing very large time_diff
-		 * value to smi_event_handler() that causes
-		 * the send command to abort.
-		 */
-		smi_info->last_timeout_jiffies = jiffies;
+		smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
  
-		mod_timer(&smi_info->si_timer, jiffies + SI_TIMEOUT_JIFFIES);
-
 		if (smi_info->thread)
 			wake_up_process(smi_info->thread);
  
@@ -1004,6 +1005,17 @@
  
 		spin_lock_irqsave(&(smi_info->si_lock), flags);
 		smi_result = smi_event_handler(smi_info, 0);
+
+		/*
+		 * If the driver is doing something, there is a possible
+		 * race with the timer.  If the timer handler see idle,
+		 * and the thread here sees something else, the timer
+		 * handler won't restart the timer even though it is
+		 * required.  So start it here if necessary.
+		 */
+		if (smi_result != SI_SM_IDLE && !smi_info->timer_running)
+			smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
+
 		spin_unlock_irqrestore(&(smi_info->si_lock), flags);
 		busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
 						  &busy_until);
@@ -1073,10 +1085,6 @@
 		     * SI_USEC_PER_JIFFY);
 	smi_result = smi_event_handler(smi_info, time_diff);
  
-	spin_unlock_irqrestore(&(smi_info->si_lock), flags);
-
-	smi_info->last_timeout_jiffies = jiffies_now;
-
 	if ((smi_info->irq) && (!smi_info->interrupt_disabled)) {
 		/* Running with interrupts, only do long timeouts. */
 		timeout = jiffies + SI_TIMEOUT_JIFFIES;
@@ -1098,7 +1106,10 @@
  
  do_mod_timer:
 	if (smi_result != SI_SM_IDLE)
-		mod_timer(&(smi_info->si_timer), timeout);
+		smi_mod_timer(smi_info, timeout);
+	else
+		smi_info->timer_running = false;
+	spin_unlock_irqrestore(&(smi_info->si_lock), flags);
 }
  
 static irqreturn_t si_irq_handler(int irq, void *data)
@@ -1146,8 +1157,7 @@
  
 	/* Set up the timer that drives the interface. */
 	setup_timer(&new_smi->si_timer, smi_timeout, (long)new_smi);
-	new_smi->last_timeout_jiffies = jiffies;
-	mod_timer(&new_smi->si_timer, jiffies + SI_TIMEOUT_JIFFIES);
+	smi_mod_timer(new_smi, jiffies + SI_TIMEOUT_JIFFIES);
  
 	/*
 	 * Check if the user forcefully enabled the daemon.
...	...	@@ -248,6 +248,9 @@
248	248	/* The timer for this si. */
249	249	struct timer_list si_timer;
250	250
	251	+ /* This flag is set, if the timer is running (timer_pending() isn't enough) */
	252	+ bool timer_running;
	253	+
251	254	/* The time (in jiffies) the last timeout occurred at. */
252	255	unsigned long last_timeout_jiffies;
253	256
...	...	@@ -434,6 +437,13 @@
434	437	smi_info->si_state = SI_CLEARING_FLAGS;
435	438	}
436	439
	440	+static void smi_mod_timer(struct smi_info *smi_info, unsigned long new_val)
	441	+{
	442	+ smi_info->last_timeout_jiffies = jiffies;
	443	+ mod_timer(&smi_info->si_timer, new_val);
	444	+ smi_info->timer_running = true;
	445	+}
	446	+
437	447	/*
438	448	* When we have a situtaion where we run out of memory and cannot
439	449	* allocate messages, we just leave them in the BMC and run the system
...	...	@@ -446,8 +456,7 @@
446	456	start_disable_irq(smi_info);
447	457	smi_info->interrupt_disabled = 1;
448	458	if (!atomic_read(&smi_info->stop_operation))
449		- mod_timer(&smi_info->si_timer,
450		- jiffies + SI_TIMEOUT_JIFFIES);
	459	+ smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
451	460	}
452	461	}
453	462
454	463
...	...	@@ -907,16 +916,8 @@
907	916	list_add_tail(&msg->link, &smi_info->xmit_msgs);
908	917
909	918	if (smi_info->si_state == SI_NORMAL && smi_info->curr_msg == NULL) {
910		- /*
911		- * last_timeout_jiffies is updated here to avoid
912		- * smi_timeout() handler passing very large time_diff
913		- * value to smi_event_handler() that causes
914		- * the send command to abort.
915		- */
916		- smi_info->last_timeout_jiffies = jiffies;
	919	+ smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
917	920
918		- mod_timer(&smi_info->si_timer, jiffies + SI_TIMEOUT_JIFFIES);
919		-
920	921	if (smi_info->thread)
921	922	wake_up_process(smi_info->thread);
922	923
...	...	@@ -1004,6 +1005,17 @@
1004	1005
1005	1006	spin_lock_irqsave(&(smi_info->si_lock), flags);
1006	1007	smi_result = smi_event_handler(smi_info, 0);
	1008	+
	1009	+ /*
	1010	+ * If the driver is doing something, there is a possible
	1011	+ * race with the timer. If the timer handler see idle,
	1012	+ * and the thread here sees something else, the timer
	1013	+ * handler won't restart the timer even though it is
	1014	+ * required. So start it here if necessary.
	1015	+ */
	1016	+ if (smi_result != SI_SM_IDLE && !smi_info->timer_running)
	1017	+ smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
	1018	+
1007	1019	spin_unlock_irqrestore(&(smi_info->si_lock), flags);
1008	1020	busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
1009	1021	&busy_until);
...	...	@@ -1073,10 +1085,6 @@
1073	1085	* SI_USEC_PER_JIFFY);
1074	1086	smi_result = smi_event_handler(smi_info, time_diff);
1075	1087
1076		- spin_unlock_irqrestore(&(smi_info->si_lock), flags);
1077		-
1078		- smi_info->last_timeout_jiffies = jiffies_now;
1079		-
1080	1088	if ((smi_info->irq) && (!smi_info->interrupt_disabled)) {
1081	1089	/* Running with interrupts, only do long timeouts. */
1082	1090	timeout = jiffies + SI_TIMEOUT_JIFFIES;
...	...	@@ -1098,7 +1106,10 @@
1098	1106
1099	1107	do_mod_timer:
1100	1108	if (smi_result != SI_SM_IDLE)
1101		- mod_timer(&(smi_info->si_timer), timeout);
	1109	+ smi_mod_timer(smi_info, timeout);
	1110	+ else
	1111	+ smi_info->timer_running = false;
	1112	+ spin_unlock_irqrestore(&(smi_info->si_lock), flags);
1102	1113	}
1103	1114
1104	1115	static irqreturn_t si_irq_handler(int irq, void *data)
...	...	@@ -1146,8 +1157,7 @@
1146	1157
1147	1158	/* Set up the timer that drives the interface. */
1148	1159	setup_timer(&new_smi->si_timer, smi_timeout, (long)new_smi);
1149		- new_smi->last_timeout_jiffies = jiffies;
1150		- mod_timer(&new_smi->si_timer, jiffies + SI_TIMEOUT_JIFFIES);
	1160	+ smi_mod_timer(new_smi, jiffies + SI_TIMEOUT_JIFFIES);
1151	1161
1152	1162	/*
1153	1163	* Check if the user forcefully enabled the daemon.