From: Ganliber <[email protected]>
In the function he_start() in drivers/atm/he.c, there
is no unmapping of he_dev->membase in the branch that
exits due to an error like reset failure, which may
cause a memory leak.
Signed-off-by: Ganliber <[email protected]>
Reviewed-by: Dongliang Mu <[email protected]>
---
drivers/atm/he.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/atm/he.c b/drivers/atm/he.c
index ad91cc6a34fc..2d12b46aa5bd 100644
--- a/drivers/atm/he.c
+++ b/drivers/atm/he.c
@@ -1058,6 +1058,7 @@ static int he_start(struct atm_dev *dev)
status = he_readl(he_dev, RESET_CNTL);
if ((status & BOARD_RST_STATUS) == 0) {
hprintk("reset failed\n");
+ iounmap(he_dev->membase);
return -EINVAL;
}
@@ -1114,8 +1115,10 @@ static int he_start(struct atm_dev *dev)
he_writel(he_dev, lb_swap, LB_SWAP);
/* 4.10 initialize the interrupt queues */
- if ((err = he_init_irq(he_dev)) != 0)
+ if ((err = he_init_irq(he_dev)) != 0) {
+ iounmap(he_dev->membase);
return err;
+ }
/* 4.11 enable pci bus controller state machines */
host_cntl |= (OUTFF_ENB | CMDFF_ENB |
@@ -1165,6 +1168,7 @@ static int he_start(struct atm_dev *dev)
if (nvpibits != -1 && nvcibits != -1 && nvpibits+nvcibits != HE_MAXCIDBITS) {
hprintk("nvpibits + nvcibits != %d\n", HE_MAXCIDBITS);
+ iounmap(he_dev->membase);
return -ENODEV;
}
@@ -1413,8 +1417,10 @@ static int he_start(struct atm_dev *dev)
/* 5.1.8 cs block connection memory initialization */
- if (he_init_cs_block_rcm(he_dev) < 0)
+ if (he_init_cs_block_rcm(he_dev) < 0) {
+ iounmap(he_dev->membase);
return -ENOMEM;
+ }
/* 5.1.10 initialize host structures */
@@ -1424,13 +1430,16 @@ static int he_start(struct atm_dev *dev)
sizeof(struct he_tpd), TPD_ALIGNMENT, 0);
if (he_dev->tpd_pool == NULL) {
hprintk("unable to create tpd dma_pool\n");
+ iounmap(he_dev->membase);
return -ENOMEM;
}
INIT_LIST_HEAD(&he_dev->outstanding_tpds);
- if (he_init_group(he_dev, 0) != 0)
+ if (he_init_group(he_dev, 0) != 0) {
+ iounmap(he_dev->membase);
return -ENOMEM;
+ }
for (group = 1; group < HE_NUM_GROUPS; ++group) {
he_writel(he_dev, 0x0, G0_RBPS_S + (group * 32));
@@ -1465,6 +1474,7 @@ static int he_start(struct atm_dev *dev)
&he_dev->hsp_phys, GFP_KERNEL);
if (he_dev->hsp == NULL) {
hprintk("failed to allocate host status page\n");
+ iounmap(he_dev->membase);
return -ENOMEM;
}
he_writel(he_dev, he_dev->hsp_phys, HSP_BA);
--
2.34.1
Gencen Gan <[email protected]> :
> In the function he_start() in drivers/atm/he.c, there
> is no unmapping of he_dev->membase in the branch that
> exits due to an error like reset failure, which may
> cause a memory leak.
Why would he_dev->membase not be unmapped in he_stop() ?
he_stop() is paired with he_start() as soon a he_start() returns
anything different from 0 in he_init_one(). I see no other place
where he_start() is used.
The atm_dev/he_dev pointers also seem correctly set.
--
Ueimor
On 3/10/23 19:28, Francois Romieu wrote:
> Gencen Gan <[email protected]> :
>> In the function he_start() in drivers/atm/he.c, there
>> is no unmapping of he_dev->membase in the branch that
>> exits due to an error like reset failure, which may
>> cause a memory leak.
>
> Why would he_dev->membase not be unmapped in he_stop() ?
>
> he_stop() is paired with he_start() as soon a he_start() returns
> anything different from 0 in he_init_one(). I see no other place
> where he_start() is used.
Yes, you're right. We will check more about reports from the static
checker Smatch.
Smatch should make a false positive here, I think it might be that,
Smatch has an assumption about do and its paired undo functions. The do
function should clean up its own allocation operations. And the paired
undo function can be only called if the do function succeeds.
+cc Dan Carpenter
Maybe @Dan could tell more about this point.
>
> The atm_dev/he_dev pointers also seem correctly set.
>
On Fri, Mar 10, 2023 at 09:15:43PM +0800, Dongliang Mu wrote:
>
>
> On 3/10/23 19:28, Francois Romieu wrote:
> > Gencen Gan <[email protected]> :
> > > In the function he_start() in drivers/atm/he.c, there
> > > is no unmapping of he_dev->membase in the branch that
> > > exits due to an error like reset failure, which may
> > > cause a memory leak.
> >
> > Why would he_dev->membase not be unmapped in he_stop() ?
> >
> > he_stop() is paired with he_start() as soon a he_start() returns
> > anything different from 0 in he_init_one(). I see no other place
> > where he_start() is used.
>
> Yes, you're right. We will check more about reports from the static checker
> Smatch.
>
> Smatch should make a false positive here, I think it might be that, Smatch
> has an assumption about do and its paired undo functions. The do function
> should clean up its own allocation operations. And the paired undo function
> can be only called if the do function succeeds.
>
> +cc Dan Carpenter
>
> Maybe @Dan could tell more about this point.
>
Yes. Smatch is assuming that every function cleans up after itself.
Generally this is the way most functions do it.
Perhaps the best option here is to create a new warning for the double
free bug if this patch were applied.
regards,
dan carpenter