* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-08-31 20:24 ` Stefan Wahren
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2017-08-31 20:24 UTC (permalink / raw)
To: Daniel Lezcano, Leo Yan, Rafael J . Wysocki, Sudeep Holla
Cc: linux-pm, linux-arm-kernel, Stefan Wahren
In case there are no DT idle states defined or
cpuidle_register_driver() fails, the copy of the idle driver is leaked:
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
So fix this by freeing the unregistered copy in error case.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---
drivers/cpuidle/cpuidle-arm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 7080c38..52a7505 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
if (ret <= 0) {
ret = ret ? : -ENODEV;
- goto out_fail;
+ goto init_fail;
}
ret = cpuidle_register_driver(drv);
if (ret) {
pr_err("Failed to register cpuidle driver\n");
- goto out_fail;
+ goto init_fail;
}
/*
@@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
}
return 0;
+init_fail:
+ kfree(drv);
out_fail:
while (--cpu >= 0) {
dev = per_cpu(cpuidle_devices, cpu);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-08-31 20:24 ` Stefan Wahren
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2017-08-31 20:24 UTC (permalink / raw)
To: linux-arm-kernel
In case there are no DT idle states defined or
cpuidle_register_driver() fails, the copy of the idle driver is leaked:
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
So fix this by freeing the unregistered copy in error case.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---
drivers/cpuidle/cpuidle-arm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 7080c38..52a7505 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
if (ret <= 0) {
ret = ret ? : -ENODEV;
- goto out_fail;
+ goto init_fail;
}
ret = cpuidle_register_driver(drv);
if (ret) {
pr_err("Failed to register cpuidle driver\n");
- goto out_fail;
+ goto init_fail;
}
/*
@@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
}
return 0;
+init_fail:
+ kfree(drv);
out_fail:
while (--cpu >= 0) {
dev = per_cpu(cpuidle_devices, cpu);
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM: cpuidle: Avoid memleak if init fail
2017-08-31 20:24 ` Stefan Wahren
@ 2017-09-01 1:33 ` Leo Yan
-1 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2017-09-01 1:33 UTC (permalink / raw)
To: Stefan Wahren
Cc: Daniel Lezcano, Rafael J . Wysocki, Sudeep Holla, linux-pm,
linux-arm-kernel
Hi Stefan,
On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> In case there are no DT idle states defined or
> cpuidle_register_driver() fails, the copy of the idle driver is leaked:
>
> 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
>
> So fix this by freeing the unregistered copy in error case.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> ---
> drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 7080c38..52a7505 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> if (ret <= 0) {
> ret = ret ? : -ENODEV;
> - goto out_fail;
> + goto init_fail;
> }
>
> ret = cpuidle_register_driver(drv);
> if (ret) {
> pr_err("Failed to register cpuidle driver\n");
> - goto out_fail;
> + goto init_fail;
> }
>
> /*
> @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> }
>
> return 0;
> +init_fail:
> + kfree(drv);
The below loop only releases resource for previous CPUs, so should check
two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
it.
Thanks,
Leo Yan
> out_fail:
> while (--cpu >= 0) {
> dev = per_cpu(cpuidle_devices, cpu);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-09-01 1:33 ` Leo Yan
0 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2017-09-01 1:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi Stefan,
On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> In case there are no DT idle states defined or
> cpuidle_register_driver() fails, the copy of the idle driver is leaked:
>
> 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
>
> So fix this by freeing the unregistered copy in error case.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> ---
> drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> index 7080c38..52a7505 100644
> --- a/drivers/cpuidle/cpuidle-arm.c
> +++ b/drivers/cpuidle/cpuidle-arm.c
> @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> if (ret <= 0) {
> ret = ret ? : -ENODEV;
> - goto out_fail;
> + goto init_fail;
> }
>
> ret = cpuidle_register_driver(drv);
> if (ret) {
> pr_err("Failed to register cpuidle driver\n");
> - goto out_fail;
> + goto init_fail;
> }
>
> /*
> @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> }
>
> return 0;
> +init_fail:
> + kfree(drv);
The below loop only releases resource for previous CPUs, so should check
two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
it.
Thanks,
Leo Yan
> out_fail:
> while (--cpu >= 0) {
> dev = per_cpu(cpuidle_devices, cpu);
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM: cpuidle: Avoid memleak if init fail
2017-09-01 1:33 ` Leo Yan
@ 2017-09-01 5:49 ` Stefan Wahren
-1 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2017-09-01 5:49 UTC (permalink / raw)
To: Leo Yan
Cc: Rafael J . Wysocki, linux-pm, Daniel Lezcano, linux-arm-kernel,
Sudeep Holla
Hi Leo,
> Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
>
>
> Hi Stefan,
>
> On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> > In case there are no DT idle states defined or
> > cpuidle_register_driver() fails, the copy of the idle driver is leaked:
> >
> > 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
> >
> > So fix this by freeing the unregistered copy in error case.
> >
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> > ---
> > drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> > index 7080c38..52a7505 100644
> > --- a/drivers/cpuidle/cpuidle-arm.c
> > +++ b/drivers/cpuidle/cpuidle-arm.c
> > @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> > ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> > if (ret <= 0) {
> > ret = ret ? : -ENODEV;
> > - goto out_fail;
> > + goto init_fail;
> > }
> >
> > ret = cpuidle_register_driver(drv);
> > if (ret) {
> > pr_err("Failed to register cpuidle driver\n");
> > - goto out_fail;
> > + goto init_fail;
> > }
> >
> > /*
> > @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> > }
> >
> > return 0;
> > +init_fail:
> > + kfree(drv);
>
> The below loop only releases resource for previous CPUs, so should check
> two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
> it.
i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
Stefan
>
> Thanks,
> Leo Yan
>
> > out_fail:
> > while (--cpu >= 0) {
> > dev = per_cpu(cpuidle_devices, cpu);
> > --
> > 2.7.4
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-09-01 5:49 ` Stefan Wahren
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2017-09-01 5:49 UTC (permalink / raw)
To: linux-arm-kernel
Hi Leo,
> Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
>
>
> Hi Stefan,
>
> On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> > In case there are no DT idle states defined or
> > cpuidle_register_driver() fails, the copy of the idle driver is leaked:
> >
> > 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
> >
> > So fix this by freeing the unregistered copy in error case.
> >
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> > ---
> > drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> > index 7080c38..52a7505 100644
> > --- a/drivers/cpuidle/cpuidle-arm.c
> > +++ b/drivers/cpuidle/cpuidle-arm.c
> > @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> > ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> > if (ret <= 0) {
> > ret = ret ? : -ENODEV;
> > - goto out_fail;
> > + goto init_fail;
> > }
> >
> > ret = cpuidle_register_driver(drv);
> > if (ret) {
> > pr_err("Failed to register cpuidle driver\n");
> > - goto out_fail;
> > + goto init_fail;
> > }
> >
> > /*
> > @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> > }
> >
> > return 0;
> > +init_fail:
> > + kfree(drv);
>
> The below loop only releases resource for previous CPUs, so should check
> two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
> it.
i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
Stefan
>
> Thanks,
> Leo Yan
>
> > out_fail:
> > while (--cpu >= 0) {
> > dev = per_cpu(cpuidle_devices, cpu);
> > --
> > 2.7.4
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM: cpuidle: Avoid memleak if init fail
2017-09-01 5:49 ` Stefan Wahren
@ 2017-09-01 6:56 ` Leo Yan
-1 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2017-09-01 6:56 UTC (permalink / raw)
To: Stefan Wahren
Cc: Rafael J . Wysocki, linux-pm, Daniel Lezcano, linux-arm-kernel,
Sudeep Holla
On Fri, Sep 01, 2017 at 07:49:29AM +0200, Stefan Wahren wrote:
> Hi Leo,
>
> > Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
> >
> >
> > Hi Stefan,
> >
> > On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> > > In case there are no DT idle states defined or
> > > cpuidle_register_driver() fails, the copy of the idle driver is leaked:
> > >
> > > 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
> > >
> > > So fix this by freeing the unregistered copy in error case.
> > >
> > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> > > ---
> > > drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> > > index 7080c38..52a7505 100644
> > > --- a/drivers/cpuidle/cpuidle-arm.c
> > > +++ b/drivers/cpuidle/cpuidle-arm.c
> > > @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> > > ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> > > if (ret <= 0) {
> > > ret = ret ? : -ENODEV;
> > > - goto out_fail;
> > > + goto init_fail;
> > > }
> > >
> > > ret = cpuidle_register_driver(drv);
> > > if (ret) {
> > > pr_err("Failed to register cpuidle driver\n");
> > > - goto out_fail;
> > > + goto init_fail;
> > > }
> > >
> > > /*
> > > @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> > > }
> > >
> > > return 0;
> > > +init_fail:
> > > + kfree(drv);
> >
> > The below loop only releases resource for previous CPUs, so should check
> > two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
> > it.
>
> i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
You are right, so please ignore my comment.
BTW, to avoid missing anything, are you working on the second
fixing to correct the previous CPU resourcee releasing? Actually
I think you are welcome to work on the second fixing, but if
not I will take the second fixing with rebasing your patch.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-09-01 6:56 ` Leo Yan
0 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2017-09-01 6:56 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 01, 2017 at 07:49:29AM +0200, Stefan Wahren wrote:
> Hi Leo,
>
> > Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
> >
> >
> > Hi Stefan,
> >
> > On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> > > In case there are no DT idle states defined or
> > > cpuidle_register_driver() fails, the copy of the idle driver is leaked:
> > >
> > > 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
> > >
> > > So fix this by freeing the unregistered copy in error case.
> > >
> > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> > > ---
> > > drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> > > index 7080c38..52a7505 100644
> > > --- a/drivers/cpuidle/cpuidle-arm.c
> > > +++ b/drivers/cpuidle/cpuidle-arm.c
> > > @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> > > ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> > > if (ret <= 0) {
> > > ret = ret ? : -ENODEV;
> > > - goto out_fail;
> > > + goto init_fail;
> > > }
> > >
> > > ret = cpuidle_register_driver(drv);
> > > if (ret) {
> > > pr_err("Failed to register cpuidle driver\n");
> > > - goto out_fail;
> > > + goto init_fail;
> > > }
> > >
> > > /*
> > > @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> > > }
> > >
> > > return 0;
> > > +init_fail:
> > > + kfree(drv);
> >
> > The below loop only releases resource for previous CPUs, so should check
> > two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
> > it.
>
> i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
You are right, so please ignore my comment.
BTW, to avoid missing anything, are you working on the second
fixing to correct the previous CPU resourcee releasing? Actually
I think you are welcome to work on the second fixing, but if
not I will take the second fixing with rebasing your patch.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM: cpuidle: Avoid memleak if init fail
2017-09-01 6:56 ` Leo Yan
@ 2017-09-01 7:08 ` Stefan Wahren
-1 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2017-09-01 7:08 UTC (permalink / raw)
To: Leo Yan
Cc: Sudeep Holla, Rafael J . Wysocki, Daniel Lezcano,
linux-arm-kernel, linux-pm
Hi Leo,
Am 01.09.2017 um 08:56 schrieb Leo Yan:
> On Fri, Sep 01, 2017 at 07:49:29AM +0200, Stefan Wahren wrote:
>> Hi Leo,
>>
>>> Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
>>>
>>>
>>> Hi Stefan,
>>>
>>> On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
>>>> In case there are no DT idle states defined or
>>>> cpuidle_register_driver() fails, the copy of the idle driver is leaked:
>>>>
>>>> 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
>>>>
>>>> So fix this by freeing the unregistered copy in error case.
>>>>
>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
>>>> ---
>>>> drivers/cpuidle/cpuidle-arm.c | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
>>>> index 7080c38..52a7505 100644
>>>> --- a/drivers/cpuidle/cpuidle-arm.c
>>>> +++ b/drivers/cpuidle/cpuidle-arm.c
>>>> @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
>>>> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
>>>> if (ret <= 0) {
>>>> ret = ret ? : -ENODEV;
>>>> - goto out_fail;
>>>> + goto init_fail;
>>>> }
>>>>
>>>> ret = cpuidle_register_driver(drv);
>>>> if (ret) {
>>>> pr_err("Failed to register cpuidle driver\n");
>>>> - goto out_fail;
>>>> + goto init_fail;
>>>> }
>>>>
>>>> /*
>>>> @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
>>>> }
>>>>
>>>> return 0;
>>>> +init_fail:
>>>> + kfree(drv);
>>> The below loop only releases resource for previous CPUs, so should check
>>> two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
>>> it.
>> i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
> You are right, so please ignore my comment.
>
> BTW, to avoid missing anything, are you working on the second
> fixing to correct the previous CPU resourcee releasing? Actually
> I think you are welcome to work on the second fixing, but if
> not I will take the second fixing with rebasing your patch.
i'm busy with other issues and the RPi doesn't have DT idle states so i
would be grateful if you could take care of this.
Thanks
Stefan
>
> Thanks,
> Leo Yan
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-09-01 7:08 ` Stefan Wahren
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Wahren @ 2017-09-01 7:08 UTC (permalink / raw)
To: linux-arm-kernel
Hi Leo,
Am 01.09.2017 um 08:56 schrieb Leo Yan:
> On Fri, Sep 01, 2017 at 07:49:29AM +0200, Stefan Wahren wrote:
>> Hi Leo,
>>
>>> Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
>>>
>>>
>>> Hi Stefan,
>>>
>>> On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
>>>> In case there are no DT idle states defined or
>>>> cpuidle_register_driver() fails, the copy of the idle driver is leaked:
>>>>
>>>> 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
>>>>
>>>> So fix this by freeing the unregistered copy in error case.
>>>>
>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
>>>> ---
>>>> drivers/cpuidle/cpuidle-arm.c | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
>>>> index 7080c38..52a7505 100644
>>>> --- a/drivers/cpuidle/cpuidle-arm.c
>>>> +++ b/drivers/cpuidle/cpuidle-arm.c
>>>> @@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
>>>> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
>>>> if (ret <= 0) {
>>>> ret = ret ? : -ENODEV;
>>>> - goto out_fail;
>>>> + goto init_fail;
>>>> }
>>>>
>>>> ret = cpuidle_register_driver(drv);
>>>> if (ret) {
>>>> pr_err("Failed to register cpuidle driver\n");
>>>> - goto out_fail;
>>>> + goto init_fail;
>>>> }
>>>>
>>>> /*
>>>> @@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
>>>> }
>>>>
>>>> return 0;
>>>> +init_fail:
>>>> + kfree(drv);
>>> The below loop only releases resource for previous CPUs, so should check
>>> two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
>>> it.
>> i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
> You are right, so please ignore my comment.
>
> BTW, to avoid missing anything, are you working on the second
> fixing to correct the previous CPU resourcee releasing? Actually
> I think you are welcome to work on the second fixing, but if
> not I will take the second fixing with rebasing your patch.
i'm busy with other issues and the RPi doesn't have DT idle states so i
would be grateful if you could take care of this.
Thanks
Stefan
>
> Thanks,
> Leo Yan
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM: cpuidle: Avoid memleak if init fail
2017-09-01 7:08 ` Stefan Wahren
@ 2017-09-01 8:10 ` Leo Yan
-1 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2017-09-01 8:10 UTC (permalink / raw)
To: Stefan Wahren
Cc: Sudeep Holla, Rafael J . Wysocki, Daniel Lezcano,
linux-arm-kernel, linux-pm
On Fri, Sep 01, 2017 at 09:08:59AM +0200, Stefan Wahren wrote:
> Hi Leo,
>
> Am 01.09.2017 um 08:56 schrieb Leo Yan:
> >On Fri, Sep 01, 2017 at 07:49:29AM +0200, Stefan Wahren wrote:
> >>Hi Leo,
> >>
> >>>Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
> >>>
> >>>
> >>>Hi Stefan,
> >>>
> >>>On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> >>>>In case there are no DT idle states defined or
> >>>>cpuidle_register_driver() fails, the copy of the idle driver is leaked:
> >>>>
> >>>> 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
> >>>>
> >>>>So fix this by freeing the unregistered copy in error case.
> >>>>
> >>>>Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >>>>Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> >>>>---
> >>>> drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> >>>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> >>>>index 7080c38..52a7505 100644
> >>>>--- a/drivers/cpuidle/cpuidle-arm.c
> >>>>+++ b/drivers/cpuidle/cpuidle-arm.c
> >>>>@@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> >>>> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> >>>> if (ret <= 0) {
> >>>> ret = ret ? : -ENODEV;
> >>>>- goto out_fail;
> >>>>+ goto init_fail;
> >>>> }
> >>>> ret = cpuidle_register_driver(drv);
> >>>> if (ret) {
> >>>> pr_err("Failed to register cpuidle driver\n");
> >>>>- goto out_fail;
> >>>>+ goto init_fail;
> >>>> }
> >>>> /*
> >>>>@@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> >>>> }
> >>>> return 0;
> >>>>+init_fail:
> >>>>+ kfree(drv);
> >>>The below loop only releases resource for previous CPUs, so should check
> >>>two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
> >>>it.
> >>i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
> >You are right, so please ignore my comment.
> >
> >BTW, to avoid missing anything, are you working on the second
> >fixing to correct the previous CPU resourcee releasing? Actually
> >I think you are welcome to work on the second fixing, but if
> >not I will take the second fixing with rebasing your patch.
>
> i'm busy with other issues and the RPi doesn't have DT idle states
> so i would be grateful if you could take care of this.
Got it, sure, will work out the patch for reviewing.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-09-01 8:10 ` Leo Yan
0 siblings, 0 replies; 14+ messages in thread
From: Leo Yan @ 2017-09-01 8:10 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Sep 01, 2017 at 09:08:59AM +0200, Stefan Wahren wrote:
> Hi Leo,
>
> Am 01.09.2017 um 08:56 schrieb Leo Yan:
> >On Fri, Sep 01, 2017 at 07:49:29AM +0200, Stefan Wahren wrote:
> >>Hi Leo,
> >>
> >>>Leo Yan <leo.yan@linaro.org> hat am 1. September 2017 um 03:33 geschrieben:
> >>>
> >>>
> >>>Hi Stefan,
> >>>
> >>>On Thu, Aug 31, 2017 at 10:24:36PM +0200, Stefan Wahren wrote:
> >>>>In case there are no DT idle states defined or
> >>>>cpuidle_register_driver() fails, the copy of the idle driver is leaked:
> >>>>
> >>>> 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
> >>>>
> >>>>So fix this by freeing the unregistered copy in error case.
> >>>>
> >>>>Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >>>>Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> >>>>---
> >>>> drivers/cpuidle/cpuidle-arm.c | 6 ++++--
> >>>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
> >>>>index 7080c38..52a7505 100644
> >>>>--- a/drivers/cpuidle/cpuidle-arm.c
> >>>>+++ b/drivers/cpuidle/cpuidle-arm.c
> >>>>@@ -104,13 +104,13 @@ static int __init arm_idle_init(void)
> >>>> ret = dt_init_idle_driver(drv, arm_idle_state_match, 1);
> >>>> if (ret <= 0) {
> >>>> ret = ret ? : -ENODEV;
> >>>>- goto out_fail;
> >>>>+ goto init_fail;
> >>>> }
> >>>> ret = cpuidle_register_driver(drv);
> >>>> if (ret) {
> >>>> pr_err("Failed to register cpuidle driver\n");
> >>>>- goto out_fail;
> >>>>+ goto init_fail;
> >>>> }
> >>>> /*
> >>>>@@ -149,6 +149,8 @@ static int __init arm_idle_init(void)
> >>>> }
> >>>> return 0;
> >>>>+init_fail:
> >>>>+ kfree(drv);
> >>>The below loop only releases resource for previous CPUs, so should check
> >>>two variables 'drv' and 'dev'. If 'dev != NULL', we also need to release
> >>>it.
> >>i cannot see a leak for 'dev', because this is already handled in the error case of cpuidle_register_device before jumping to out_fail. I agree this isn't consistent, but this is a fix which should go to stable. So only necessary changes.
> >You are right, so please ignore my comment.
> >
> >BTW, to avoid missing anything, are you working on the second
> >fixing to correct the previous CPU resourcee releasing? Actually
> >I think you are welcome to work on the second fixing, but if
> >not I will take the second fixing with rebasing your patch.
>
> i'm busy with other issues and the RPi doesn't have DT idle states
> so i would be grateful if you could take care of this.
Got it, sure, will work out the patch for reviewing.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM: cpuidle: Avoid memleak if init fail
2017-08-31 20:24 ` Stefan Wahren
@ 2017-10-03 13:21 ` Daniel Lezcano
-1 siblings, 0 replies; 14+ messages in thread
From: Daniel Lezcano @ 2017-10-03 13:21 UTC (permalink / raw)
To: Stefan Wahren, Leo Yan, Rafael J . Wysocki, Sudeep Holla
Cc: linux-arm-kernel, linux-pm
On 31/08/2017 22:24, Stefan Wahren wrote:
> In case there are no DT idle states defined or
> cpuidle_register_driver() fails, the copy of the idle driver is leaked:
>
> 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
>
> So fix this by freeing the unregistered copy in error case.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> ---
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
--
<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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM: cpuidle: Avoid memleak if init fail
@ 2017-10-03 13:21 ` Daniel Lezcano
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Lezcano @ 2017-10-03 13:21 UTC (permalink / raw)
To: linux-arm-kernel
On 31/08/2017 22:24, Stefan Wahren wrote:
> In case there are no DT idle states defined or
> cpuidle_register_driver() fails, the copy of the idle driver is leaked:
>
> 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
>
> So fix this by freeing the unregistered copy in error case.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> Fixes: d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
> ---
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
--
<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] 14+ messages in thread
end of thread, other threads:[~2017-10-03 13:21 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-31 20:24 [PATCH] ARM: cpuidle: Avoid memleak if init fail Stefan Wahren
2017-08-31 20:24 ` Stefan Wahren
2017-09-01 1:33 ` Leo Yan
2017-09-01 1:33 ` Leo Yan
2017-09-01 5:49 ` Stefan Wahren
2017-09-01 5:49 ` Stefan Wahren
2017-09-01 6:56 ` Leo Yan
2017-09-01 6:56 ` Leo Yan
2017-09-01 7:08 ` Stefan Wahren
2017-09-01 7:08 ` Stefan Wahren
2017-09-01 8:10 ` Leo Yan
2017-09-01 8:10 ` Leo Yan
2017-10-03 13:21 ` Daniel Lezcano
2017-10-03 13:21 ` Daniel Lezcano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.