[PATCH] Fix a memory leak in scsi_host_dev_release()

[PATCH] Fix a memory leak in scsi_host_dev_release()

[Bart Van Assche]

Avoid that kmemleak reports the following memory leak if a
SCSI LLD calls scsi_host_alloc() and scsi_host_put() but neither
scsi_host_add() nor scsi_host_remove(). The following shell
command triggers that scenario:

for ((i=0; i<2; i++)); do
  srp_daemon -oac |
  while read line; do
    echo $line >/sys/class/infiniband_srp/srp-mlx4_0-1/add_target
  done
done

unreferenced object 0xffff88021b24a220 (size 8):
  comm "srp_daemon", pid 56421, jiffies 4295006762 (age 4240.750s)
  hex dump (first 8 bytes):
    68 6f 73 74 35 38 00 a5                          host58..
  backtrace:
    [<ffffffff8151014a>] kmemleak_alloc+0x7a/0xc0
    [<ffffffff81165c1e>] __kmalloc_track_caller+0xfe/0x160
    [<ffffffff81260d2b>] kvasprintf+0x5b/0x90
    [<ffffffff81260e2d>] kvasprintf_const+0x8d/0xb0
    [<ffffffff81254b0c>] kobject_set_name_vargs+0x3c/0xa0
    [<ffffffff81337e3c>] dev_set_name+0x3c/0x40
    [<ffffffff81355757>] scsi_host_alloc+0x327/0x4b0
    [<ffffffffa03edc8e>] srp_create_target+0x4e/0x8a0 [ib_srp]
    [<ffffffff8133778b>] dev_attr_store+0x1b/0x20
    [<ffffffff811f27fa>] sysfs_kf_write+0x4a/0x60
    [<ffffffff811f1e8e>] kernfs_fop_write+0x14e/0x180
    [<ffffffff81176eef>] __vfs_write+0x2f/0xf0
    [<ffffffff811771e4>] vfs_write+0xa4/0x100
    [<ffffffff81177c64>] SyS_write+0x54/0xc0
    [<ffffffff8151b257>] entry_SYSCALL_64_fastpath+0x12/0x6f

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: stable <stable@vger.kernel.org>
---
 drivers/scsi/hosts.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 323982f..82ac1cd 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -333,6 +333,17 @@ static void scsi_host_dev_release(struct device *dev)
 		kfree(queuedata);
 	}
 
+	if (shost->shost_state == SHOST_CREATED) {
+		/*
+		 * Free the shost_dev device name here if scsi_host_alloc()
+		 * and scsi_host_put() have been called but neither
+		 * scsi_host_add() nor scsi_host_remove() has been called.
+		 * This avoids that the memory allocated for the shost_dev
+		 * name is leaked.
+		 */
+		kfree(dev_name(&shost->shost_dev));
+	}
+
 	scsi_destroy_command_freelist(shost);
 	if (shost_use_blk_mq(shost)) {
 		if (shost->tag_set.tags)
-- 
2.1.4

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Christoph Hellwig]

Hi Bart,

the memory leak looks real, and your fix looks corret, but I still
don't like it.

I think it's reasonable for SCSI to assume that the final put_device
fully frees the struct device including the name pointer that is
assigned entirely behind the back of the caller.

So I think the fix for this probably should be in the driver core.

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Bart Van Assche]

On 11/20/2015 03:52 AM, Christoph Hellwig wrote:
> the memory leak looks real, and your fix looks corret, but I still
> don't like it.
>
> I think it's reasonable for SCSI to assume that the final put_device
> fully frees the struct device including the name pointer that is
> assigned entirely behind the back of the caller.
>
> So I think the fix for this probably should be in the driver core.

Hello Christoph,

Thanks for the feedback. However, I'm not sure this can be fixed by 
modifying the driver core. If scsi_host_remove() is not called the SCSI 
core doesn't call put_device(&shost->shost_dev). I will post a second 
version of this patch that ensures that the SCSI core always calls 
put_device(&shost->shost_dev).

Bart.

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Bart Van Assche]

On 11/20/2015 03:52 AM, Christoph Hellwig wrote:
> the memory leak looks real, and your fix looks corret, but I still
> don't like it.
>
> I think it's reasonable for SCSI to assume that the final put_device
> fully frees the struct device including the name pointer that is
> assigned entirely behind the back of the caller.
>
> So I think the fix for this probably should be in the driver core.

Hello Christoph,

Thanks for the feedback. However, I'm not sure this can be fixed by 
modifying the driver core. If scsi_host_remove() is not called the SCSI 
core doesn't call put_device(&shost->shost_dev). I will post a second 
version of this patch that ensures that the SCSI core always calls 
put_device(&shost->shost_dev).

Bart.

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Christoph Hellwig]

On Fri, Nov 20, 2015 at 09:49:42AM -0800, Bart Van Assche wrote:
> On 11/20/2015 03:52 AM, Christoph Hellwig wrote:
> >the memory leak looks real, and your fix looks corret, but I still
> >don't like it.
> >
> >I think it's reasonable for SCSI to assume that the final put_device
> >fully frees the struct device including the name pointer that is
> >assigned entirely behind the back of the caller.
> >
> >So I think the fix for this probably should be in the driver core.
> 
> Hello Christoph,
> 
> Thanks for the feedback. However, I'm not sure this can be fixed by
> modifying the driver core. If scsi_host_remove() is not called the SCSI core
> doesn't call put_device(&shost->shost_dev). I will post a second version of
> this patch that ensures that the SCSI core always calls
> put_device(&shost->shost_dev).


Oh, I see.  The release method is called on shost_gendev, but the
name that needs to be freed is in shost_dev.  I take my comment on the
core back.

Let's get this patch in for now and see if we can do something about the
creative driver model (ab-)use for struct Scsi_Host in the long run.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Johannes Thumshirn]

On Wed, 2015-11-18 at 14:56 -0800, Bart Van Assche wrote:
> Avoid that kmemleak reports the following memory leak if a
> SCSI LLD calls scsi_host_alloc() and scsi_host_put() but neither
> scsi_host_add() nor scsi_host_remove(). The following shell
> command triggers that scenario:
> 
> for ((i=0; i<2; i++)); do
>   srp_daemon -oac |
>   while read line; do
>     echo $line >/sys/class/infiniband_srp/srp-mlx4_0-1/add_target
>   done
> done
> 
> unreferenced object 0xffff88021b24a220 (size 8):
>   comm "srp_daemon", pid 56421, jiffies 4295006762 (age 4240.750s)
>   hex dump (first 8 bytes):
>     68 6f 73 74 35 38 00 a5                          host58..
>   backtrace:
>     [<ffffffff8151014a>] kmemleak_alloc+0x7a/0xc0
>     [<ffffffff81165c1e>] __kmalloc_track_caller+0xfe/0x160
>     [<ffffffff81260d2b>] kvasprintf+0x5b/0x90
>     [<ffffffff81260e2d>] kvasprintf_const+0x8d/0xb0
>     [<ffffffff81254b0c>] kobject_set_name_vargs+0x3c/0xa0
>     [<ffffffff81337e3c>] dev_set_name+0x3c/0x40
>     [<ffffffff81355757>] scsi_host_alloc+0x327/0x4b0
>     [<ffffffffa03edc8e>] srp_create_target+0x4e/0x8a0 [ib_srp]
>     [<ffffffff8133778b>] dev_attr_store+0x1b/0x20
>     [<ffffffff811f27fa>] sysfs_kf_write+0x4a/0x60
>     [<ffffffff811f1e8e>] kernfs_fop_write+0x14e/0x180
>     [<ffffffff81176eef>] __vfs_write+0x2f/0xf0
>     [<ffffffff811771e4>] vfs_write+0xa4/0x100
>     [<ffffffff81177c64>] SyS_write+0x54/0xc0
>     [<ffffffff8151b257>] entry_SYSCALL_64_fastpath+0x12/0x6f
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: stable <stable@vger.kernel.org>
> ---
>  drivers/scsi/hosts.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 323982f..82ac1cd 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -333,6 +333,17 @@ static void scsi_host_dev_release(struct device
> *dev)
>  		kfree(queuedata);
>  	}
>  
> +	if (shost->shost_state == SHOST_CREATED) {
> +		/*
> +		 * Free the shost_dev device name here if
> scsi_host_alloc()
> +		 * and scsi_host_put() have been called but neither
> +		 * scsi_host_add() nor scsi_host_remove() has been
> called.
> +		 * This avoids that the memory allocated for the
> shost_dev
> +		 * name is leaked.
> +		 */
> +		kfree(dev_name(&shost->shost_dev));
> +	}
> +
>  	scsi_destroy_command_freelist(shost);
>  	if (shost_use_blk_mq(shost)) {
>  		if (shost->tag_set.tags)

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Sagi Grimberg]

On 19/11/2015 00:56, Bart Van Assche wrote:
> Avoid that kmemleak reports the following memory leak if a
> SCSI LLD calls scsi_host_alloc() and scsi_host_put() but neither
> scsi_host_add() nor scsi_host_remove(). The following shell
> command triggers that scenario:

Looks good,

Reviewed-by: Sagi Grimberg <sagig@mellanox.com>

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Lee Duncan]

On 11/18/2015 02:56 PM, Bart Van Assche wrote:
> Avoid that kmemleak reports the following memory leak if a
> SCSI LLD calls scsi_host_alloc() and scsi_host_put() but neither
> scsi_host_add() nor scsi_host_remove(). The following shell
> command triggers that scenario:
> 
> for ((i=0; i<2; i++)); do
>   srp_daemon -oac |
>   while read line; do
>     echo $line >/sys/class/infiniband_srp/srp-mlx4_0-1/add_target
>   done
> done
> 
> unreferenced object 0xffff88021b24a220 (size 8):
>   comm "srp_daemon", pid 56421, jiffies 4295006762 (age 4240.750s)
>   hex dump (first 8 bytes):
>     68 6f 73 74 35 38 00 a5                          host58..
>   backtrace:
>     [<ffffffff8151014a>] kmemleak_alloc+0x7a/0xc0
>     [<ffffffff81165c1e>] __kmalloc_track_caller+0xfe/0x160
>     [<ffffffff81260d2b>] kvasprintf+0x5b/0x90
>     [<ffffffff81260e2d>] kvasprintf_const+0x8d/0xb0
>     [<ffffffff81254b0c>] kobject_set_name_vargs+0x3c/0xa0
>     [<ffffffff81337e3c>] dev_set_name+0x3c/0x40
>     [<ffffffff81355757>] scsi_host_alloc+0x327/0x4b0
>     [<ffffffffa03edc8e>] srp_create_target+0x4e/0x8a0 [ib_srp]
>     [<ffffffff8133778b>] dev_attr_store+0x1b/0x20
>     [<ffffffff811f27fa>] sysfs_kf_write+0x4a/0x60
>     [<ffffffff811f1e8e>] kernfs_fop_write+0x14e/0x180
>     [<ffffffff81176eef>] __vfs_write+0x2f/0xf0
>     [<ffffffff811771e4>] vfs_write+0xa4/0x100
>     [<ffffffff81177c64>] SyS_write+0x54/0xc0
>     [<ffffffff8151b257>] entry_SYSCALL_64_fastpath+0x12/0x6f
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: stable <stable@vger.kernel.org>
> ---
>  drivers/scsi/hosts.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 323982f..82ac1cd 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -333,6 +333,17 @@ static void scsi_host_dev_release(struct device *dev)
>  		kfree(queuedata);
>  	}
>  
> +	if (shost->shost_state == SHOST_CREATED) {
> +		/*
> +		 * Free the shost_dev device name here if scsi_host_alloc()
> +		 * and scsi_host_put() have been called but neither
> +		 * scsi_host_add() nor scsi_host_remove() has been called.
> +		 * This avoids that the memory allocated for the shost_dev
> +		 * name is leaked.
> +		 */
> +		kfree(dev_name(&shost->shost_dev));
> +	}
> +
>  	scsi_destroy_command_freelist(shost);
>  	if (shost_use_blk_mq(shost)) {
>  		if (shost->tag_set.tags)
> 

Reviewed-by: Lee Duncan <lduncan@suse.com>

Re: [PATCH] Fix a memory leak in scsi_host_dev_release()

[Martin K. Petersen]

>>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:

Bart> Avoid that kmemleak reports the following memory leak if a SCSI
Bart> LLD calls scsi_host_alloc() and scsi_host_put() but neither
Bart> scsi_host_add() nor scsi_host_remove(). The following shell
Bart> command triggers that scenario:

Applied to 4.4/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering