[PATCH V2 0/2] scsi: two fixes in scsi_add_host_with_dma

[PATCH V2 0/2] scsi: two fixes in scsi_add_host_with_dma

[Ming Lei]

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

[PATCH V2 1/2] 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 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

[PATCH V2 2/2] scsi: core: put shost->shost_gendev in failure handling path

[Ming Lei]

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

Re: [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma

[John Garry]

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

Re: [PATCH V2 1/2] scsi: core: fix failure handling of scsi_add_host_with_dma

[Ming Lei]

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