From 2512e40e48d21d8bac09f7e91d2c3ceb2d3b50b2 Mon Sep 17 00:00:00 2001
From: Corey Minyard <cminyard@mvista.com>
Date: Wed, 22 Aug 2018 12:08:13 -0500
Subject: [PATCH] ipmi: Rework SMI registration failure
Git-commit: 2512e40e48d21d8bac09f7e91d2c3ceb2d3b50b2
Patch-mainline: v4.19-rc4
References: FATE#326156
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
Acked-by: Takashi Iwai <tiwai@suse.de>
---
drivers/char/ipmi/ipmi_msghandler.c | 57 +++++++++++++++++++-----------------
drivers/char/ipmi/ipmi_si_intf.c | 17 ++--------
drivers/char/ipmi/ipmi_ssif.c | 13 +-------
3 files changed, 38 insertions(+), 49 deletions(-)
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -3624,45 +3624,49 @@ int ipmi_register_smi(const struct ipmi_
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;
+ goto out_err_bmc_reg;
rv = add_proc_entries(intf, i);
+ if (rv)
+ goto out_err_bmc_reg;
- out:
- if (rv) {
- ipmi_bmc_unregister(intf);
- if (intf->proc_dir)
- remove_proc_entries(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;
}
@@ -3753,7 +3757,8 @@ void ipmi_unregister_smi(struct ipmi_smi
}
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);
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2107,18 +2107,9 @@ static int try_smi_init(struct smi_info
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;
}
@@ -2247,6 +2238,8 @@ static void shutdown_smi(void *send_info
kfree(smi_info->si_sm);
smi_info->si_sm = NULL;
+
+ smi_info->intf = NULL;
}
/*
@@ -2260,10 +2253,8 @@ static void cleanup_one_si(struct smi_in
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)
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1156,18 +1156,11 @@ static void shutdown_ssif(void *send_inf
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)
@@ -1177,9 +1170,7 @@ static int ssif_remove(struct i2c_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) {
@@ -1188,6 +1179,8 @@ static int ssif_remove(struct i2c_client
}
}
+ kfree(ssif_info);
+
return 0;
}