All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND PATCH v2] scsi: hpsa: fix memory leak in hpsa_init_one
@ 2020-09-30 15:50 Keita Suzuki
  2020-10-26 21:49 ` Martin K. Petersen
  0 siblings, 1 reply; 6+ messages in thread
From: Keita Suzuki @ 2020-09-30 15:50 UTC (permalink / raw)
  Cc: keitasuzuki.park, takafumi, Don Brace, James E.J. Bottomley,
	Martin K. Petersen, Kevin Barnett, Johannes Thumshirn,
	Scott Teel, esc.storagedev, linux-scsi, linux-kernel

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(-)

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;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH v2] scsi: hpsa: fix memory leak in hpsa_init_one
  2020-09-30 15:50 [RESEND PATCH v2] scsi: hpsa: fix memory leak in hpsa_init_one Keita Suzuki
@ 2020-10-26 21:49 ` Martin K. Petersen
  2020-10-27  1:22   ` Keita Suzuki
  0 siblings, 1 reply; 6+ messages in thread
From: Martin K. Petersen @ 2020-10-26 21:49 UTC (permalink / raw)
  To: Keita Suzuki
  Cc: takafumi, Don Brace, James E.J. Bottomley, Martin K. Petersen,
	Kevin Barnett, Johannes Thumshirn, Scott Teel, esc.storagedev,
	linux-scsi, linux-kernel


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RESEND PATCH v2] scsi: hpsa: fix memory leak in hpsa_init_one
  2020-10-26 21:49 ` Martin K. Petersen
@ 2020-10-27  1:22   ` Keita Suzuki
  2020-10-27  7:31     ` [PATCH v3] " Keita Suzuki
  0 siblings, 1 reply; 6+ messages in thread
From: Keita Suzuki @ 2020-10-27  1:22 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: Takafumi Kubota, Don Brace, James E.J. Bottomley, Kevin Barnett,
	Johannes Thumshirn, Scott Teel, esc.storagedev, linux-scsi,
	open list

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3] scsi: hpsa: fix memory leak in hpsa_init_one
  2020-10-27  1:22   ` Keita Suzuki
@ 2020-10-27  7:31     ` Keita Suzuki
  2020-10-27 15:27       ` Don.Brace
  2020-10-30  2:20       ` Martin K. Petersen
  0 siblings, 2 replies; 6+ messages in thread
From: Keita Suzuki @ 2020-10-27  7:31 UTC (permalink / raw)
  Cc: keitasuzuki.park, takafumi, Don Brace, James E.J. Bottomley,
	Martin K. Petersen, esc.storagedev, linux-scsi, linux-kernel

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.

Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
---
v3: revert label name to numbered labels
v2: rename labels
 drivers/scsi/hpsa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 48d5da59262b..aed59ec20ad9 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -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 clean8; /* lastlogicals, perf, sg, cmd, irq, shost, pci, lu, aer/h */
 
 	/* Monitor the controller for firmware lockups */
 	h->heartbeat_sample_interval = HEARTBEAT_SAMPLE_INTERVAL;
@@ -8869,6 +8869,8 @@ static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				HPSA_EVENT_MONITOR_INTERVAL);
 	return 0;
 
+clean8: /* lastlogicals, perf, sg, cmd, irq, shost, pci, lu, aer/h */
+	kfree(h->lastlogicals);
 clean7: /* perf, sg, cmd, irq, shost, pci, lu, aer/h */
 	hpsa_free_performant_mode(h);
 	h->access.set_intr_mask(h, HPSA_INTR_OFF);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: [PATCH v3] scsi: hpsa: fix memory leak in hpsa_init_one
  2020-10-27  7:31     ` [PATCH v3] " Keita Suzuki
@ 2020-10-27 15:27       ` Don.Brace
  2020-10-30  2:20       ` Martin K. Petersen
  1 sibling, 0 replies; 6+ messages in thread
From: Don.Brace @ 2020-10-27 15:27 UTC (permalink / raw)
  To: keitasuzuki.park
  Cc: takafumi, don.brace, jejb, martin.petersen, esc.storagedev,
	linux-scsi, linux-kernel

-----Original Message-----
From: Keita Suzuki [mailto:keitasuzuki.park@sslab.ics.keio.ac.jp] 
Sent: Tuesday, October 27, 2020 2:31 AM
Cc: keitasuzuki.park@sslab.ics.keio.ac.jp; takafumi@sslab.ics.keio.ac.jp; Don Brace <don.brace@microsemi.com>; James E.J. Bottomley <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>; esc.storagedev@microsemi.com; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [PATCH v3] scsi: hpsa: fix memory leak in hpsa_init_one

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

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.

Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>

Acked-by: Don Brace <don.brace@microchip.com>
Tested-by: Don Brace <don.brace@microchip.com>

---
v3: revert label name to numbered labels
v2: rename labels
 drivers/scsi/hpsa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 48d5da59262b..aed59ec20ad9 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -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 clean8; /* lastlogicals, perf, sg, cmd, irq, shost, 
+ pci, lu, aer/h */

        /* Monitor the controller for firmware lockups */
        h->heartbeat_sample_interval = HEARTBEAT_SAMPLE_INTERVAL; @@ -8869,6 +8869,8 @@ static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
                                HPSA_EVENT_MONITOR_INTERVAL);
        return 0;

+clean8: /* lastlogicals, perf, sg, cmd, irq, shost, pci, lu, aer/h */
+       kfree(h->lastlogicals);
 clean7: /* perf, sg, cmd, irq, shost, pci, lu, aer/h */
        hpsa_free_performant_mode(h);
        h->access.set_intr_mask(h, HPSA_INTR_OFF);
--
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] scsi: hpsa: fix memory leak in hpsa_init_one
  2020-10-27  7:31     ` [PATCH v3] " Keita Suzuki
  2020-10-27 15:27       ` Don.Brace
@ 2020-10-30  2:20       ` Martin K. Petersen
  1 sibling, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2020-10-30  2:20 UTC (permalink / raw)
  To: Keita Suzuki
  Cc: Martin K . Petersen, James E.J. Bottomley, linux-scsi, Don Brace,
	esc.storagedev, linux-kernel, takafumi

On Tue, 27 Oct 2020 07:31:24 +0000, Keita Suzuki wrote:

> 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.

Applied to 5.10/scsi-fixes, thanks!

[1/1] scsi: hpsa: Fix memory leak in hpsa_init_one()
      https://git.kernel.org/mkp/scsi/c/af61bc1e33d2

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-10-30  2:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 15:50 [RESEND PATCH v2] scsi: hpsa: fix memory leak in hpsa_init_one Keita Suzuki
2020-10-26 21:49 ` Martin K. Petersen
2020-10-27  1:22   ` Keita Suzuki
2020-10-27  7:31     ` [PATCH v3] " Keita Suzuki
2020-10-27 15:27       ` Don.Brace
2020-10-30  2:20       ` Martin K. Petersen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.