| Message ID | 1534958412-27199-1-git-send-email-minyard@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | ipmi: Rework SMI registration failure | expand |
Tested-by: Justin Ernst <justin.ernst@hpe.com> Passed the testing. Thanks Corey! -Justin > -----Original Message----- > From: Corey Minyard [mailto:tcminyard@gmail.com] On Behalf Of > minyard@acm.org > Sent: Wednesday, August 22, 2018 12:20 PM > To: Ernst, Justin <justin.ernst@hpe.com> > Cc: Corey Minyard <cminyard@mvista.com>; Banman, Andrew > <abanman@hpe.com>; Anderson, Russ <russ.anderson@hpe.com>; > openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org > Subject: [PATCH] ipmi: Rework SMI registration failure > > From: Corey Minyard <cminyard@mvista.com> > > 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> > Cc: Andrew Banman <abanman@hpe.com> > Cc: Russ Anderson <russ.anderson@hpe.com> > Cc: openipmi-developer@lists.sourceforge.net > Cc: linux-kernel@vger.kernel.org > --- > drivers/char/ipmi/ipmi_msghandler.c | 53 +++++++++++++++++++++-------- > -------- > drivers/char/ipmi/ipmi_si_intf.c | 17 +++--------- > drivers/char/ipmi/ipmi_ssif.c | 13 +++------ > 3 files changed, 37 insertions(+), 46 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c > b/drivers/char/ipmi/ipmi_msghandler.c > index 46ab1ba..04f64c5 100644 > --- a/drivers/char/ipmi/ipmi_msghandler.c > +++ b/drivers/char/ipmi/ipmi_msghandler.c > @@ -3378,39 +3378,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; > } > @@ -3501,7 +3507,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 2194b4c..bb7d8af 100644 > --- a/drivers/char/ipmi/ipmi_si_intf.c > +++ b/drivers/char/ipmi/ipmi_si_intf.c > @@ -2102,18 +2102,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; > } > > @@ -2246,6 +2237,8 @@ static void shutdown_smi(void *send_info) > > kfree(smi_info->si_sm); > smi_info->si_sm = NULL; > + > + smi_info->intf = NULL; > } > > /* > @@ -2259,10 +2252,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 fddf646..3574322 100644 > --- a/drivers/char/ipmi/ipmi_ssif.c > +++ b/drivers/char/ipmi/ipmi_ssif.c > @@ -1218,18 +1218,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) > @@ -1239,9 +1232,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) { > @@ -1250,6 +1241,8 @@ static int ssif_remove(struct i2c_client *client) > } > } > > + kfree(ssif_info); > + > return 0; > } > > -- > 2.7.4
diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c index 46ab1ba..04f64c5 100644 --- a/drivers/char/ipmi/ipmi_msghandler.c +++ b/drivers/char/ipmi/ipmi_msghandler.c @@ -3378,39 +3378,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; } @@ -3501,7 +3507,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 2194b4c..bb7d8af 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -2102,18 +2102,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; } @@ -2246,6 +2237,8 @@ static void shutdown_smi(void *send_info) kfree(smi_info->si_sm); smi_info->si_sm = NULL; + + smi_info->intf = NULL; } /* @@ -2259,10 +2252,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 fddf646..3574322 100644 --- a/drivers/char/ipmi/ipmi_ssif.c +++ b/drivers/char/ipmi/ipmi_ssif.c @@ -1218,18 +1218,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) @@ -1239,9 +1232,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) { @@ -1250,6 +1241,8 @@ static int ssif_remove(struct i2c_client *client) } } + kfree(ssif_info); + return 0; }