Skip to contentSkip to conversation

Fix a scsi_host_dev_release() memory leak

PATCH In review Bart Van Assche 4 participants 10 messages Nov 20, 2015 – Nov 30, 2015 SCSI

lore
Share & tools
lore
#0 lore raw

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.

#1 lore raw

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.

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 +6 -6
index 24eaaf6..8a88226 100644
@@ -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
Reviewed-by on v1 1/2
#2 lore raw

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:

Reviewed-by on v1 1/2
#3 lore raw

Re: [PATCH 1/2] scsi_transport_fc: Introduce scsi_host_{get,put}()

In reply to Bart Van Assche
Quoted 83 lines · Bart Van Assche

Use scsi_host_{get,put}() instead of open-coding these functions. Compile-tested only.

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
index 24eaaf6..8a88226 100644
@@ -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

#4 lore raw

Re: [PATCH 1/2] scsi_transport_fc: Introduce scsi_host_{get,put}()

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,

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
#5 lore raw

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.

#6 lore raw

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
#7 lore raw

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

In reply to Bart Van Assche

Fix 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
Cc Christoph Hellwig <[email protected]>
Cc Hannes Reinecke <[email protected]>
Cc stable <[email protected]>
---
 drivers/scsi/hosts.c     | 57 +++++++++++++++++++++++++++++++++---------------
 include/scsi/scsi_host.h |  5 +++++
 2 files changed, 44 insertions(+), 18 deletions(-)
drivers/scsi/hosts.c +39 -18
index 323982f..b9fa1f9 100644
@@ -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
index ed52712..4f32cf3 100644
@@ -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
#8 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> 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
#9 lore raw
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.