[PATCH] scsi: scsi_transport_iscsi: use put_device() instead of kfree()

[PATCH] scsi: scsi_transport_iscsi: use put_device() instead of kfree()

[Arvind Yadav]

Never directly free @dev after calling device_register(), even
if it returned an error! Always use put_device() to give up the
reference initialized.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index f4b52b4..aacb7ab 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -221,8 +221,10 @@ struct iscsi_endpoint *
 	ep->dev.class = &iscsi_endpoint_class;
 	dev_set_name(&ep->dev, "ep-%llu", (unsigned long long) id);
 	err = device_register(&ep->dev);
-        if (err)
-                goto free_ep;
+	if (err) {
+		put_device(&ep->dev);
+		return NULL;
+	}
 
 	err = sysfs_create_group(&ep->dev.kobj, &iscsi_endpoint_group);
 	if (err)
@@ -235,10 +237,6 @@ struct iscsi_endpoint *
 unregister_dev:
 	device_unregister(&ep->dev);
 	return NULL;
-
-free_ep:
-	kfree(ep);
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_create_endpoint);
 
@@ -783,7 +781,7 @@ struct iscsi_iface *
 
 free_iface:
 	put_device(iface->dev.parent);
-	kfree(iface);
+	put_device(&iface->dev);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_create_iface);
@@ -1260,7 +1258,7 @@ struct iscsi_bus_flash_session *
 	return fnode_sess;
 
 free_fnode_sess:
-	kfree(fnode_sess);
+	put_device(&fnode_sess->dev);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_create_flashnode_sess);
@@ -1308,7 +1306,7 @@ struct iscsi_bus_flash_conn *
 	return fnode_conn;
 
 free_fnode_conn:
-	kfree(fnode_conn);
+	put_device(&fnode_conn->dev);
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_create_flashnode_conn);
@@ -2268,6 +2266,8 @@ struct iscsi_cls_conn *
 
 release_parent_ref:
 	put_device(&session->dev);
+	put_device(&conn->dev);
+	conn = NULL;
 free_conn:
 	kfree(conn);
 	return NULL;
@@ -4420,8 +4420,10 @@ struct scsi_transport_template *
 	priv->dev.class = &iscsi_transport_class;
 	dev_set_name(&priv->dev, "%s", tt->name);
 	err = device_register(&priv->dev);
-	if (err)
-		goto free_priv;
+	if (err) {
+		put_device(&priv->dev);
+		return NULL;
+	}
 
 	err = sysfs_create_group(&priv->dev.kobj, &iscsi_transport_group);
 	if (err)
@@ -4456,9 +4458,6 @@ struct scsi_transport_template *
 unregister_dev:
 	device_unregister(&priv->dev);
 	return NULL;
-free_priv:
-	kfree(priv);
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(iscsi_register_transport);
 
-- 
1.9.1

Re: [PATCH] scsi: scsi_transport_iscsi: use put_device() instead of kfree()

[Martin K. Petersen]

> Never directly free @dev after calling device_register(), even
> if it returned an error! Always use put_device() to give up the
> reference initialized.

Lee, Chris: Please review!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: scsi_transport_iscsi: use put_device() instead of kfree()

[Chris Leech]

On Wed, Mar 07, 2018 at 05:07:33PM +0530, Arvind Yadav wrote:
> Never directly free @dev after calling device_register(), even
> if it returned an error! Always use put_device() to give up the
> reference initialized.
> 
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 

> @@ -783,7 +781,7 @@ struct iscsi_iface *
>  
>  free_iface:
>  	put_device(iface->dev.parent);
> -	kfree(iface);
> +	put_device(&iface->dev);
>  	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(iscsi_create_iface);

Am I reading the device code correctly that the parent reference is only
released in the unregister path (device_unregister calls device_del) and
so the manual put_device on the parent here is still correct?

Just want to make sure that's still needed with the call to put_device.

Other than that question, I this all looks good.
Thanks.

Signed-off-by: Chris Leech <cleech@redhat.com>

Re: [PATCH] scsi: scsi_transport_iscsi: use put_device() instead of kfree()

[arvindY]

On Thursday 15 March 2018 11:14 PM, Chris Leech wrote:
> On Wed, Mar 07, 2018 at 05:07:33PM +0530, Arvind Yadav wrote:
>> Never directly free @dev after calling device_register(), even
>> if it returned an error! Always use put_device() to give up the
>> reference initialized.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>>   drivers/scsi/scsi_transport_iscsi.c | 27 +++++++++++++--------------
>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> @@ -783,7 +781,7 @@ struct iscsi_iface *
>>   
>>   free_iface:
>>   	put_device(iface->dev.parent);
>> -	kfree(iface);
>> +	put_device(&iface->dev);
>>   	return NULL;
>>   }
>>   EXPORT_SYMBOL_GPL(iscsi_create_iface);
> Am I reading the device code correctly that the parent reference is only
> released in the unregister path (device_unregister calls device_del) and
> so the manual put_device on the parent here is still correct?
>
> Just want to make sure that's still needed with the call to put_device.
The manual put_device on the parent is not correct.
This should be removed for here. we should call
put_device(&iface->dev) only and It'll released
parent reference.

> Other than that question, I this all looks good.
> Thanks.
>
> Signed-off-by: Chris Leech <cleech@redhat.com>
>

~arvind