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

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

[Julia Lawall]

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 <julia@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;

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

[Mark Brown]

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

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

[Julia Lawall]

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

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

[Mark Brown]

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.

devm_kfree

[Julia Lawall]

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

Re: devm_kfree

[Mark Brown]

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.

Re: devm_kfree

[Julia Lawall]

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

julia

Re: devm_kfree

[Mark Brown]

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.

Re: devm_kfree

[Adam Jiang]

2011/12/26 Julia Lawall <julia.lawall@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.

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

Re: devm_kfree

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2625 bytes --]

On Tue, 27 Dec 2011, Adam Jiang wrote:

> 2011/12/26 Julia Lawall <julia.lawall@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



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