* [PATCH V3 1/3] scsi: core: use put_device() to release host
2021-05-31 5:07 [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma Ming Lei
@ 2021-05-31 5:07 ` Ming Lei
2021-05-31 5:21 ` [PATCH V4 " Ming Lei
2021-05-31 5:07 ` [PATCH V3 2/3] scsi: core: fix failure handling of scsi_add_host_with_dma Ming Lei
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Ming Lei @ 2021-05-31 5:07 UTC (permalink / raw)
To: Martin K . Petersen, linux-scsi
Cc: Ming Lei, Bart Van Assche, John Garry, Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH V4 1/3] scsi: core: use put_device() to release host
2021-05-31 5:07 ` [PATCH V3 1/3] scsi: core: use put_device() to release host Ming Lei
@ 2021-05-31 5:21 ` Ming Lei
0 siblings, 0 replies; 14+ messages in thread
From: Ming Lei @ 2021-05-31 5:21 UTC (permalink / raw)
To: Martin K . Petersen, linux-scsi
Cc: Bart Van Assche, John Garry, Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH V3 2/3] scsi: core: fix failure handling of scsi_add_host_with_dma
2021-05-31 5:07 [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma Ming Lei
2021-05-31 5:07 ` [PATCH V3 1/3] scsi: core: use put_device() to release host Ming Lei
@ 2021-05-31 5:07 ` Ming Lei
2021-05-31 6:31 ` Hannes Reinecke
2021-05-31 5:07 ` [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path Ming Lei
2021-06-01 10:34 ` [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma John Garry
3 siblings, 1 reply; 14+ messages in thread
From: Ming Lei @ 2021-05-31 5:07 UTC (permalink / raw)
To: Martin K . Petersen, linux-scsi
Cc: Ming Lei, Bart Van Assche, John Garry, Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH V3 2/3] scsi: core: fix failure handling of scsi_add_host_with_dma
2021-05-31 5:07 ` [PATCH V3 2/3] scsi: core: fix failure handling of scsi_add_host_with_dma Ming Lei
@ 2021-05-31 6:31 ` Hannes Reinecke
0 siblings, 0 replies; 14+ messages in thread
From: Hannes Reinecke @ 2021-05-31 6:31 UTC (permalink / raw)
To: Ming Lei, Martin K . Petersen, linux-scsi; +Cc: Bart Van Assche, John Garry
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path
2021-05-31 5:07 [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma Ming Lei
2021-05-31 5:07 ` [PATCH V3 1/3] scsi: core: use put_device() to release host Ming Lei
2021-05-31 5:07 ` [PATCH V3 2/3] scsi: core: fix failure handling of scsi_add_host_with_dma Ming Lei
@ 2021-05-31 5:07 ` Ming Lei
2021-05-31 6:28 ` Hannes Reinecke
2021-06-01 10:34 ` [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma John Garry
3 siblings, 1 reply; 14+ messages in thread
From: Ming Lei @ 2021-05-31 5:07 UTC (permalink / raw)
To: Martin K . Petersen, linux-scsi
Cc: Ming Lei, John Garry, Bart Van Assche, Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path
2021-05-31 5:07 ` [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path Ming Lei
@ 2021-05-31 6:28 ` Hannes Reinecke
2021-05-31 7:56 ` Ming Lei
0 siblings, 1 reply; 14+ messages in thread
From: Hannes Reinecke @ 2021-05-31 6:28 UTC (permalink / raw)
To: Ming Lei, Martin K . Petersen, linux-scsi; +Cc: John Garry, Bart Van Assche
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path
2021-05-31 6:28 ` Hannes Reinecke
@ 2021-05-31 7:56 ` Ming Lei
2021-05-31 8:23 ` Hannes Reinecke
0 siblings, 1 reply; 14+ messages in thread
From: Ming Lei @ 2021-05-31 7:56 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K . Petersen, linux-scsi, John Garry, Bart Van Assche
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path
2021-05-31 7:56 ` Ming Lei
@ 2021-05-31 8:23 ` Hannes Reinecke
2021-05-31 9:17 ` Ming Lei
0 siblings, 1 reply; 14+ messages in thread
From: Hannes Reinecke @ 2021-05-31 8:23 UTC (permalink / raw)
To: Ming Lei; +Cc: Martin K . Petersen, linux-scsi, John Garry, Bart Van Assche
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path
2021-05-31 8:23 ` Hannes Reinecke
@ 2021-05-31 9:17 ` Ming Lei
0 siblings, 0 replies; 14+ messages in thread
From: Ming Lei @ 2021-05-31 9:17 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Martin K . Petersen, linux-scsi, John Garry, Bart Van Assche
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma
2021-05-31 5:07 [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma Ming Lei
` (2 preceding siblings ...)
2021-05-31 5:07 ` [PATCH V3 3/3] scsi: core: put ->shost_gendev.parent in failure handling path Ming Lei
@ 2021-06-01 10:34 ` John Garry
2021-06-01 13:11 ` Ming Lei
3 siblings, 1 reply; 14+ messages in thread
From: John Garry @ 2021-06-01 10:34 UTC (permalink / raw)
To: Ming Lei, Martin K . Petersen, linux-scsi
Cc: Bart Van Assche, Hannes Reinecke
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma
2021-06-01 10:34 ` [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma John Garry
@ 2021-06-01 13:11 ` Ming Lei
2021-06-01 15:07 ` John Garry
0 siblings, 1 reply; 14+ messages in thread
From: Ming Lei @ 2021-06-01 13:11 UTC (permalink / raw)
To: John Garry
Cc: Martin K . Petersen, linux-scsi, Bart Van Assche, Hannes Reinecke
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma
2021-06-01 13:11 ` Ming Lei
@ 2021-06-01 15:07 ` John Garry
2021-06-02 1:55 ` Ming Lei
0 siblings, 1 reply; 14+ messages in thread
From: John Garry @ 2021-06-01 15:07 UTC (permalink / raw)
To: Ming Lei
Cc: Martin K . Petersen, linux-scsi, Bart Van Assche, Hannes Reinecke
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V3 0/3] scsi: two fixes in scsi_add_host_with_dma
2021-06-01 15:07 ` John Garry
@ 2021-06-02 1:55 ` Ming Lei
0 siblings, 0 replies; 14+ messages in thread
From: Ming Lei @ 2021-06-02 1:55 UTC (permalink / raw)
To: John Garry
Cc: Martin K . Petersen, linux-scsi, Bart Van Assche, Hannes Reinecke
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
^ permalink raw reply [flat|nested] 14+ messages in thread