Fix a memory leak in scsi_host_dev_release()

PATCH Applied Bart Van Assche 6 participants 8 messages Nov 18, 2015 – Dec 1, 2015 SCSI

Share & tools

Bart Van Assche

author

2015-11-18 22:56 UTC #0 lore raw

PATCH 1/1 Fix a memory leak in scsi_host_dev_release()

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 <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Hannes Reinecke <[email protected]>

Cc stable <[email protected]>

---
 drivers/scsi/hosts.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/scsi/hosts.c +11 -0

index 323982f..82ac1cd 100644
@@ -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)

Signature · 1 line

2.1.4

Christoph Hellwig

2015-11-20 11:52 UTC #1 lore raw

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.

Bart Van Assche

author

2015-11-20 17:49 UTC #2 lore raw

Quoted 8 lines · Christoph Hellwig

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.

Christoph Hellwig

Reviewed-by

2015-11-22 15:08 UTC #3 lore raw

Quoted 10 lines · Bart Van Assche

Quoted 8 lines · Christoph Hellwig
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.

Quoted 7 lines

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 <[email protected]>

Johannes Thumshirn

Reviewed-by on v1 1/1

2015-11-24 12:55 UTC #4 lore raw

In reply to Bart Van Assche

Quoted 64 lines · 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 <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc stable <[email protected]>
---
drivers/scsi/hosts.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/scsi/hosts.c +0 -0
index 323982f..82ac1cd 100644
@@ -333,6 +333,17 @@ static void scsi_host_dev_release(struct device
 *dev)
  		kfree(queuedata);
  	}
drivers/scsi/hosts.c +3 -0
+	if (shost->shost_state == SHOST_CREATED) {
+		/*
+		 * Free the shost_dev device name here if
scsi_host_alloc()
drivers/scsi/hosts.c +2 -0
+		 * and scsi_host_put() have been called but neither
+		 * scsi_host_add() nor scsi_host_remove() has been
called.
drivers/scsi/hosts.c +1 -0
+		 * This avoids that the memory allocated for the
shost_dev
drivers/scsi/hosts.c +5 -0
+		 * name is leaked.
+		 */
+		kfree(dev_name(&shost->shost_dev));
+	}
+

Quoted 3 lines

scsi_destroy_command_freelist(shost);
if (shost_use_blk_mq(shost)) {
if (shost->tag_set.tags)

Reviewed-by Johannes Thumshirn <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sagi Grimberg

Reviewed-by on v1 1/1

2015-11-24 14:50 UTC #5 lore raw

In reply to Bart Van Assche

Quoted 4 lines · 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:

Looks good,

Reviewed-by Sagi Grimberg <[email protected]>

Lee Duncan

Reviewed-by on v1 1/1

2015-11-24 23:11 UTC #6 lore raw

In reply to Bart Van Assche

Quoted 54 lines · 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 <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc stable <[email protected]>
---
 drivers/scsi/hosts.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/scsi/hosts.c +4 -0
index 323982f..82ac1cd 100644
@@ -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

drivers/scsi/hosts.c static void scsi_host_dev_release(struct device *dev) · line 349

Quoted 10 lines

drivers/scsi/hosts.c +7 -0

+		 * 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 <[email protected]>

Martin K. Petersen

Applied

2015-12-01 01:46 UTC #7 lore raw

In reply to Bart Van Assche

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Bart" == Bart Van Assche <[email protected]> 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.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering