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