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