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

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 51832b8a2c6283f9b1ccfd07a60997fdade8d4cf..7fc9612070a1f1abe43b489f226c84a9c4c50632 100644 (file)
--- 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 90ec010bffbd9776c012586b4e01b24cdd0bd2d6..5faa917df1b629647cb7fecd291a1d1b8d80eae9 100644 (file)
--- 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 18e4650c233b1de514ee836dfc28021f35953a5b..c12edc8e91dfd50361d9dd53cd9735cb5dd86249 100644 (file)
--- 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;
 }