[PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma

[PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma

[Ming Lei]

Hello Martin,


Fix two memory leaks and one double free issue in alloc/add host
code path.


V3:
	- fix memory leak caused in scsi_host_alloc
	- comment typo suggested by John

V2:
	- add patch 2 for addressing shost leak in case of adding host
	  failure, reported by John Garry.

Ming Lei (3):
  scsi: core: use put_device() to release host
  scsi: core: fix failure handling of scsi_add_host_with_dma
  scsi: core: put ->shost_gendev.parent in failure handling path

 drivers/scsi/hosts.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: John Garry <john.garry@huawei.com>
Cc: Hannes Reinecke <hare@suse.de>

-- 
2.29.2

[PATCH V3 1/3] scsi: core: use put_device() to release host

[Ming Lei]

After device is initialized via device_initialize(), or its name is
set via dev_set_name(), the device has to be freed via put_device(),
otherwise device name will be leaked because it is allocated
dynamically in dev_set_name().

Fixes the issue by replacing kfree(shost) via two put_device() since
both .shost_dev and .shost_gendev share same lifetime. Meantime move
get_device(shost->shost_gendev) from scsi_add_host_with_dma to
scsi_host_alloc(), so that we can grab parent's refcnt explicitly when
assigning .shost_dev->parent. With this way code becomes more readable.

Also call put_device(dev->parent) in scsi_host_cls_release() so that
code readability can be improved.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: John Garry <john.garry@huawei.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/hosts.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 624e2582c3df..ada11e3ae577 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -55,7 +55,7 @@ static DEFINE_IDA(host_index_ida);
 
 static void scsi_host_cls_release(struct device *dev)
 {
-	put_device(&class_to_shost(dev)->shost_gendev);
+	put_device(dev->parent);
 }
 
 static struct class shost_class = {
@@ -261,8 +261,6 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	if (error)
 		goto out_del_gendev;
 
-	get_device(&shost->shost_gendev);
-
 	if (shost->transportt->host_size) {
 		shost->shost_data = kzalloc(shost->transportt->host_size,
 					 GFP_KERNEL);
@@ -473,7 +471,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->shost_gendev.type = &scsi_host_type;
 
 	device_initialize(&shost->shost_dev);
-	shost->shost_dev.parent = &shost->shost_gendev;
+	shost->shost_dev.parent = get_device(&shost->shost_gendev);
 	shost->shost_dev.class = &shost_class;
 	dev_set_name(&shost->shost_dev, "host%d", shost->host_no);
 	shost->shost_dev.groups = scsi_sysfs_shost_attr_groups;
@@ -503,7 +501,9 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
  fail_index_remove:
 	ida_simple_remove(&host_index_ida, shost->host_no);
  fail_kfree:
-	kfree(shost);
+	put_device(&shost->shost_dev);
+	put_device(&shost->shost_gendev);
+
 	return NULL;
 }
 EXPORT_SYMBOL(scsi_host_alloc);
-- 
2.29.2

[PATCH V3 2/3] scsi: core: fix failure handling of scsi_add_host_with_dma

[Ming Lei]

When scsi_add_host_with_dma() return failure, the caller will call
scsi_host_put(shost) to release everything allocated for this host
instance. So we can't free allocated stuff in scsi_add_host_with_dma(),
otherwise double free will be caused.

Strictly speaking, these host resources allocation should have been
moved to scsi_host_alloc(), but the allocation may need driver's
info which can be built between calling scsi_host_alloc() and
scsi_add_host(), so just keep the allocations in
scsi_add_host_with_dma().

Fixes the problem by relying on host device's release handler to
release everything.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: John Garry <john.garry@huawei.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/hosts.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index ada11e3ae577..6cbc3eb16525 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -279,23 +279,22 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 
 		if (!shost->work_q) {
 			error = -EINVAL;
-			goto out_free_shost_data;
+			goto out_del_dev;
 		}
 	}
 
 	error = scsi_sysfs_add_host(shost);
 	if (error)
-		goto out_destroy_host;
+		goto out_del_dev;
 
 	scsi_proc_host_add(shost);
 	scsi_autopm_put_host(shost);
 	return error;
 
- out_destroy_host:
-	if (shost->work_q)
-		destroy_workqueue(shost->work_q);
- out_free_shost_data:
-	kfree(shost->shost_data);
+	/*
+	 * any host allocation in this function will be freed in
+	 * scsi_host_dev_release, so don't free them in the failure path
+	 */
  out_del_dev:
 	device_del(&shost->shost_dev);
  out_del_gendev:
@@ -305,7 +304,6 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	pm_runtime_disable(&shost->shost_gendev);
 	pm_runtime_set_suspended(&shost->shost_gendev);
 	pm_runtime_put_noidle(&shost->shost_gendev);
-	scsi_mq_destroy_tags(shost);
  fail:
 	return error;
 }
-- 
2.29.2

[PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path

[Ming Lei]

get_device(shost->shost_gendev.parent) is called in
scsi_add_host_with_dma(), but its counter pair isn't called in the failure
path, so fix it by calling put_device(shost->shost_gendev.parent) in its
failure path.

Reported-by: John Garry <john.garry@huawei.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/hosts.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 6cbc3eb16525..6cc43c51b7b3 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
  out_del_dev:
 	device_del(&shost->shost_dev);
  out_del_gendev:
+	put_device(shost->shost_gendev.parent);
 	device_del(&shost->shost_gendev);
  out_disable_runtime_pm:
 	device_disable_async_suspend(&shost->shost_gendev);
-- 
2.29.2

[PATCH V4 1/3] scsi: core: use put_device() to release host

[Ming Lei]

From 2a70cffeca09156faf68d933157586b5bf9278fa Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@redhat.com>
Date: Mon, 31 May 2021 11:02:01 +0800
Subject: [PATCH V4 1/3] scsi: core: use put_device() to release host

After device is initialized via device_initialize(), or its name is
set via dev_set_name(), the device has to be freed via put_device(),
otherwise device name will be leaked because it is allocated
dynamically in dev_set_name().

Fixes the issue by replacing kfree(shost) via two put_device() since
both .shost_dev and .shost_gendev share same lifetime. Meantime move
get_device(shost->shost_gendev) from scsi_add_host_with_dma to
scsi_host_alloc(), so that we can grab parent's refcnt explicitly when
assigning .shost_dev->parent. With this way code becomes more readable.

Also call put_device(dev->parent) in scsi_host_cls_release() so that
code readability can be improved.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: John Garry <john.garry@huawei.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
V4:
	- avoid to touch un-initialized device instance

 drivers/scsi/hosts.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 624e2582c3df..181a5d6e3c7b 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -55,7 +55,7 @@ static DEFINE_IDA(host_index_ida);
 
 static void scsi_host_cls_release(struct device *dev)
 {
-	put_device(&class_to_shost(dev)->shost_gendev);
+	put_device(dev->parent);
 }
 
 static struct class shost_class = {
@@ -261,8 +261,6 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	if (error)
 		goto out_del_gendev;
 
-	get_device(&shost->shost_gendev);
-
 	if (shost->transportt->host_size) {
 		shost->shost_data = kzalloc(shost->transportt->host_size,
 					 GFP_KERNEL);
@@ -391,8 +389,10 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	mutex_init(&shost->scan_mutex);
 
 	index = ida_simple_get(&host_index_ida, 0, 0, GFP_KERNEL);
-	if (index < 0)
-		goto fail_kfree;
+	if (index < 0) {
+		kfree(shost);
+		return NULL;
+	}
 	shost->host_no = index;
 
 	shost->dma_channel = 0xff;
@@ -473,7 +473,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->shost_gendev.type = &scsi_host_type;
 
 	device_initialize(&shost->shost_dev);
-	shost->shost_dev.parent = &shost->shost_gendev;
+	shost->shost_dev.parent = get_device(&shost->shost_gendev);
 	shost->shost_dev.class = &shost_class;
 	dev_set_name(&shost->shost_dev, "host%d", shost->host_no);
 	shost->shost_dev.groups = scsi_sysfs_shost_attr_groups;
@@ -502,8 +502,9 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	kthread_stop(shost->ehandler);
  fail_index_remove:
 	ida_simple_remove(&host_index_ida, shost->host_no);
- fail_kfree:
-	kfree(shost);
+	/* free host instance */
+	put_device(&shost->shost_dev);
+	put_device(&shost->shost_gendev);
 	return NULL;
 }
 EXPORT_SYMBOL(scsi_host_alloc);
-- 
2.29.2

Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path

[Hannes Reinecke]

On 5/31/21 7:07 AM, Ming Lei wrote:
> get_device(shost->shost_gendev.parent) is called in
> scsi_add_host_with_dma(), but its counter pair isn't called in the failure
> path, so fix it by calling put_device(shost->shost_gendev.parent) in its
> failure path.
> 
> Reported-by: John Garry <john.garry@huawei.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/scsi/hosts.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 6cbc3eb16525..6cc43c51b7b3 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>    out_del_dev:
>   	device_del(&shost->shost_dev);
>    out_del_gendev:
> +	put_device(shost->shost_gendev.parent);
>   	device_del(&shost->shost_gendev);
>    out_disable_runtime_pm:
>   	device_disable_async_suspend(&shost->shost_gendev);
> 
This really needs to be folded into the first patch as it's really a 
bugfix for that.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V3 2/3] scsi: core: fix failure handling of scsi_add_host_with_dma

[Hannes Reinecke]

On 5/31/21 7:07 AM, Ming Lei wrote:
> When scsi_add_host_with_dma() return failure, the caller will call
> scsi_host_put(shost) to release everything allocated for this host
> instance. So we can't free allocated stuff in scsi_add_host_with_dma(),
> otherwise double free will be caused.
> 
> Strictly speaking, these host resources allocation should have been
> moved to scsi_host_alloc(), but the allocation may need driver's
> info which can be built between calling scsi_host_alloc() and
> scsi_add_host(), so just keep the allocations in
> scsi_add_host_with_dma().
> 
> Fixes the problem by relying on host device's release handler to
> release everything.
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>   drivers/scsi/hosts.c | 14 ++++++--------
>   1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index ada11e3ae577..6cbc3eb16525 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -279,23 +279,22 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   
>   		if (!shost->work_q) {
>   			error = -EINVAL;
> -			goto out_free_shost_data;
> +			goto out_del_dev;
>   		}
>   	}
>   
>   	error = scsi_sysfs_add_host(shost);
>   	if (error)
> -		goto out_destroy_host;
> +		goto out_del_dev;
>   
>   	scsi_proc_host_add(shost);
>   	scsi_autopm_put_host(shost);
>   	return error;
>   
> - out_destroy_host:
> -	if (shost->work_q)
> -		destroy_workqueue(shost->work_q);
> - out_free_shost_data:
> -	kfree(shost->shost_data);
> +	/*
> +	 * any host allocation in this function will be freed in
> +	 * scsi_host_dev_release, so don't free them in the failure path
> +	 */
>    out_del_dev:
>   	device_del(&shost->shost_dev);
>    out_del_gendev:
> @@ -305,7 +304,6 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   	pm_runtime_disable(&shost->shost_gendev);
>   	pm_runtime_set_suspended(&shost->shost_gendev);
>   	pm_runtime_put_noidle(&shost->shost_gendev);
> -	scsi_mq_destroy_tags(shost);
>    fail:
>   	return error;
>   }
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path

[Ming Lei]

On Mon, May 31, 2021 at 08:28:49AM +0200, Hannes Reinecke wrote:
> On 5/31/21 7:07 AM, Ming Lei wrote:
> > get_device(shost->shost_gendev.parent) is called in
> > scsi_add_host_with_dma(), but its counter pair isn't called in the failure
> > path, so fix it by calling put_device(shost->shost_gendev.parent) in its
> > failure path.
> > 
> > Reported-by: John Garry <john.garry@huawei.com>
> > Cc: Bart Van Assche <bvanassche@acm.org>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >   drivers/scsi/hosts.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > index 6cbc3eb16525..6cc43c51b7b3 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
> >    out_del_dev:
> >   	device_del(&shost->shost_dev);
> >    out_del_gendev:
> > +	put_device(shost->shost_gendev.parent);
> >   	device_del(&shost->shost_gendev);
> >    out_disable_runtime_pm:
> >   	device_disable_async_suspend(&shost->shost_gendev);
> > 
> This really needs to be folded into the first patch as it's really a bugfix
> for that.

All three are bug fixes, and this one may leak .shost_gendev's parent,
but the 1st one leaks .shost_gendev->kobj.name, so we needn't to fold
the 3rd into the 1st one.


Thanks,
Ming

Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path

[Hannes Reinecke]

On 5/31/21 9:56 AM, Ming Lei wrote:
> On Mon, May 31, 2021 at 08:28:49AM +0200, Hannes Reinecke wrote:
>> On 5/31/21 7:07 AM, Ming Lei wrote:
>>> get_device(shost->shost_gendev.parent) is called in
>>> scsi_add_host_with_dma(), but its counter pair isn't called in the failure
>>> path, so fix it by calling put_device(shost->shost_gendev.parent) in its
>>> failure path.
>>>
>>> Reported-by: John Garry <john.garry@huawei.com>
>>> Cc: Bart Van Assche <bvanassche@acm.org>
>>> Cc: Hannes Reinecke <hare@suse.de>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> ---
>>>    drivers/scsi/hosts.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>>> index 6cbc3eb16525..6cc43c51b7b3 100644
>>> --- a/drivers/scsi/hosts.c
>>> +++ b/drivers/scsi/hosts.c
>>> @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>>>     out_del_dev:
>>>    	device_del(&shost->shost_dev);
>>>     out_del_gendev:
>>> +	put_device(shost->shost_gendev.parent);
>>>    	device_del(&shost->shost_gendev);
>>>     out_disable_runtime_pm:
>>>    	device_disable_async_suspend(&shost->shost_gendev);
>>>
>> This really needs to be folded into the first patch as it's really a bugfix
>> for that.
> 
> All three are bug fixes, and this one may leak .shost_gendev's parent,
> but the 1st one leaks .shost_gendev->kobj.name, so we needn't to fold
> the 3rd into the 1st one.
> 
I beg to disagree.
The first patch removes the 'get_device()' from
scsi_add_host_with_dma(), but does not remove the corresponding 
'put_device()' in the error path.
So the first patch introduces a reference imbalance which is fixed by 
the third patch. Hence my suggestion to merge those two patches.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path

[Ming Lei]

On Mon, May 31, 2021 at 10:23:43AM +0200, Hannes Reinecke wrote:
> On 5/31/21 9:56 AM, Ming Lei wrote:
> > On Mon, May 31, 2021 at 08:28:49AM +0200, Hannes Reinecke wrote:
> > > On 5/31/21 7:07 AM, Ming Lei wrote:
> > > > get_device(shost->shost_gendev.parent) is called in
> > > > scsi_add_host_with_dma(), but its counter pair isn't called in the failure
> > > > path, so fix it by calling put_device(shost->shost_gendev.parent) in its
> > > > failure path.
> > > > 
> > > > Reported-by: John Garry <john.garry@huawei.com>
> > > > Cc: Bart Van Assche <bvanassche@acm.org>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >    drivers/scsi/hosts.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > > > index 6cbc3eb16525..6cc43c51b7b3 100644
> > > > --- a/drivers/scsi/hosts.c
> > > > +++ b/drivers/scsi/hosts.c
> > > > @@ -298,6 +298,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
> > > >     out_del_dev:
> > > >    	device_del(&shost->shost_dev);
> > > >     out_del_gendev:
> > > > +	put_device(shost->shost_gendev.parent);
> > > >    	device_del(&shost->shost_gendev);
> > > >     out_disable_runtime_pm:
> > > >    	device_disable_async_suspend(&shost->shost_gendev);
> > > > 
> > > This really needs to be folded into the first patch as it's really a bugfix
> > > for that.
> > 
> > All three are bug fixes, and this one may leak .shost_gendev's parent,
> > but the 1st one leaks .shost_gendev->kobj.name, so we needn't to fold
> > the 3rd into the 1st one.
> > 
> I beg to disagree.
> The first patch removes the 'get_device()' from
> scsi_add_host_with_dma(), but does not remove the corresponding
> 'put_device()' in the error path.

There isn't such 'put_device' in the error path of scsi_add_host_with_dma(),
is there?

> So the first patch introduces a reference imbalance which is fixed by the
> third patch. Hence my suggestion to merge those two patches.

No, that isn't true, please look at current code of
scsi_add_host_with_dma().


Thanks, 
Ming

Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma

[John Garry]

On 31/05/2021 06:07, Ming Lei wrote:
> Hello Martin,
> 
> 
> Fix two memory leaks and one double free issue in alloc/add host
> code path.
> 
> 
> V3:
> 	- fix memory leak caused in scsi_host_alloc
> 	- comment typo suggested by John
> 
> V2:
> 	- add patch 2 for addressing shost leak in case of adding host
> 	  failure, reported by John Garry.
> 
> Ming Lei (3):
>    scsi: core: use put_device() to release host
>    scsi: core: fix failure handling of scsi_add_host_with_dma
>    scsi: core: put ->shost_gendev.parent in failure handling path
> 
>   drivers/scsi/hosts.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
> 
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Hannes Reinecke <hare@suse.de>


I tested this again with the same experiment as before to error in
scsi_add_host_with_dma(), and we don't call scsi_host_dev_release() now. 
The shost_gendev dev refcount is 2 upon exit in scsi_add_host_with_dma().

We don't call scsi_host_cls_release() either, so I guess a ref count is 
leaked for shost_dev - I see its refcount is 1 at exit in 
scsi_add_host_with_dma(). We have the device_initialize(), device_add(), 
device_del() in the alloc and add host functions, but I don't know who 
is responsible for the final "device put".

Thanks,
John

Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma

[Ming Lei]

On Tue, Jun 01, 2021 at 11:34:20AM +0100, John Garry wrote:
> On 31/05/2021 06:07, Ming Lei wrote:
> > Hello Martin,
> > 
> > 
> > Fix two memory leaks and one double free issue in alloc/add host
> > code path.
> > 
> > 
> > V3:
> > 	- fix memory leak caused in scsi_host_alloc
> > 	- comment typo suggested by John
> > 
> > V2:
> > 	- add patch 2 for addressing shost leak in case of adding host
> > 	  failure, reported by John Garry.
> > 
> > Ming Lei (3):
> >    scsi: core: use put_device() to release host
> >    scsi: core: fix failure handling of scsi_add_host_with_dma
> >    scsi: core: put ->shost_gendev.parent in failure handling path
> > 
> >   drivers/scsi/hosts.c | 25 ++++++++++++-------------
> >   1 file changed, 12 insertions(+), 13 deletions(-)
> > 
> > Cc: Bart Van Assche <bvanassche@acm.org>
> > Cc: John Garry <john.garry@huawei.com>
> > Cc: Hannes Reinecke <hare@suse.de>
> 
> 
> I tested this again with the same experiment as before to error in
> scsi_add_host_with_dma(), and we don't call scsi_host_dev_release() now. The
> shost_gendev dev refcount is 2 upon exit in scsi_add_host_with_dma().
> 
> We don't call scsi_host_cls_release() either, so I guess a ref count is
> leaked for shost_dev - I see its refcount is 1 at exit in
> scsi_add_host_with_dma(). We have the device_initialize(), device_add(),
> device_del() in the alloc and add host functions, but I don't know who is
> responsible for the final "device put".

Hammm, we still need to put ->shost_dev before returning the error, and the
following delta patch can fix the issue, and it should have been wrapped
into the 1st one.

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 22a58e453a0c..532165462a42 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -306,6 +306,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	pm_runtime_set_suspended(&shost->shost_gendev);
 	pm_runtime_put_noidle(&shost->shost_gendev);
  fail:
+	/* drop ref of ->shost_dev so that caller can release this host */
+	put_device(&shost->shost_dev);
 	return error;
 }
 EXPORT_SYMBOL(scsi_add_host_with_dma);


Thanks,
Ming

Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma

[John Garry]

On 01/06/2021 14:11, Ming Lei wrote:
>> We don't call scsi_host_cls_release() either, so I guess a ref count is
>> leaked for shost_dev - I see its refcount is 1 at exit in
>> scsi_add_host_with_dma(). We have the device_initialize(), device_add(),
>> device_del() in the alloc and add host functions, but I don't know who is
>> responsible for the final "device put".
> Hammm, we still need to put ->shost_dev before returning the error, and the
> following delta patch can fix the issue, and it should have been wrapped
> into the 1st one.
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 22a58e453a0c..532165462a42 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -306,6 +306,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   	pm_runtime_set_suspended(&shost->shost_gendev);
>   	pm_runtime_put_noidle(&shost->shost_gendev);
>    fail:
> +	/* drop ref of ->shost_dev so that caller can release this host */
> +	put_device(&shost->shost_dev);
>   	return error;
>   }
>   EXPORT_SYMBOL(scsi_add_host_with_dma);

That looks better now.

And we can see the equivalent on the normal removal path in 
scsi_remove_host() -> device_unregister(&shost->shost_dev), which does a 
device_del()+put_device().

So could we actually just have:
out_del_dev:
	unregister_dev(&shost->shost_dev)

I am not sure if we are required to keep that shost_dev reference all 
the way until the exit, as you do.

Thanks,
John

Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma

[Ming Lei]

On Tue, Jun 01, 2021 at 04:07:05PM +0100, John Garry wrote:
> On 01/06/2021 14:11, Ming Lei wrote:
> > > We don't call scsi_host_cls_release() either, so I guess a ref count is
> > > leaked for shost_dev - I see its refcount is 1 at exit in
> > > scsi_add_host_with_dma(). We have the device_initialize(), device_add(),
> > > device_del() in the alloc and add host functions, but I don't know who is
> > > responsible for the final "device put".
> > Hammm, we still need to put ->shost_dev before returning the error, and the
> > following delta patch can fix the issue, and it should have been wrapped
> > into the 1st one.
> > 
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > index 22a58e453a0c..532165462a42 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -306,6 +306,8 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
> >   	pm_runtime_set_suspended(&shost->shost_gendev);
> >   	pm_runtime_put_noidle(&shost->shost_gendev);
> >    fail:
> > +	/* drop ref of ->shost_dev so that caller can release this host */
> > +	put_device(&shost->shost_dev);
> >   	return error;
> >   }
> >   EXPORT_SYMBOL(scsi_add_host_with_dma);
> 
> That looks better now.
> 
> And we can see the equivalent on the normal removal path in
> scsi_remove_host() -> device_unregister(&shost->shost_dev), which does a
> device_del()+put_device().
> 
> So could we actually just have:
> out_del_dev:
> 	unregister_dev(&shost->shost_dev)
> 

No, we still have to call put_device(&shost->shost_dev) only in case of
failure before adding &shost->shost_dev.

> I am not sure if we are required to keep that shost_dev reference all the
> way until the exit, as you do.

That has been done in this way, the problem is that both .shost_dev and
.shost_gendev share same lifetime and memory(same struct Scsi_Host instance),
this kind of pattern isn't one usual driver core use case, and we have to
handle it carefully.


Thanks,
Ming