* [Bug] ARM: cpuidle: possible memleak
@ 2017-08-12 13:19 Stefan Wahren
2017-08-13 3:50 ` Leo Yan
0 siblings, 1 reply; 5+ messages in thread
From: Stefan Wahren @ 2017-08-12 13:19 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
if i additionally enable kmemleak (on top of multi_v7_defconfig) on RPi 3 (4 cores) with 4.13-rc4, i get the following output from kmemleak:
unreferenced object 0xede0dc00 (size 1024):
comm "swapper/0", pid 1, jiffies 4294937431 (age 744.510s)
hex dump (first 32 bytes):
94 9e 0b c1 00 00 00 00 00 00 00 00 00 00 00 00 ................
57 46 49 00 00 00 00 00 00 00 00 00 00 00 00 00 WFI.............
backtrace:
[<c1295f04>] arm_idle_init+0x44/0x1ac
[<c0301e6c>] do_one_initcall+0x3c/0x16c
[<c1200d70>] kernel_init_freeable+0x110/0x1d0
[<c0cb3624>] kernel_init+0x8/0x114
[<c0307a98>] ret_from_fork+0x14/0x3c
[<ffffffff>] 0xffffffff
If i revert the last commit in cpuidle-arm d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition") then kmemleak stays calm.
Best regards
Stefan
^ permalink raw reply [flat|nested] 5+ messages in thread* [Bug] ARM: cpuidle: possible memleak 2017-08-12 13:19 [Bug] ARM: cpuidle: possible memleak Stefan Wahren @ 2017-08-13 3:50 ` Leo Yan 2017-08-13 10:10 ` Stefan Wahren 0 siblings, 1 reply; 5+ messages in thread From: Leo Yan @ 2017-08-13 3:50 UTC (permalink / raw) To: linux-arm-kernel Hi Stefan, On Sat, Aug 12, 2017 at 03:19:33PM +0200, Stefan Wahren wrote: > Hi, > > if i additionally enable kmemleak (on top of multi_v7_defconfig) on RPi 3 (4 cores) with 4.13-rc4, i get the following output from kmemleak: > > unreferenced object 0xede0dc00 (size 1024): > comm "swapper/0", pid 1, jiffies 4294937431 (age 744.510s) > hex dump (first 32 bytes): > 94 9e 0b c1 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 57 46 49 00 00 00 00 00 00 00 00 00 00 00 00 00 WFI............. > backtrace: > [<c1295f04>] arm_idle_init+0x44/0x1ac > [<c0301e6c>] do_one_initcall+0x3c/0x16c > [<c1200d70>] kernel_init_freeable+0x110/0x1d0 > [<c0cb3624>] kernel_init+0x8/0x114 > [<c0307a98>] ret_from_fork+0x14/0x3c > [<ffffffff>] 0xffffffff > > If i revert the last commit in cpuidle-arm d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition") then kmemleak stays calm. I cannot reproduce the error at my side, I also tried to disable idle states but cannot trigger the failure. So first thing is to know the reason for registeration failure and finally introduce the memory leaking. Could you point out the dts you are using for idle states binding? Also reviewed a bit for the idle driver, it does have issue to handle failure case. I tweaked a bit for it, could you try it? Also would like to hear the comments from Daniel. diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c index 7080c38..80dc86f 100644 --- a/drivers/cpuidle/cpuidle-arm.c +++ b/drivers/cpuidle/cpuidle-arm.c @@ -152,11 +152,13 @@ static int __init arm_idle_init(void) out_fail: while (--cpu >= 0) { dev = per_cpu(cpuidle_devices, cpu); - cpuidle_unregister_device(dev); - kfree(dev); - drv = cpuidle_get_driver(); + drv = cpuidle_get_cpu_driver(dev); + cpuidle_unregister_driver(drv); kfree(drv); + + cpuidle_unregister_device(dev); + kfree(dev); } return ret; > > Best regards > Stefan ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Bug] ARM: cpuidle: possible memleak 2017-08-13 3:50 ` Leo Yan @ 2017-08-13 10:10 ` Stefan Wahren 2017-08-26 11:46 ` Stefan Wahren 0 siblings, 1 reply; 5+ messages in thread From: Stefan Wahren @ 2017-08-13 10:10 UTC (permalink / raw) To: linux-arm-kernel Hi Leo, > Leo Yan <leo.yan@linaro.org> hat am 13. August 2017 um 05:50 geschrieben: > > > Hi Stefan, > > On Sat, Aug 12, 2017 at 03:19:33PM +0200, Stefan Wahren wrote: > > Hi, > > > > if i additionally enable kmemleak (on top of multi_v7_defconfig) on RPi 3 (4 cores) with 4.13-rc4, i get the following output from kmemleak: > > > > unreferenced object 0xede0dc00 (size 1024): > > comm "swapper/0", pid 1, jiffies 4294937431 (age 744.510s) > > hex dump (first 32 bytes): > > 94 9e 0b c1 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > 57 46 49 00 00 00 00 00 00 00 00 00 00 00 00 00 WFI............. > > backtrace: > > [<c1295f04>] arm_idle_init+0x44/0x1ac > > [<c0301e6c>] do_one_initcall+0x3c/0x16c > > [<c1200d70>] kernel_init_freeable+0x110/0x1d0 > > [<c0cb3624>] kernel_init+0x8/0x114 > > [<c0307a98>] ret_from_fork+0x14/0x3c > > [<ffffffff>] 0xffffffff > > > > If i revert the last commit in cpuidle-arm d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition") then kmemleak stays calm. > > I cannot reproduce the error at my side, I also tried to disable idle > states but cannot trigger the failure. So first thing is to know the > reason for registeration failure and finally introduce the memory > leaking. Could you point out the dts you are using for idle states > binding? thanks for you quick response. Sorry, i forgot to mention that arch/arm64/boot/dts/broadcom/bcm2837.dtsi doesn't contain any idle states. So the error path of dt_init_idle_driver() is expected. If i get it right, the clean up loop after out_fail only handles registered drivers (better label name or a comment would be helpful). So combined with your fix the following fixes the memleak for me (didn't test the clean up loop): diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c index 7080c38..827ab25 100644 --- a/drivers/cpuidle/cpuidle-arm.c +++ b/drivers/cpuidle/cpuidle-arm.c @@ -104,12 +104,14 @@ static int __init arm_idle_init(void) ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); if (ret <= 0) { ret = ret ? : -ENODEV; + kfree(drv); goto out_fail; } ret = cpuidle_register_driver(drv); if (ret) { pr_err("Failed to register cpuidle driver\n"); + kfree(drv); goto out_fail; } @@ -152,11 +154,13 @@ static int __init arm_idle_init(void) out_fail: while (--cpu >= 0) { dev = per_cpu(cpuidle_devices, cpu); - cpuidle_unregister_device(dev); - kfree(dev); - drv = cpuidle_get_driver(); + drv = cpuidle_get_cpu_driver(dev); + cpuidle_unregister_driver(drv); kfree(drv); + + cpuidle_unregister_device(dev); + kfree(dev); } return ret; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Bug] ARM: cpuidle: possible memleak 2017-08-13 10:10 ` Stefan Wahren @ 2017-08-26 11:46 ` Stefan Wahren 2017-08-29 9:04 ` Daniel Lezcano 0 siblings, 1 reply; 5+ messages in thread From: Stefan Wahren @ 2017-08-26 11:46 UTC (permalink / raw) To: linux-arm-kernel Hi, > Stefan Wahren <stefan.wahren@i2se.com> hat am 13. August 2017 um 12:10 geschrieben: > > > Hi Leo, > > > Leo Yan <leo.yan@linaro.org> hat am 13. August 2017 um 05:50 geschrieben: > > > > > > Hi Stefan, > > > > On Sat, Aug 12, 2017 at 03:19:33PM +0200, Stefan Wahren wrote: > > > Hi, > > > > > > if i additionally enable kmemleak (on top of multi_v7_defconfig) on RPi 3 (4 cores) with 4.13-rc4, i get the following output from kmemleak: > > > > > > unreferenced object 0xede0dc00 (size 1024): > > > comm "swapper/0", pid 1, jiffies 4294937431 (age 744.510s) > > > hex dump (first 32 bytes): > > > 94 9e 0b c1 00 00 00 00 00 00 00 00 00 00 00 00 ................ > > > 57 46 49 00 00 00 00 00 00 00 00 00 00 00 00 00 WFI............. > > > backtrace: > > > [<c1295f04>] arm_idle_init+0x44/0x1ac > > > [<c0301e6c>] do_one_initcall+0x3c/0x16c > > > [<c1200d70>] kernel_init_freeable+0x110/0x1d0 > > > [<c0cb3624>] kernel_init+0x8/0x114 > > > [<c0307a98>] ret_from_fork+0x14/0x3c > > > [<ffffffff>] 0xffffffff > > > > > > If i revert the last commit in cpuidle-arm d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition") then kmemleak stays calm. > > > > I cannot reproduce the error at my side, I also tried to disable idle > > states but cannot trigger the failure. So first thing is to know the > > reason for registeration failure and finally introduce the memory > > leaking. Could you point out the dts you are using for idle states > > binding? > > thanks for you quick response. Sorry, i forgot to mention that arch/arm64/boot/dts/broadcom/bcm2837.dtsi doesn't contain any idle states. So the error path of dt_init_idle_driver() is expected. If i get it right, the clean up loop after out_fail only handles registered drivers (better label name or a comment would be helpful). So combined with your fix the following fixes the memleak for me (didn't test the clean up loop): > > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c > index 7080c38..827ab25 100644 > --- a/drivers/cpuidle/cpuidle-arm.c > +++ b/drivers/cpuidle/cpuidle-arm.c > @@ -104,12 +104,14 @@ static int __init arm_idle_init(void) > ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); > if (ret <= 0) { > ret = ret ? : -ENODEV; > + kfree(drv); > goto out_fail; > } > > ret = cpuidle_register_driver(drv); > if (ret) { > pr_err("Failed to register cpuidle driver\n"); > + kfree(drv); > goto out_fail; > } > > @@ -152,11 +154,13 @@ static int __init arm_idle_init(void) > out_fail: > while (--cpu >= 0) { > dev = per_cpu(cpuidle_devices, cpu); > - cpuidle_unregister_device(dev); > - kfree(dev); > - drv = cpuidle_get_driver(); > + drv = cpuidle_get_cpu_driver(dev); > + > cpuidle_unregister_driver(drv); > kfree(drv); > + > + cpuidle_unregister_device(dev); > + kfree(dev); > } > > return ret; should i split this patch (fix for "my" memleak and cleanup fix)? ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bug] ARM: cpuidle: possible memleak 2017-08-26 11:46 ` Stefan Wahren @ 2017-08-29 9:04 ` Daniel Lezcano 0 siblings, 0 replies; 5+ messages in thread From: Daniel Lezcano @ 2017-08-29 9:04 UTC (permalink / raw) To: linux-arm-kernel On 26/08/2017 13:46, Stefan Wahren wrote: [ ... ] >> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c >> index 7080c38..827ab25 100644 >> --- a/drivers/cpuidle/cpuidle-arm.c >> +++ b/drivers/cpuidle/cpuidle-arm.c >> @@ -104,12 +104,14 @@ static int __init arm_idle_init(void) >> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1); >> if (ret <= 0) { >> ret = ret ? : -ENODEV; >> + kfree(drv); >> goto out_fail; >> } >> >> ret = cpuidle_register_driver(drv); >> if (ret) { >> pr_err("Failed to register cpuidle driver\n"); >> + kfree(drv); >> goto out_fail; >> } I would prefer if you create another label on top of out_fail to free 'drv' and 'dev'. So you can remove the kfree in this loop (also take the opportunity to remove the pr_err("Failed to allocate cpuidle device") message. > should i split this patch (fix for "my" memleak and cleanup fix)? Yes, please. Thanks. -- Daniel -- <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-29 9:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-12 13:19 [Bug] ARM: cpuidle: possible memleak Stefan Wahren 2017-08-13 3:50 ` Leo Yan 2017-08-13 10:10 ` Stefan Wahren 2017-08-26 11:46 ` Stefan Wahren 2017-08-29 9:04 ` Daniel Lezcano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox