* question about drivers/pinctrl/pinctrl-at91.c
@ 2012-12-08 15:52 Julia Lawall
2012-12-11 8:51 ` Linus Walleij
2012-12-11 9:06 ` Grant Likely
0 siblings, 2 replies; 6+ messages in thread
From: Julia Lawall @ 2012-12-08 15:52 UTC (permalink / raw)
To: linux-arm-kernel
The function at91_dt_node_to_map in drivers/pinctrl/pinctrl-at91.c
contains the following code:
new_map = devm_kzalloc(pctldev->dev, sizeof(*new_map) * map_num, GFP_KERNEL);
if (!new_map)
return -ENOMEM;
*map = new_map;
*num_maps = map_num;
/* create mux map */
parent = of_get_parent(np);
if (!parent) {
kfree(new_map);
return -EINVAL;
}
This is clearly not correct, because the combination of devm_kzalloc and
kfree risks creating a double free. But I am not sure how best to fix it.
Is the data structure intended to normally exist until the driver's remove
function is called? If so, perhaps the devm_kzalloc is OK. If I just
remove the kfree, then the structure will persist until the remove
function is called, even though there was an error, which is perhaps not
good. So I could change the kfree to devm_kfree?
thanks,
julia
^ permalink raw reply [flat|nested] 6+ messages in thread
* question about drivers/pinctrl/pinctrl-at91.c
2012-12-08 15:52 question about drivers/pinctrl/pinctrl-at91.c Julia Lawall
@ 2012-12-11 8:51 ` Linus Walleij
2012-12-11 9:04 ` Julia Lawall
2012-12-11 9:06 ` Grant Likely
1 sibling, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2012-12-11 8:51 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, Dec 8, 2012 at 4:52 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> The function at91_dt_node_to_map in drivers/pinctrl/pinctrl-at91.c contains
> the following code:
>
> new_map = devm_kzalloc(pctldev->dev, sizeof(*new_map) * map_num,
> GFP_KERNEL);
> if (!new_map)
> return -ENOMEM;
>
> *map = new_map;
> *num_maps = map_num;
>
> /* create mux map */
> parent = of_get_parent(np);
> if (!parent) {
> kfree(new_map);
> return -EINVAL;
> }
>
> This is clearly not correct, because the combination of devm_kzalloc and
> kfree risks creating a double free.
Agreed, probably just some spurious leftover.
> But I am not sure how best to fix it.
> Is the data structure intended to normally exist until the driver's remove
> function is called? If so, perhaps the devm_kzalloc is OK. If I just
> remove the kfree, then the structure will persist until the remove function
> is called, even though there was an error, which is perhaps not good. So I
> could change the kfree to devm_kfree?
I was under the impression that if you exit the probe function
with a negative value anything allocated with devm_* was freed
immediately, that is atleast how it's described in
Documentation/driver-model/devres.txt
atleast that seems to be the intetion with the whole thing.
So just delete the kfree() oneliner.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* question about drivers/pinctrl/pinctrl-at91.c
2012-12-11 8:51 ` Linus Walleij
@ 2012-12-11 9:04 ` Julia Lawall
2012-12-11 9:08 ` Linus Walleij
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2012-12-11 9:04 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 11 Dec 2012, Linus Walleij wrote:
> On Sat, Dec 8, 2012 at 4:52 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
> > The function at91_dt_node_to_map in drivers/pinctrl/pinctrl-at91.c contains
> > the following code:
> >
> > new_map = devm_kzalloc(pctldev->dev, sizeof(*new_map) * map_num,
> > GFP_KERNEL);
> > if (!new_map)
> > return -ENOMEM;
> >
> > *map = new_map;
> > *num_maps = map_num;
> >
> > /* create mux map */
> > parent = of_get_parent(np);
> > if (!parent) {
> > kfree(new_map);
> > return -EINVAL;
> > }
> >
> > This is clearly not correct, because the combination of devm_kzalloc and
> > kfree risks creating a double free.
>
> Agreed, probably just some spurious leftover.
>
> > But I am not sure how best to fix it.
> > Is the data structure intended to normally exist until the driver's remove
> > function is called? If so, perhaps the devm_kzalloc is OK. If I just
> > remove the kfree, then the structure will persist until the remove function
> > is called, even though there was an error, which is perhaps not good. So I
> > could change the kfree to devm_kfree?
>
> I was under the impression that if you exit the probe function
> with a negative value anything allocated with devm_* was freed
> immediately, that is atleast how it's described in
> Documentation/driver-model/devres.txt
> atleast that seems to be the intetion with the whole thing.
That is true, but I wasn't sure taht this function was part of the probe
function. Its only reference is in:
static struct pinctrl_ops at91_pctrl_ops = {
.get_groups_count = at91_get_groups_count,
.get_group_name = at91_get_group_name,
.get_group_pins = at91_get_group_pins,
.pin_dbg_show = at91_pin_dbg_show,
.dt_node_to_map = at91_dt_node_to_map,
.dt_free_map = at91_dt_free_map,
};
Working backwards, one possible call site is pinctrl_get, which is an
exported function. Is it safe to assume that it will always be called
from within a probe function?
thanks,
julia
^ permalink raw reply [flat|nested] 6+ messages in thread
* question about drivers/pinctrl/pinctrl-at91.c
2012-12-08 15:52 question about drivers/pinctrl/pinctrl-at91.c Julia Lawall
2012-12-11 8:51 ` Linus Walleij
@ 2012-12-11 9:06 ` Grant Likely
1 sibling, 0 replies; 6+ messages in thread
From: Grant Likely @ 2012-12-11 9:06 UTC (permalink / raw)
To: linux-arm-kernel
On Sat, 8 Dec 2012 16:52:42 +0100 (CET), Julia Lawall <julia.lawall@lip6.fr> wrote:
> The function at91_dt_node_to_map in drivers/pinctrl/pinctrl-at91.c
> contains the following code:
>
> new_map = devm_kzalloc(pctldev->dev, sizeof(*new_map) * map_num, GFP_KERNEL);
> if (!new_map)
> return -ENOMEM;
>
> *map = new_map;
> *num_maps = map_num;
>
> /* create mux map */
> parent = of_get_parent(np);
> if (!parent) {
> kfree(new_map);
> return -EINVAL;
> }
>
> This is clearly not correct, because the combination of devm_kzalloc and
> kfree risks creating a double free. But I am not sure how best to fix it.
> Is the data structure intended to normally exist until the driver's remove
> function is called? If so, perhaps the devm_kzalloc is OK. If I just
> remove the kfree, then the structure will persist until the remove
> function is called, even though there was an error, which is perhaps not
> good. So I could change the kfree to devm_kfree?
Hi Julia,
I don't have that file in my tree. Is it in linux-next? (I'm too lazy to
go and look)
Yes, devm_kfree() is the right thing to call, but I'd be tempted to just
drop the free entirely. If it fails there is something wrong and it will
get cleaned up when the probe returns a fail code... but look at the
code; if failing there is a valid way for the driver to operate, then
doing the cleanup is the right thing....
Alternately the of_get_parent() call could simply be moved above the
devm_kzalloc() and then the issue becomes moot. :-)
g.
^ permalink raw reply [flat|nested] 6+ messages in thread
* question about drivers/pinctrl/pinctrl-at91.c
2012-12-11 9:04 ` Julia Lawall
@ 2012-12-11 9:08 ` Linus Walleij
2012-12-11 9:59 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2012-12-11 9:08 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 11, 2012 at 10:04 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Tue, 11 Dec 2012, Linus Walleij wrote:
>> I was under the impression that if you exit the probe function
>> with a negative value anything allocated with devm_* was freed
>> immediately, that is atleast how it's described in
>> Documentation/driver-model/devres.txt
>> atleast that seems to be the intetion with the whole thing.
>
> That is true, but I wasn't sure taht this function was part of the probe
> function. Its only reference is in:
>
> static struct pinctrl_ops at91_pctrl_ops = {
> .get_groups_count = at91_get_groups_count,
> .get_group_name = at91_get_group_name,
> .get_group_pins = at91_get_group_pins,
> .pin_dbg_show = at91_pin_dbg_show,
> .dt_node_to_map = at91_dt_node_to_map,
> .dt_free_map = at91_dt_free_map,
> };
>
> Working backwards, one possible call site is pinctrl_get, which is an
> exported function. Is it safe to assume that it will always be called
> from within a probe function?
Aha sorry I got it all backwards :-(
Well, yes in that case it's devm_kfree() for sure.
Thanks!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* question about drivers/pinctrl/pinctrl-at91.c
2012-12-11 9:08 ` Linus Walleij
@ 2012-12-11 9:59 ` Julia Lawall
0 siblings, 0 replies; 6+ messages in thread
From: Julia Lawall @ 2012-12-11 9:59 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 11 Dec 2012, Linus Walleij wrote:
> On Tue, Dec 11, 2012 at 10:04 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Tue, 11 Dec 2012, Linus Walleij wrote:
>
> >> I was under the impression that if you exit the probe function
> >> with a negative value anything allocated with devm_* was freed
> >> immediately, that is atleast how it's described in
> >> Documentation/driver-model/devres.txt
> >> atleast that seems to be the intetion with the whole thing.
> >
> > That is true, but I wasn't sure taht this function was part of the probe
> > function. Its only reference is in:
> >
> > static struct pinctrl_ops at91_pctrl_ops = {
> > .get_groups_count = at91_get_groups_count,
> > .get_group_name = at91_get_group_name,
> > .get_group_pins = at91_get_group_pins,
> > .pin_dbg_show = at91_pin_dbg_show,
> > .dt_node_to_map = at91_dt_node_to_map,
> > .dt_free_map = at91_dt_free_map,
> > };
> >
> > Working backwards, one possible call site is pinctrl_get, which is an
> > exported function. Is it safe to assume that it will always be called
> > from within a probe function?
>
> Aha sorry I got it all backwards :-(
>
> Well, yes in that case it's devm_kfree() for sure.
I've sent a patch, thanks.
julia
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-11 9:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-08 15:52 question about drivers/pinctrl/pinctrl-at91.c Julia Lawall
2012-12-11 8:51 ` Linus Walleij
2012-12-11 9:04 ` Julia Lawall
2012-12-11 9:08 ` Linus Walleij
2012-12-11 9:59 ` Julia Lawall
2012-12-11 9:06 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).