* [PATCH V2 0/2] scsi: two fixes in scsi_add_host_with_dma @ 2021-05-28 1:18 Ming Lei 2021-05-28 1:18 ` [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma Ming Lei 2021-05-28 1:18 ` [PATCH V2 2/2] scsi: core: put shost->shost_gendev in failure handling path Ming Lei 0 siblings, 2 replies; 5+ messages in thread From: Ming Lei @ 2021-05-28 1:18 UTC (permalink / raw) To: Martin K . Petersen, linux-scsi Cc: Ming Lei, Bart Van Assche, John Garry, Hannes Reinecke Hello Martin, The 1st patch fixes one double free in failure path of adding host. The 2nd patch fixes one host device leak in failure path of adding host. V2: - add patch 2 for addressing shost leak in case of adding host failure, reported by John Garry. Ming Lei (2): scsi: core: fix failure handling of scsi_add_host_with_dma scsi: core: put shost->shost_gendev in failure handling path drivers/scsi/hosts.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) Cc: Bart Van Assche <bvanassche@acm.org> Cc: John Garry <john.garry@huawei.com> Cc: Hannes Reinecke <hare@suse.de> -- 2.29.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma 2021-05-28 1:18 [PATCH V2 0/2] scsi: two fixes in scsi_add_host_with_dma Ming Lei @ 2021-05-28 1:18 ` Ming Lei 2021-05-28 8:34 ` John Garry 2021-05-28 1:18 ` [PATCH V2 2/2] scsi: core: put shost->shost_gendev in failure handling path Ming Lei 1 sibling, 1 reply; 5+ messages in thread From: Ming Lei @ 2021-05-28 1:18 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 697c09ef259b..ea50856cb203 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -278,23 +278,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 not free them in the failure path + */ out_del_dev: device_del(&shost->shost_dev); out_del_gendev: @@ -304,7 +303,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] 5+ messages in thread
* Re: [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma 2021-05-28 1:18 ` [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma Ming Lei @ 2021-05-28 8:34 ` John Garry 2021-05-31 1:22 ` Ming Lei 0 siblings, 1 reply; 5+ messages in thread From: John Garry @ 2021-05-28 8:34 UTC (permalink / raw) To: Ming Lei, Martin K . Petersen, linux-scsi Cc: Bart Van Assche, Hannes Reinecke On 28/05/2021 02:18, 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> It now looks like we have a memory leak: unreferenced object 0xffff0410070a4e00 (size 128): comm "swapper/0", pid 1, jiffies 4294894100 (age 90.744s) hex dump (first 32 bytes): 68 6f 73 74 30 00 00 00 00 00 00 00 00 00 00 00 host0........... 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<(____ptrval____)>] __kmalloc_track_caller+0x25c/0x380 [<(____ptrval____)>] kvasprintf+0xe8/0x1a4 [<(____ptrval____)>] kvasprintf_const+0xc8/0x17c [<(____ptrval____)>] kobject_set_name_vargs+0x58/0xf4 [<(____ptrval____)>] dev_set_name+0xa4/0xe0 [<(____ptrval____)>] scsi_host_alloc+0x45c/0x5d0 [<(____ptrval____)>] hisi_sas_probe+0x40/0x570 [<(____ptrval____)>] hisi_sas_v2_probe+0x1c/0x2c [<(____ptrval____)>] platform_probe+0x90/0x110 [<(____ptrval____)>] really_probe+0x148/0x744 [<(____ptrval____)>] driver_probe_device+0x8c/0xfc [<(____ptrval____)>] device_driver_attach+0x11c/0x12c [<(____ptrval____)>] __driver_attach+0xc8/0x1a0 [<(____ptrval____)>] bus_for_each_dev+0xe4/0x154 [<(____ptrval____)>] driver_attach+0x38/0x50 [<(____ptrval____)>] bus_add_driver+0x1bc/0x2c4 unreferenced object 0xffff001056581800 (size 256): comm "swapper/0", pid 1, jiffies 4294894398 (age 89.560s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 08 18 58 56 10 00 ff ff ..........XV.... 08 18 58 56 10 00 ff ff c0 f4 b4 10 00 80 ff ff ..XV............ backtrace: [<(____ptrval____)>] kmem_cache_alloc+0x298/0x350 [<(____ptrval____)>] device_add+0x6d8/0xc4c [<(____ptrval____)>] scsi_add_host_with_dma+0x370/0x5dc [<(____ptrval____)>] hisi_sas_probe+0x418/0x570 [<(____ptrval____)>] hisi_sas_v2_probe+0x1c/0x2c [<(____ptrval____)>] platform_probe+0x90/0x110 [<(____ptrval____)>] really_probe+0x148/0x744 [<(____ptrval____)>] driver_probe_device+0x8c/0xfc [<(____ptrval____)>] device_driver_attach+0x11c/0x12c [<(____ptrval[ 101.941505] random: fast init done ____)>] __driver_attach+0xc8/0x1a0 [<(____ptrval____)>] bus_for_each_dev+0xe4/0x154 [<(____ptrval____)>] driver_attach+0x38/0x50 [<(____ptrval____)>] bus_add_driver+0x1bc/0x2c4 [<(____ptrval____)>] driver_register+0xe4/0x210 [<(____ptrval____)>] __platform_driver_register+0x48/0x60 [<(____ptrval____)>] hisi_sas_v2_driver_init+0x20/0x2c I think that the release for the shost_dev dev name memory needs fixing. In scsi_host_dev_release(), for my experiment, shost state is running, so we miss the kfree(dev_name(&shost->shost_dev)), I guess. Not sure on the proper fix. > --- > 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 697c09ef259b..ea50856cb203 100644 > --- a/drivers/scsi/hosts.c > +++ b/drivers/scsi/hosts.c > @@ -278,23 +278,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 not free them in the failure path nit: "so do not free them" > + */ > out_del_dev: > device_del(&shost->shost_dev); > out_del_gendev: > @@ -304,7 +303,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; > } > Thanks, John ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma 2021-05-28 8:34 ` John Garry @ 2021-05-31 1:22 ` Ming Lei 0 siblings, 0 replies; 5+ messages in thread From: Ming Lei @ 2021-05-31 1:22 UTC (permalink / raw) To: John Garry Cc: Martin K . Petersen, linux-scsi, Bart Van Assche, Hannes Reinecke On Fri, May 28, 2021 at 09:34:44AM +0100, John Garry wrote: > On 28/05/2021 02:18, 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> > > It now looks like we have a memory leak: > > unreferenced object 0xffff0410070a4e00 (size 128): > comm "swapper/0", pid 1, jiffies 4294894100 (age 90.744s) > hex dump (first 32 bytes): > 68 6f 73 74 30 00 00 00 00 00 00 00 00 00 00 00 host0........... > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > backtrace: > [<(____ptrval____)>] __kmalloc_track_caller+0x25c/0x380 > [<(____ptrval____)>] kvasprintf+0xe8/0x1a4 > [<(____ptrval____)>] kvasprintf_const+0xc8/0x17c > [<(____ptrval____)>] kobject_set_name_vargs+0x58/0xf4 > [<(____ptrval____)>] dev_set_name+0xa4/0xe0 > [<(____ptrval____)>] scsi_host_alloc+0x45c/0x5d0 > [<(____ptrval____)>] hisi_sas_probe+0x40/0x570 > [<(____ptrval____)>] hisi_sas_v2_probe+0x1c/0x2c > [<(____ptrval____)>] platform_probe+0x90/0x110 > [<(____ptrval____)>] really_probe+0x148/0x744 > [<(____ptrval____)>] driver_probe_device+0x8c/0xfc > [<(____ptrval____)>] device_driver_attach+0x11c/0x12c > [<(____ptrval____)>] __driver_attach+0xc8/0x1a0 > [<(____ptrval____)>] bus_for_each_dev+0xe4/0x154 > [<(____ptrval____)>] driver_attach+0x38/0x50 > [<(____ptrval____)>] bus_add_driver+0x1bc/0x2c4 > unreferenced object 0xffff001056581800 (size 256): > comm "swapper/0", pid 1, jiffies 4294894398 (age 89.560s) > hex dump (first 32 bytes): > 00 00 00 00 00 00 00 00 08 18 58 56 10 00 ff ff ..........XV.... > 08 18 58 56 10 00 ff ff c0 f4 b4 10 00 80 ff ff ..XV............ > backtrace: > [<(____ptrval____)>] kmem_cache_alloc+0x298/0x350 > [<(____ptrval____)>] device_add+0x6d8/0xc4c > [<(____ptrval____)>] scsi_add_host_with_dma+0x370/0x5dc > [<(____ptrval____)>] hisi_sas_probe+0x418/0x570 > [<(____ptrval____)>] hisi_sas_v2_probe+0x1c/0x2c > [<(____ptrval____)>] platform_probe+0x90/0x110 > [<(____ptrval____)>] really_probe+0x148/0x744 > [<(____ptrval____)>] driver_probe_device+0x8c/0xfc > [<(____ptrval____)>] device_driver_attach+0x11c/0x12c > [<(____ptrval[ 101.941505] random: fast init done > ____)>] __driver_attach+0xc8/0x1a0 > [<(____ptrval____)>] bus_for_each_dev+0xe4/0x154 > [<(____ptrval____)>] driver_attach+0x38/0x50 > [<(____ptrval____)>] bus_add_driver+0x1bc/0x2c4 > [<(____ptrval____)>] driver_register+0xe4/0x210 > [<(____ptrval____)>] __platform_driver_register+0x48/0x60 > [<(____ptrval____)>] hisi_sas_v2_driver_init+0x20/0x2c > > I think that the release for the shost_dev dev name memory needs fixing. > > In scsi_host_dev_release(), for my experiment, shost state is running, so we > miss the kfree(dev_name(&shost->shost_dev)), I guess. Not sure on the proper > fix. kobject_cleanup() is responsible for freeing dev->kobj.name, so nothing to do with freeing the device name. The following delta patch may fix the issue, and we should have wrap it into the 2nd patch, can you test it and see if the kmemleak warning is fixed? diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index 492b64f349e1..7f7e0b3f6a3c 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, put_device(&shost->shost_gendev); 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); Thanks, Ming ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH V2 2/2] scsi: core: put shost->shost_gendev in failure handling path 2021-05-28 1:18 [PATCH V2 0/2] scsi: two fixes in scsi_add_host_with_dma Ming Lei 2021-05-28 1:18 ` [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma Ming Lei @ 2021-05-28 1:18 ` Ming Lei 1 sibling, 0 replies; 5+ messages in thread From: Ming Lei @ 2021-05-28 1:18 UTC (permalink / raw) To: Martin K . Petersen, linux-scsi Cc: Ming Lei, John Garry, Bart Van Assche, Hannes Reinecke get_device(&shost->shost_gendev) 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) 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 ea50856cb203..492b64f349e1 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -295,6 +295,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, * scsi_host_dev_release, so not free them in the failure path */ out_del_dev: + put_device(&shost->shost_gendev); device_del(&shost->shost_dev); out_del_gendev: device_del(&shost->shost_gendev); -- 2.29.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-31 1:22 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-28 1:18 [PATCH V2 0/2] scsi: two fixes in scsi_add_host_with_dma Ming Lei 2021-05-28 1:18 ` [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma Ming Lei 2021-05-28 8:34 ` John Garry 2021-05-31 1:22 ` Ming Lei 2021-05-28 1:18 ` [PATCH V2 2/2] scsi: core: put shost->shost_gendev in failure handling path Ming Lei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.