diff mbox series

[RESEND,v2] scsi: hpsa: fix memory leak in hpsa_init_one

Message ID 20200930155100.11528-1-keitasuzuki.park@sslab.ics.keio.ac.jp
State New
Headers show
Series [RESEND,v2] scsi: hpsa: fix memory leak in hpsa_init_one | expand

Commit Message

Keita Suzuki Sept. 30, 2020, 3:50 p.m. UTC
When hpsa_scsi_add_host fails, h->lastlogicals is leaked since it lacks
free in the error handler.

Fix this by adding free when hpsa_scsi_add_host fails.

This patch also renames the numbered labels to detailed names.

Fixes: cf47723763a7 ("hpsa: correct initialization order issue")
Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
---
 drivers/scsi/hpsa.c | 52 +++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

Comments

Martin K. Petersen Oct. 26, 2020, 9:49 p.m. UTC | #1
Keita,

> When hpsa_scsi_add_host fails, h->lastlogicals is leaked since it lacks
> free in the error handler.
>
> Fix this by adding free when hpsa_scsi_add_host fails.
>
> This patch also renames the numbered labels to detailed names.

While I am no fan of numbered labels, these initialization stages are
referenced several other places in the driver. As a result, renaming the
labels makes the rest of the code harder to follow.

I suggest you submit a fix for just the leak. And then, if the hpsa
maintainers agree, we can entertain a separate patch to improve the
naming.

Thank you!
Keita Suzuki Oct. 27, 2020, 1:22 a.m. UTC | #2
Hi Martin,

Thanks for the review.

> I suggest you submit a fix for just the leak. And then, if the hpsa

> maintainers agree, we can entertain a separate patch to improve the

> naming.


I'll revert the labels to numbered labels and resend the patch.

Thanks,
Keita

2020年10月27日(火) 6:49 Martin K. Petersen <martin.petersen@oracle.com>:
>

>

> Keita,

>

> > When hpsa_scsi_add_host fails, h->lastlogicals is leaked since it lacks

> > free in the error handler.

> >

> > Fix this by adding free when hpsa_scsi_add_host fails.

> >

> > This patch also renames the numbered labels to detailed names.

>

> While I am no fan of numbered labels, these initialization stages are

> referenced several other places in the driver. As a result, renaming the

> labels makes the rest of the code harder to follow.

>

> I suggest you submit a fix for just the leak. And then, if the hpsa

> maintainers agree, we can entertain a separate patch to improve the

> naming.

>

> Thank you!

>

> --

> Martin K. Petersen      Oracle Linux Engineering
diff mbox series

Patch

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 48d5da59262b..4911ca22efe4 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -8691,19 +8691,19 @@  static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!h->lockup_detected) {
 		dev_err(&h->pdev->dev, "Failed to allocate lockup detector\n");
 		rc = -ENOMEM;
-		goto clean1;	/* aer/h */
+		goto out_destroy_workqueue;	/* aer/h */
 	}
 	set_lockup_detected_for_all_cpus(h, 0);
 
 	rc = hpsa_pci_init(h);
 	if (rc)
-		goto clean2;	/* lu, aer/h */
+		goto out_free_lockup_detected;	/* lu, aer/h */
 
 	/* relies on h-> settings made by hpsa_pci_init, including
 	 * interrupt_mode h->intr */
 	rc = hpsa_scsi_host_alloc(h);
 	if (rc)
-		goto clean2_5;	/* pci, lu, aer/h */
+		goto out_free_pci_init;	/* pci, lu, aer/h */
 
 	sprintf(h->devname, HPSA "%d", h->scsi_host->host_no);
 	h->ctlr = number_of_controllers;
@@ -8719,7 +8719,7 @@  static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 			dac = 0;
 		} else {
 			dev_err(&pdev->dev, "no suitable DMA available\n");
-			goto clean3;	/* shost, pci, lu, aer/h */
+			goto out_scsi_host_put;	/* shost, pci, lu, aer/h */
 		}
 	}
 
@@ -8728,13 +8728,13 @@  static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	rc = hpsa_request_irqs(h, do_hpsa_intr_msi, do_hpsa_intr_intx);
 	if (rc)
-		goto clean3;	/* shost, pci, lu, aer/h */
+		goto out_scsi_host_put;	/* shost, pci, lu, aer/h */
 	rc = hpsa_alloc_cmd_pool(h);
 	if (rc)
-		goto clean4;	/* irq, shost, pci, lu, aer/h */
+		goto out_free_irqs;	/* irq, shost, pci, lu, aer/h */
 	rc = hpsa_alloc_sg_chain_blocks(h);
 	if (rc)
-		goto clean5;	/* cmd, irq, shost, pci, lu, aer/h */
+		goto out_free_cmd_pool;	/* cmd, irq, shost, pci, lu, aer/h */
 	init_waitqueue_head(&h->scan_wait_queue);
 	init_waitqueue_head(&h->event_sync_wait_queue);
 	mutex_init(&h->reset_mutex);
@@ -8747,25 +8747,25 @@  static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	spin_lock_init(&h->devlock);
 	rc = hpsa_put_ctlr_into_performant_mode(h);
 	if (rc)
-		goto clean6; /* sg, cmd, irq, shost, pci, lu, aer/h */
+		goto out_free_sg_chain_blocks; /* sg, cmd, irq, shost, pci, lu, aer/h */
 
 	/* create the resubmit workqueue */
 	h->rescan_ctlr_wq = hpsa_create_controller_wq(h, "rescan");
 	if (!h->rescan_ctlr_wq) {
 		rc = -ENOMEM;
-		goto clean7;
+		goto out_free_performant_mode;
 	}
 
 	h->resubmit_wq = hpsa_create_controller_wq(h, "resubmit");
 	if (!h->resubmit_wq) {
 		rc = -ENOMEM;
-		goto clean7;	/* aer/h */
+		goto out_free_performant_mode;	/* aer/h */
 	}
 
 	h->monitor_ctlr_wq = hpsa_create_controller_wq(h, "monitor");
 	if (!h->monitor_ctlr_wq) {
 		rc = -ENOMEM;
-		goto clean7;
+		goto out_free_performant_mode;
 	}
 
 	/*
@@ -8795,20 +8795,20 @@  static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 			 * cannot goto clean7 or free_irqs will be called
 			 * again. Instead, do its work
 			 */
-			hpsa_free_performant_mode(h);	/* clean7 */
-			hpsa_free_sg_chain_blocks(h);	/* clean6 */
-			hpsa_free_cmd_pool(h);		/* clean5 */
+			hpsa_free_performant_mode(h);	/* out_free_performant_mode */
+			hpsa_free_sg_chain_blocks(h);	/* out_free_sg_chain_blocks */
+			hpsa_free_cmd_pool(h);		/* out_free_cmd_pool */
 			/*
 			 * skip hpsa_free_irqs(h) clean4 since that
 			 * was just called before request_irqs failed
 			 */
-			goto clean3;
+			goto out_scsi_host_put;
 		}
 
 		rc = hpsa_kdump_soft_reset(h);
 		if (rc)
 			/* Neither hard nor soft reset worked, we're hosed. */
-			goto clean7;
+			goto out_free_performant_mode;
 
 		dev_info(&h->pdev->dev, "Board READY.\n");
 		dev_info(&h->pdev->dev,
@@ -8854,7 +8854,7 @@  static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* hook into SCSI subsystem */
 	rc = hpsa_scsi_add_host(h);
 	if (rc)
-		goto clean7; /* perf, sg, cmd, irq, shost, pci, lu, aer/h */
+		goto out_free_lastlogicals; /* ll, perf, sg, cmd, irq, shost, pci, lu, aer/h */
 
 	/* Monitor the controller for firmware lockups */
 	h->heartbeat_sample_interval = HEARTBEAT_SAMPLE_INTERVAL;
@@ -8869,26 +8869,28 @@  static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				HPSA_EVENT_MONITOR_INTERVAL);
 	return 0;
 
-clean7: /* perf, sg, cmd, irq, shost, pci, lu, aer/h */
+out_free_lastlogicals: /* ll, perf, sg, cmd, irq, shost, pci, lu, aer/h */
+	kfree(h->lastlogicals);
+out_free_performant_mode: /* perf, sg, cmd, irq, shost, pci, lu, aer/h */
 	hpsa_free_performant_mode(h);
 	h->access.set_intr_mask(h, HPSA_INTR_OFF);
-clean6: /* sg, cmd, irq, pci, lockup, wq/aer/h */
+out_free_sg_chain_blocks: /* sg, cmd, irq, pci, lockup, wq/aer/h */
 	hpsa_free_sg_chain_blocks(h);
-clean5: /* cmd, irq, shost, pci, lu, aer/h */
+out_free_cmd_pool: /* cmd, irq, shost, pci, lu, aer/h */
 	hpsa_free_cmd_pool(h);
-clean4: /* irq, shost, pci, lu, aer/h */
+out_free_irqs: /* irq, shost, pci, lu, aer/h */
 	hpsa_free_irqs(h);
-clean3: /* shost, pci, lu, aer/h */
+out_scsi_host_put: /* shost, pci, lu, aer/h */
 	scsi_host_put(h->scsi_host);
 	h->scsi_host = NULL;
-clean2_5: /* pci, lu, aer/h */
+out_free_pci_init: /* pci, lu, aer/h */
 	hpsa_free_pci_init(h);
-clean2: /* lu, aer/h */
+out_free_lockup_detected: /* lu, aer/h */
 	if (h->lockup_detected) {
 		free_percpu(h->lockup_detected);
 		h->lockup_detected = NULL;
 	}
-clean1:	/* wq/aer/h */
+out_destroy_workqueue:	/* wq/aer/h */
 	if (h->resubmit_wq) {
 		destroy_workqueue(h->resubmit_wq);
 		h->resubmit_wq = NULL;