ipmi: Stop timers before cleaning up the module

System may crash after unloading ipmi_si.ko module
because a timer may remain and fire after the module cleaned up resources.

cleanup_one_si() contains the following processing.

        /*
         * Make sure that interrupts, the timer and the thread are
         * stopped and will not run again.
         */
        if (to_clean->irq_cleanup)
                to_clean->irq_cleanup(to_clean);
        wait_for_timer_and_thread(to_clean);

        /*
         * Timeouts are stopped, now make sure the interrupts are off
         * in the BMC.  Note that timers and CPU interrupts are off,
         * so no need for locks.
         */
        while (to_clean->curr_msg || (to_clean->si_state != SI_NORMAL)) {
                poll(to_clean);
                schedule_timeout_uninterruptible(1);
        }

si_state changes as following in the while loop calling poll(to_clean).

  SI_GETTING_MESSAGES
    => SI_CHECKING_ENABLES
     => SI_SETTING_ENABLES
      => SI_GETTING_EVENTS
       => SI_NORMAL

As written in the code comments above,
timers are expected to stop before the polling loop and not to run again.
But the timer is set again in the following process
when si_state becomes SI_SETTING_ENABLES.

  => poll
     => smi_event_handler
       => handle_transaction_done
          // smi_info->si_state == SI_SETTING_ENABLES
         => start_getting_events
           => start_new_msg
            => smi_mod_timer
              => mod_timer

As a result, before the timer set in start_new_msg() expires,
the polling loop may see si_state becoming SI_NORMAL
and the module clean-up finishes.

For example, hard LOCKUP and panic occurred as following.
smi_timeout was called after smi_event_handler,
kcs_event and hangs at port_inb()
trying to access I/O port after release.

    [exception RIP: port_inb+19]
    RIP: ffffffffc0473053  RSP: ffff88069fdc3d80  RFLAGS: 00000006
    RAX: ffff8806800f8e00  RBX: ffff880682bd9400  RCX: 0000000000000000
    RDX: 0000000000000ca3  RSI: 0000000000000ca3  RDI: ffff8806800f8e40
    RBP: ffff88069fdc3d80   R8: ffffffff81d86dfc   R9: ffffffff81e36426
    R10: 00000000000509f0  R11: 0000000000100000  R12: 0000000000]:000000
    R13: 0000000000000000  R14: 0000000000000246  R15: ffff8806800f8e00
    ORIG_RAX: ffffffffffffffff  CS: 0010  SS: 0000
 --- <NMI exception stack> ---

To fix the problem I defined a flag, timer_can_start,
as member of struct smi_info.
The flag is enabled immediately after initializing the timer
and disabled immediately before waiting for timer deletion.

Fixes: 0cfec916e86d ("ipmi: Start the timer and thread on internal msgs")
Signed-off-by: Yamazaki Masamitsu <m-yamazaki@ah.jp.nec.com>
[Adjusted for recent changes in the driver.]
Signed-off-by: Corey Minyard <cminyard@mvista.com>

1 files changed, 23 insertions, 21 deletions

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 71d33a1807e4..99b0513bb55b 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -199,6 +199,9 @@ struct smi_info {
         /* The timer for this si. */
         struct timer_list   si_timer;
 
+        /* This flag is set, if the timer can be set */
+        bool                timer_can_start;
+
         /* This flag is set, if the timer is running (timer_pending() isn't enough) */
         bool                timer_running;
 
@@ -355,6 +358,8 @@ out:
 
 static void smi_mod_timer(struct smi_info *smi_info, unsigned long new_val)
 {
+        if (!smi_info->timer_can_start)
+                return;
         smi_info->last_timeout_jiffies = jiffies;
         mod_timer(&smi_info->si_timer, new_val);
         smi_info->timer_running = true;
@@ -374,21 +379,18 @@ static void start_new_msg(struct smi_info *smi_info, unsigned char *msg,
         smi_info->handlers->start_transaction(smi_info->si_sm, msg, size);
 }
 
-static void start_check_enables(struct smi_info *smi_info, bool start_timer)
+static void start_check_enables(struct smi_info *smi_info)
 {
         unsigned char msg[2];
 
         msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
         msg[1] = IPMI_GET_BMC_GLOBAL_ENABLES_CMD;
 
-        if (start_timer)
-                start_new_msg(smi_info, msg, 2);
-        else
-                smi_info->handlers->start_transaction(smi_info->si_sm, msg, 2);
+        start_new_msg(smi_info, msg, 2);
         smi_info->si_state = SI_CHECKING_ENABLES;
 }
 
-static void start_clear_flags(struct smi_info *smi_info, bool start_timer)
+static void start_clear_flags(struct smi_info *smi_info)
 {
         unsigned char msg[3];
 
@@ -397,10 +399,7 @@ static void start_clear_flags(struct smi_info *smi_info, bool start_timer)
         msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD;
         msg[2] = WDT_PRE_TIMEOUT_INT;
 
-        if (start_timer)
-                start_new_msg(smi_info, msg, 3);
-        else
-                smi_info->handlers->start_transaction(smi_info->si_sm, msg, 3);
+        start_new_msg(smi_info, msg, 3);
         smi_info->si_state = SI_CLEARING_FLAGS;
 }
 
@@ -435,11 +434,11 @@ static void start_getting_events(struct smi_info *smi_info)
  * Note that we cannot just use disable_irq(), since the interrupt may
  * be shared.
  */
-static inline bool disable_si_irq(struct smi_info *smi_info, bool start_timer)
+static inline bool disable_si_irq(struct smi_info *smi_info)
 {
         if ((smi_info->io.irq) && (!smi_info->interrupt_disabled)) {
                 smi_info->interrupt_disabled = true;
-                start_check_enables(smi_info, start_timer);
+                start_check_enables(smi_info);
                 return true;
         }
         return false;
@@ -449,7 +448,7 @@ static inline bool enable_si_irq(struct smi_info *smi_info)
 {
         if ((smi_info->io.irq) && (smi_info->interrupt_disabled)) {
                 smi_info->interrupt_disabled = false;
-                start_check_enables(smi_info, true);
+                start_check_enables(smi_info);
                 return true;
         }
         return false;
@@ -467,7 +466,7 @@ static struct ipmi_smi_msg *alloc_msg_handle_irq(struct smi_info *smi_info)
 
         msg = ipmi_alloc_smi_msg();
         if (!msg) {
-                if (!disable_si_irq(smi_info, true))
+                if (!disable_si_irq(smi_info))
                         smi_info->si_state = SI_NORMAL;
         } else if (enable_si_irq(smi_info)) {
                 ipmi_free_smi_msg(msg);
@@ -483,7 +482,7 @@ retry:
                 /* Watchdog pre-timeout */
                 smi_inc_stat(smi_info, watchdog_pretimeouts);
 
-                start_clear_flags(smi_info, true);
+                start_clear_flags(smi_info);
                 smi_info->msg_flags &= ~WDT_PRE_TIMEOUT_INT;
                 if (smi_info->intf)
                         ipmi_smi_watchdog_pretimeout(smi_info->intf);
@@ -866,7 +865,7 @@ restart:
                  * disable and messages disabled.
                  */
                 if (smi_info->supports_event_msg_buff || smi_info->io.irq) {
-                        start_check_enables(smi_info, true);
+                        start_check_enables(smi_info);
                 } else {
                         smi_info->curr_msg = alloc_msg_handle_irq(smi_info);
                         if (!smi_info->curr_msg)
@@ -1167,6 +1166,7 @@ static int smi_start_processing(void       *send_info,
 
         /* Set up the timer that drives the interface. */
         setup_timer(&new_smi->si_timer, smi_timeout, (long)new_smi);
+        new_smi->timer_can_start = true;
         smi_mod_timer(new_smi, jiffies + SI_TIMEOUT_JIFFIES);
 
         /* Try to claim any interrupts. */
@@ -1936,10 +1936,12 @@ static void check_for_broken_irqs(struct smi_info *smi_info)
         check_set_rcv_irq(smi_info);
 }
 
-static inline void wait_for_timer_and_thread(struct smi_info *smi_info)
+static inline void stop_timer_and_thread(struct smi_info *smi_info)
 {
         if (smi_info->thread != NULL)
                 kthread_stop(smi_info->thread);
+
+        smi_info->timer_can_start = false;
         if (smi_info->timer_running)
                 del_timer_sync(&smi_info->si_timer);
 }
@@ -2152,7 +2154,7 @@ static int try_smi_init(struct smi_info *new_smi)
          * Start clearing the flags before we enable interrupts or the
          * timer to avoid racing with the timer.
          */
-        start_clear_flags(new_smi, false);
+        start_clear_flags(new_smi);
 
         /*
          * IRQ is defined to be set when non-zero.  req_events will
@@ -2238,7 +2240,7 @@ out_err_remove_attrs:
         dev_set_drvdata(new_smi->io.dev, NULL);
 
 out_err_stop_timer:
-        wait_for_timer_and_thread(new_smi);
+        stop_timer_and_thread(new_smi);
 
 out_err:
         new_smi->interrupt_disabled = true;
@@ -2388,7 +2390,7 @@ static void cleanup_one_si(struct smi_info *to_clean)
          */
         if (to_clean->io.irq_cleanup)
                 to_clean->io.irq_cleanup(&to_clean->io);
-        wait_for_timer_and_thread(to_clean);
+        stop_timer_and_thread(to_clean);
 
         /*
          * Timeouts are stopped, now make sure the interrupts are off
@@ -2400,7 +2402,7 @@ static void cleanup_one_si(struct smi_info *to_clean)
                 schedule_timeout_uninterruptible(1);
         }
         if (to_clean->handlers)
-                disable_si_irq(to_clean, false);
+                disable_si_irq(to_clean);
         while (to_clean->curr_msg || (to_clean->si_state != SI_NORMAL)) {
                 poll(to_clean);
                 schedule_timeout_uninterruptible(1);