[PATCH] ipmi: Rework SMI registration failure

[PATCH] ipmi: Rework SMI registration failure

[minyard]

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

RE: [PATCH] ipmi: Rework SMI registration failure

[Ernst, Justin]

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