Fix a scsi_host_dev_release() memory leak
cover Fix a scsi_host_dev_release() memory leak
This patch series consists of two patches:
- A first patch that converts the code that accesses shost->shost_gendev to manipulate the shost reference count into scsi_host_{get,put}() calls.
- A second patch that fixes the actual memory leak.
PATCH 1/2 scsi_transport_fc: Introduce scsi_host_{get,put}()
Use scsi_host_{get,put}() instead of open-coding these functions. Compile-tested only.
--- drivers/scsi/scsi_transport_fc.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/scsi/scsi_transport_fc.c +6 -6
@@ -2586,7 +2586,7 @@ fc_rport_final_delete(struct work_struct *work)transport_remove_device (dev);device_del (dev);transport_destroy_device (dev); -put_device (&shost->shost_gendev);/* for fc_host->rport list */ +scsi_host_put (shost);/* for fc_host->rport list */ put_device (dev);/* for self-reference */ }
@@ -2650,7 +2650,7 @@ fc_rport_create(struct Scsi_Host *shost, int channel,else rport->scsi_target_id =-1 ;list_add_tail (&rport->peers, &fc_host->rports); -get_device (&shost->shost_gendev);/* for fc_host->rport list */ +scsi_host_get (shost);/* for fc_host->rport list */ spin_unlock_irqrestore (shost->host_lock, flags);
@@ -2685,7 +2685,7 @@ delete_rport:transport_destroy_device (dev);spin_lock_irqsave (shost->host_lock, flags);list_del (&rport->peers); -put_device (&shost->shost_gendev);/* for fc_host->rport list */ +scsi_host_put (shost);/* for fc_host->rport list */ spin_unlock_irqrestore (shost->host_lock, flags);put_device (dev->parent);kfree (rport);
@@ -3383,7 +3383,7 @@ fc_vport_setup(struct Scsi_Host *shost, int channel, struct device *pdev, fc_host->npiv_vports_inuse++; vport->number = fc_host->next_vport_number++;list_add_tail (&vport->peers, &fc_host->vports); -get_device (&shost->shost_gendev);/* for fc_host->vport list */ +scsi_host_get (shost);/* for fc_host->vport list */ spin_unlock_irqrestore (shost->host_lock, flags);
@@ -3441,7 +3441,7 @@ delete_vport:transport_destroy_device (dev);spin_lock_irqsave (shost->host_lock, flags);list_del (&vport->peers); -put_device (&shost->shost_gendev);/* for fc_host->vport list */ +scsi_host_put (shost);/* for fc_host->vport list */ fc_host->npiv_vports_inuse--;spin_unlock_irqrestore (shost->host_lock, flags);put_device (dev->parent);
@@ -3504,7 +3504,7 @@ fc_vport_terminate(struct fc_vport *vport) vport->flags |=FC_VPORT_DELETED ;list_del (&vport->peers); fc_host->npiv_vports_inuse--; -put_device (&shost->shost_gendev);/* for fc_host->vport list */ +scsi_host_put (shost);/* for fc_host->vport list */ }spin_unlock_irqrestore (shost->host_lock, flags);
Signature · 1 line
2.1.4
Re: [PATCH 1/2] scsi_transport_fc: Introduce scsi_host_{get,put}()
Quoted 2 lines · Bart Van Assche
Use scsi_host_{get,put}() instead of open-coding these functions. Compile-tested only.
s/Introdue/use/ in the subject?
Otherwise this looks fine:
Re: [PATCH 1/2] scsi_transport_fc: Introduce scsi_host_{get,put}()
In reply to Bart Van AsscheQuoted 83 lines · Bart Van Assche
Use scsi_host_{get,put}() instead of open-coding these functions. Compile-tested only.
Signed-off-by Bart Van Assche <[email protected]>Cc Christoph Hellwig <[email protected]>Cc Hannes Reinecke <[email protected]>Cc James Smart <[email protected]>Cc stable <[email protected]>---
drivers/scsi/scsi_transport_fc.c | 12 ++++++------1 file changed, 6 insertions(+), 6 deletions(-)drivers/scsi/scsi_transport_fc.c +0
-1@@ -2586,7 +2586,7 @@ fc_rport_final_delete(struct work_struct *work) transport_remove_device(dev); device_del(dev); transport_destroy_device(dev); -put_device (&shost->shost_gendev);/* for fc_host- Quoted 1 line
rport list */
drivers/scsi/scsi_transport_fc.c +1
-0+scsi_host_put (shost);/* for fc_host- Quoted 1 line
rport list */
put_device(dev); /* for self-
reference */
}drivers/scsi/scsi_transport_fc.c +0
-0@@ -2650,7 +2650,7 @@ fc_rport_create(struct Scsi_Host *shost, int channel,else
rport->scsi_target_id= -1;list_add_tail(&rport->peers, &fc_host->rports);drivers/scsi/scsi_transport_fc.c +0
-1-get_device (&shost->shost_gendev);/* for fc_host- Quoted 1 line
rport list */
drivers/scsi/scsi_transport_fc.c +1
-0+scsi_host_get (shost);/* for fc_host- Quoted 1 line
rport list */
spin_unlock_irqrestore(shost->host_lock, flags);drivers/scsi/scsi_transport_fc.c +0
-1@@ -2685,7 +2685,7 @@ delete_rport: transport_destroy_device(dev); spin_lock_irqsave(shost->host_lock, flags); list_del(&rport->peers); -put_device (&shost->shost_gendev);/* for fc_host- Quoted 1 line
rport list */
drivers/scsi/scsi_transport_fc.c +1
-0+scsi_host_put (shost);/* for fc_host- Quoted 1 line
rport list */
spin_unlock_irqrestore(shost->host_lock, flags);put_device(dev->parent);
kfree(rport);drivers/scsi/scsi_transport_fc.c +0
-1@@ -3383,7 +3383,7 @@ fc_vport_setup(struct Scsi_Host *shost, int channel, struct device *pdev, fc_host->npiv_vports_inuse++; vport->number = fc_host->next_vport_number++; list_add_tail(&vport->peers, &fc_host->vports); -get_device (&shost->shost_gendev);/* for fc_host- Quoted 1 line
vport list */
drivers/scsi/scsi_transport_fc.c +1
-0+scsi_host_get (shost);/* for fc_host- Quoted 1 line
vport list */
spin_unlock_irqrestore(shost->host_lock, flags);drivers/scsi/scsi_transport_fc.c +0
-1@@ -3441,7 +3441,7 @@ delete_vport: transport_destroy_device(dev); spin_lock_irqsave(shost->host_lock, flags); list_del(&vport->peers); -put_device (&shost->shost_gendev);/* for fc_host- Quoted 1 line
vport list */
drivers/scsi/scsi_transport_fc.c +1
-0+scsi_host_put (shost);/* for fc_host- Quoted 1 line
vport list */
fc_host->npiv_vports_inuse--;
spin_unlock_irqrestore(shost->host_lock, flags);put_device(dev->parent);drivers/scsi/scsi_transport_fc.c +0
-1@@ -3504,7 +3504,7 @@ fc_vport_terminate(struct fc_vport *vport) vport->flags |= FC_VPORT_DELETED; list_del(&vport->peers); fc_host->npiv_vports_inuse--; - put_device(&shost->shost_gendev); /* for fc_host-Quoted 1 line
vport list */
drivers/scsi/scsi_transport_fc.c +1
-0+scsi_host_put (shost);/* for fc_host- Quoted 1 line
vport list */
Quoted 3 lines
}
spin_unlock_irqrestore(shost->host_lock, flags);
--
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
Re: [PATCH 1/2] scsi_transport_fc: Introduce scsi_host_{get,put}()
In reply to Bart Van AsscheQuoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Bart" == Bart Van Assche <[email protected]> writes:
Bart,
Bart> Use scsi_host_{get,put}() instead of open-coding these functions. Bart> Compile-tested only.
Do you want me to queue this as a general cleanup for 4.5 (independently of the leak patch)?
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH 1/2] scsi_transport_fc: Introduce scsi_host_{get,put}()
Quoted 9 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Bart" == Bart Van Assche <[email protected]> writes:
Bart,
Bart> Use scsi_host_{get,put}() instead of open-coding these functions. Bart> Compile-tested only.
Do you want me to queue this as a general cleanup for 4.5 (independently of the leak patch)?
Hello Martin,
That would be appreciated.
Thanks,
Bart.
Re: [PATCH 1/2] scsi_transport_fc: Introduce scsi_host_{get,put}()
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Bart" == Bart Van Assche <[email protected]> writes:
Do you want me to queue this as a general cleanup for 4.5 (independently of the leak patch)?
Bart> That would be appreciated.
OK. Done.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
PATCH 2/2 Fix a memory leak in scsi_host_dev_release()
In reply to Bart Van AsscheFix a memory leak that occurs if a SCSI LLD calls scsi_host_alloc() and scsi_host_put() but neither scsi_host_add() nor scsi_host_remove(). This leak is fixed by ensuring that put_device(&shost->shost_dev) is always called. This patch also removes the get_device() call from scsi_add_host*() and the put_device() call from scsi_remove_host() since these calls are no longer needed.
The following shell command triggers the scenario described above:
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
The kmemleak report is as follows:
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
--- drivers/scsi/hosts.c | 57 +++++++++++++++++++++++++++++++++--------------- include/scsi/scsi_host.h | 5 +++++ 2 files changed, 44 insertions(+), 18 deletions(-)
drivers/scsi/hosts.c +39 -18
@@ -45,16 +45,6 @@static atomic_t scsi_host_next_hn =ATOMIC_INIT (0 );/* host_no for next new host */ -static void scsi_host_cls_release (struct device *dev) -{ -put_device (&class_to_shost (dev)->shost_gendev); -} - -static struct class shost_class = { - .name ="scsi_host" , - .dev_release = scsi_host_cls_release, -}; -/** * scsi_host_set_state - Take the given host through the host state model. * @shost: scsi host to change the state of.
@@ -180,7 +170,7 @@ void scsi_remove_host(struct Scsi_Host *shost)spin_unlock_irqrestore (shost->host_lock, flags);transport_unregister_device (&shost->shost_gendev); -device_unregister (&shost->shost_dev); +device_del (&shost->shost_dev);device_del (&shost->shost_gendev); }EXPORT_SYMBOL (scsi_remove_host);
@@ -263,8 +253,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 );
@@ -311,10 +299,10 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, }EXPORT_SYMBOL (scsi_add_host_with_dma); -static void scsi_host_dev_release (struct device *dev) +static void scsi_host_free (struct kref *kref) { -struct Scsi_Host *shost =dev_to_shost (dev); -struct device *parent = dev->parent; +struct Scsi_Host *shost =container_of (kref,typeof (*shost), kref2); +struct device *parent = shost->shost_gendev.parent;struct request_queue *q;void *queuedata;
@@ -349,6 +337,35 @@ static void scsi_host_dev_release(struct device *dev)kfree (shost); } +/* Called if shost_gendev refcnt drops to zero. */ +static void scsi_host_dev_release (struct device *dev) +{ +struct Scsi_Host *shost =dev_to_shost (dev); + +kref_put (&shost->kref2, scsi_host_free); +} + +/* Called if shost_dev refcnt drops to zero. */ +static void scsi_host_cls_release (struct device *dev) +{ +struct Scsi_Host *shost =class_to_shost (dev); + +kref_put (&shost->kref2, scsi_host_free); +} + +static struct class shost_class = { + .name ="scsi_host" , + .dev_release = scsi_host_cls_release, +}; + +static void scsi_host_release (struct kref *kref) +{ +struct Scsi_Host *shost =container_of (kref,typeof (*shost), kref1); + +put_device (&shost->shost_gendev); +put_device (&shost->shost_dev); +} +static int shost_eh_deadline =-1 ;module_param_named (eh_deadline, shost_eh_deadline, int,S_IRUGO |S_IWUSR );
@@ -467,6 +484,10 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) shost->use_blk_mq = scsi_use_blk_mq && !shost->hostt->disable_blk_mq; +kref_init (&shost->kref1); +kref_init (&shost->kref2); +kref_get (&shost->kref2); +device_initialize (&shost->shost_gendev);dev_set_name (&shost->shost_gendev,"host%d" , shost->host_no); shost->shost_gendev.bus = &scsi_bus_type;
@@ -571,7 +592,7 @@ EXPORT_SYMBOL(scsi_host_lookup);struct Scsi_Host *scsi_host_get (struct Scsi_Host *shost) {if ((shost->shost_state ==SHOST_DEL ) || - !get_device (&shost->shost_gendev)) + !kref_get_unless_zero (&shost->kref1))return NULL ;return shost; }
@@ -583,7 +604,7 @@ EXPORT_SYMBOL(scsi_host_get);**/ void scsi_host_put (struct Scsi_Host *shost) { -put_device (&shost->shost_gendev); +kref_put (&shost->kref1, scsi_host_release); }EXPORT_SYMBOL (scsi_host_put);
include/scsi/scsi_host.h +5 -0
@@ -707,6 +707,11 @@ struct Scsi_Host {enum scsi_host_state shost_state; +/* refcnt manipulated by scsi_host_get() / scsi_host_put() */ +struct kref kref1; +/* refcnt that tracks existence of shost_gendev and shost_dev */ +struct kref kref2; +/* ldm bits */ struct device shost_gendev, shost_dev;
Signature · 1 line
2.1.4
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Bart" == Bart Van Assche <[email protected]> writes:
Bart> This patch series consists of two patches: - A first patch that Bart> converts the code that accesses shost->shost_gendev to manipulate Bart> the shost reference count into scsi_host_{get,put}() calls. - A Bart> second patch that fixes the actual memory leak.
I gave a bunch of reviews for v1 but no takers for v2 of the second patch.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 2 lines · Martin K. Petersen
I gave a bunch of reviews for v1 but no takers for v2 of the second patch.
I think v1 is the right thing to do. Sorry for leading Bart in the wrong direction - the two different device structures in Scsi_Host confused me.