ipmi: Rework SMI registration failure

There were certain situations where ipmi_register_smi() would
return a failure, but the interface would still be registered
and would need to be unregistered.  This is obviously a bad
design and resulted in an oops in certain failure cases.

If the interface is started up in ipmi_register_smi(), then
an error occurs, shut down the interface there so the
cleanup can be done properly.

Fix the various smi users, too.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Reported-by: Justin Ernst <justin.ernst@hpe.com>
Tested-by: Justin Ernst <justin.ernst@hpe.com>
Cc: Andrew Banman <abanman@hpe.com>
Cc: Russ Anderson <russ.anderson@hpe.com>
Cc: <stable@vger.kernel.org> # 4.18.x

3 files changed, 37 insertions, 46 deletions

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 51832b8a2c62..7fc9612070a1 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -3381,39 +3381,45 @@ int ipmi_register_smi(const struct ipmi_smi_handlers *handlers,
 
         rv = handlers->start_processing(send_info, intf);
         if (rv)
-                goto out;
+                goto out_err;
 
         rv = __bmc_get_device_id(intf, NULL, &id, NULL, NULL, i);
         if (rv) {
                 dev_err(si_dev, "Unable to get the device id: %d\n", rv);
-                goto out;
+                goto out_err_started;
         }
 
         mutex_lock(&intf->bmc_reg_mutex);
         rv = __scan_channels(intf, &id);
         mutex_unlock(&intf->bmc_reg_mutex);
+        if (rv)
+                goto out_err_bmc_reg;
 
- out:
-        if (rv) {
-                ipmi_bmc_unregister(intf);
-                list_del_rcu(&intf->link);
-                mutex_unlock(&ipmi_interfaces_mutex);
-                synchronize_srcu(&ipmi_interfaces_srcu);
-                cleanup_srcu_struct(&intf->users_srcu);
-                kref_put(&intf->refcount, intf_free);
-        } else {
-                /*
-                 * Keep memory order straight for RCU readers.  Make
-                 * sure everything else is committed to memory before
-                 * setting intf_num to mark the interface valid.
-                 */
-                smp_wmb();
-                intf->intf_num = i;
-                mutex_unlock(&ipmi_interfaces_mutex);
+        /*
+         * Keep memory order straight for RCU readers.  Make
+         * sure everything else is committed to memory before
+         * setting intf_num to mark the interface valid.
+         */
+        smp_wmb();
+        intf->intf_num = i;
+        mutex_unlock(&ipmi_interfaces_mutex);
 
-                /* After this point the interface is legal to use. */
-                call_smi_watchers(i, intf->si_dev);
-        }
+        /* After this point the interface is legal to use. */
+        call_smi_watchers(i, intf->si_dev);
+
+        return 0;
+
+ out_err_bmc_reg:
+        ipmi_bmc_unregister(intf);
+ out_err_started:
+        if (intf->handlers->shutdown)
+                intf->handlers->shutdown(intf->send_info);
+ out_err:
+        list_del_rcu(&intf->link);
+        mutex_unlock(&ipmi_interfaces_mutex);
+        synchronize_srcu(&ipmi_interfaces_srcu);
+        cleanup_srcu_struct(&intf->users_srcu);
+        kref_put(&intf->refcount, intf_free);
 
         return rv;
 }
@@ -3504,7 +3510,8 @@ void ipmi_unregister_smi(struct ipmi_smi *intf)
         }
         srcu_read_unlock(&intf->users_srcu, index);
 
-        intf->handlers->shutdown(intf->send_info);
+        if (intf->handlers->shutdown)
+                intf->handlers->shutdown(intf->send_info);
 
         cleanup_smi_msgs(intf);
 
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 90ec010bffbd..5faa917df1b6 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2083,18 +2083,9 @@ static int try_smi_init(struct smi_info *new_smi)
                  si_to_str[new_smi->io.si_type]);
 
         WARN_ON(new_smi->io.dev->init_name != NULL);
-        kfree(init_name);
-
-        return 0;
-
-out_err:
-        if (new_smi->intf) {
-                ipmi_unregister_smi(new_smi->intf);
-                new_smi->intf = NULL;
-        }
 
+ out_err:
         kfree(init_name);
-
         return rv;
 }
 
@@ -2227,6 +2218,8 @@ static void shutdown_smi(void *send_info)
 
         kfree(smi_info->si_sm);
         smi_info->si_sm = NULL;
+
+        smi_info->intf = NULL;
 }
 
 /*
@@ -2240,10 +2233,8 @@ static void cleanup_one_si(struct smi_info *smi_info)
 
         list_del(&smi_info->link);
 
-        if (smi_info->intf) {
+        if (smi_info->intf)
                 ipmi_unregister_smi(smi_info->intf);
-                smi_info->intf = NULL;
-        }
 
         if (smi_info->pdev) {
                 if (smi_info->pdev_registered)
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 18e4650c233b..c12edc8e91df 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1214,18 +1214,11 @@ static void shutdown_ssif(void *send_info)
                 complete(&ssif_info->wake_thread);
                 kthread_stop(ssif_info->thread);
         }
-
-        /*
-         * No message can be outstanding now, we have removed the
-         * upper layer and it permitted us to do so.
-         */
-        kfree(ssif_info);
 }
 
 static int ssif_remove(struct i2c_client *client)
 {
         struct ssif_info *ssif_info = i2c_get_clientdata(client);
-        struct ipmi_smi *intf;
         struct ssif_addr_info *addr_info;
 
         if (!ssif_info)
@@ -1235,9 +1228,7 @@ static int ssif_remove(struct i2c_client *client)
          * After this point, we won't deliver anything asychronously
          * to the message handler.  We can unregister ourself.
          */
-        intf = ssif_info->intf;
-        ssif_info->intf = NULL;
-        ipmi_unregister_smi(intf);
+        ipmi_unregister_smi(ssif_info->intf);
 
         list_for_each_entry(addr_info, &ssif_infos, link) {
                 if (addr_info->client == client) {
@@ -1246,6 +1237,8 @@ static int ssif_remove(struct i2c_client *client)
                 }
         }
 
+        kfree(ssif_info);
+
         return 0;
 }