Tested-by: Justin Ernst <justin.er...@hpe.com> Passed the testing. Thanks Corey! -Justin
> -----Original Message----- > From: Corey Minyard [mailto:tcminy...@gmail.com] On Behalf Of > miny...@acm.org > Sent: Wednesday, August 22, 2018 12:20 PM > To: Ernst, Justin <justin.er...@hpe.com> > Cc: Corey Minyard <cminy...@mvista.com>; Banman, Andrew > <aban...@hpe.com>; Anderson, Russ <russ.ander...@hpe.com>; > openipmi-developer@lists.sourceforge.net; linux-ker...@vger.kernel.org > Subject: [PATCH] ipmi: Rework SMI registration failure > > From: Corey Minyard <cminy...@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 <cminy...@mvista.com> > Reported-by: Justin Ernst <justin.er...@hpe.com> > Cc: Andrew Banman <aban...@hpe.com> > Cc: Russ Anderson <russ.ander...@hpe.com> > Cc: openipmi-developer@lists.sourceforge.net > Cc: linux-ker...@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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
