[PATCH 1/9] drivers/mfd/88pm860x-i2c.c: introduce missing kfree

Julia Lawall

unread,

Dec 23, 2011, 12:50:02 PM12/23/11

to

Error handling code following a kzalloc should free the allocated data. At
this point, chip has been allocated and some fields have been initialized,
but it has not been stored anywhere, so it should be freed before leaving
the function.

A simplified version of the semantic match that finds the problem is as
follows: (http://coccinelle.lip6.fr)

// <smpl>
@r exists@
local idexpression x;
statement S;
identifier f1;
position p1,p2;
expression *ptr != NULL;
@@

x@p1 = \(kmalloc\|kzalloc\|kcalloc\)(...);
...
if (x == NULL) S
<... when != x
when != if (...) { <+...x...+> }
x->f1
...>
(
return \(0\|<+...x...+>\|ptr\);
|
return@p2 ...;
)

@script:python@
p1 << r.p1;
p2 << r.p2;
@@

print "* file: %s kmalloc %s return %s" % (p1[0].file,p1[0].line,p2[0].line)
// </smpl>

Signed-off-by: Julia Lawall <ju...@diku.dk>

---
drivers/mfd/88pm860x-i2c.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/88pm860x-i2c.c b/drivers/mfd/88pm860x-i2c.c
index 630f1b5..f93dd95 100644
--- a/drivers/mfd/88pm860x-i2c.c
+++ b/drivers/mfd/88pm860x-i2c.c
@@ -286,6 +286,7 @@ static int __devinit pm860x_probe(struct i2c_client *client,
ret = PTR_ERR(chip->regmap);
dev_err(&client->dev, "Failed to allocate register map: %d\n",
ret);
+ kfree(chip);
return ret;
}
chip->client = client;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Mark Brown

unread,

Dec 26, 2011, 7:10:01 AM12/26/11

to

On Fri, Dec 23, 2011 at 06:39:26PM +0100, Julia Lawall wrote:
> Error handling code following a kzalloc should free the allocated data. At
> this point, chip has been allocated and some fields have been initialized,
> but it has not been stored anywhere, so it should be freed before leaving
> the function.

With this and probably a bunch of the other corrections in this series a
conversion to devm_kazlloc() would be a more complete fix as it prevents
people introducing similar missing cleanup paths in future. Not sure if
spatch can generate that automatically though...

Julia Lawall

unread,

Dec 26, 2011, 7:20:01 AM12/26/11

to

On Mon, 26 Dec 2011, Mark Brown wrote:

> On Fri, Dec 23, 2011 at 06:39:26PM +0100, Julia Lawall wrote:
>> Error handling code following a kzalloc should free the allocated data. At
>> this point, chip has been allocated and some fields have been initialized,
>> but it has not been stored anywhere, so it should be freed before leaving
>> the function.
>
> With this and probably a bunch of the other corrections in this series a
> conversion to devm_kazlloc() would be a more complete fix as it prevents
> people introducing similar missing cleanup paths in future. Not sure if
> spatch can generate that automatically though...

I saw a patch for that recently, and looked into it a little bit, but I
wasn't sure what should be done. What are the criteria for using
devm_kzalloc?

julia

Mark Brown

unread,

Dec 26, 2011, 7:30:02 AM12/26/11

to

On Mon, Dec 26, 2011 at 01:18:54PM +0100, Julia Lawall wrote:
> On Mon, 26 Dec 2011, Mark Brown wrote:

> >With this and probably a bunch of the other corrections in this series a
> >conversion to devm_kazlloc() would be a more complete fix as it prevents
> >people introducing similar missing cleanup paths in future. Not sure if
> >spatch can generate that automatically though...

> I saw a patch for that recently, and looked into it a little bit,
> but I wasn't sure what should be done. What are the criteria for
> using devm_kzalloc?

It's good for any allocation that lasts for the lifetime of the device -
most driver data allocated in probe() fits well, for example.

Julia Lawall

unread,

Dec 26, 2011, 8:50:03 AM12/26/11

to

Is it reasonable to use devm_kfree in a probe function that fails or a
remove function that succeeds?

Examples from drivers/watchdog/shwdt.c:

sh_wdt_probe:

...
out_unreg:
unregister_reboot_notifier(&sh_wdt_notifier);
out_unmap:
- devm_iounmap(&pdev->dev, wdt->base);
out_err:
- devm_kfree(&pdev->dev, wdt);
out_release:
- devm_release_mem_region(&pdev->dev, res->start,
resource_size(res));

return rc;
}

sh_wdt_remove:

...
unregister_reboot_notifier(&sh_wdt_notifier);
devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
devm_iounmap(&pdev->dev, wdt->base);
devm_kfree(&pdev->dev, wdt);

return 0;
}

On the other hand, perhaps devm_kfree is needed in probe function that
succeeds, eg in drivers/input/keyboard/samsung-keypad.c:

if (pdev->dev.of_node) {
devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
devm_kfree(&pdev->dev, (void *)pdata);
}
return 0;

julia

Mark Brown

unread,

Dec 26, 2011, 9:30:01 AM12/26/11

to

On Mon, Dec 26, 2011 at 02:44:58PM +0100, Julia Lawall wrote:
> Is it reasonable to use devm_kfree in a probe function that fails or
> a remove function that succeeds?

If we need to do this I'd suggest that we should be fixing the manager
to not need it rather than having drivers do this.

Julia Lawall

unread,

Dec 26, 2011, 11:20:01 AM12/26/11

to

And I guess that eg using kfree to free something allocated using
devm_kzalloc would be completely wrong, leading to a dangling pointer?

julia

Mark Brown

unread,

Dec 26, 2011, 11:20:01 AM12/26/11

to

On Mon, Dec 26, 2011 at 05:17:26PM +0100, Julia Lawall wrote:
> And I guess that eg using kfree to free something allocated using
> devm_kzalloc would be completely wrong, leading to a dangling
> pointer?

I'd expect that matching kfree() with devm_kzalloc() would lead to an
attempt to double free whenever the devm_ mechanism kicks in.

Adam Jiang

unread,

Dec 26, 2011, 9:50:01 PM12/26/11

to

2011/12/26 Julia Lawall <julia....@lip6.fr>:

> Is it reasonable to use devm_kfree in a probe function that fails or a
> remove function that succeeds?

I cannot catch your question. If the memory is allocated by
devm_kzalloc(), it is not necessary to release with devm_kfree() at
all. It would be release once the driver was detached or destroyed.

>
> Examples from drivers/watchdog/shwdt.c:
>
> sh_wdt_probe:
>
> ...
> out_unreg:
> unregister_reboot_notifier(&sh_wdt_notifier);
> out_unmap:
> - devm_iounmap(&pdev->dev, wdt->base);
> out_err:
> - devm_kfree(&pdev->dev, wdt);

It is necessary?

> out_release:
> - devm_release_mem_region(&pdev->dev, res->start, resource_size(res));

It is necessary?

>
> return rc;
> }
>
>
> sh_wdt_remove:
>
> ...
> unregister_reboot_notifier(&sh_wdt_notifier);
> devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
> devm_iounmap(&pdev->dev, wdt->base);
> devm_kfree(&pdev->dev, wdt);

These lines should be deleted if the driver has detached.

Julia Lawall

unread,

Dec 27, 2011, 1:20:01 AM12/27/11

to

On Tue, 27 Dec 2011, Adam Jiang wrote:

> 2011/12/26 Julia Lawall <julia....@lip6.fr>:
>> Is it reasonable to use devm_kfree in a probe function that fails or a
>> remove function that succeeds?
>
> I cannot catch your question. If the memory is allocated by
> devm_kzalloc(), it is not necessary to release with devm_kfree() at
> all. It would be release once the driver was detached or destroyed.

Perhaps it is undesirable to keep it around until then, as in my last
example below where the probe function seems to want to go with a
different solution in some cases?

I was also worried that one call could make some information required by
another one unavailable. For example in the following case:

drivers/dma/mpc512x_dma.c

devm_free_irq(dev, mdma->irq, mdma);
irq_dispose_mapping(mdma->irq);

Is it OK to call irq_dispose_mapping and then let devm_free_irq be called
implicitly afterwards?

julia

> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in