ARM: cpuidle: Avoid memleak if init fail

PATCH New Stefan Wahren 3 participants 7 messages Aug 31, 2017 – Oct 3, 2017 CPU idle time management framework

Share & tools

Stefan Wahren

author

2017-08-31 20:24 UTC #0 lore raw

PATCH 1/1 ARM: cpuidle: Avoid memleak if init fail

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 <[email protected]>

Fixes d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")

---
 drivers/cpuidle/cpuidle-arm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-arm.c +4 -2

index 7080c38..52a7505 100644
@@ -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);

Signature · 1 line

2.7.4

Leo Yan

2017-09-01 01:33 UTC #1 lore raw

PATCH 1/1 ARM: cpuidle: Avoid memleak if init fail

Hi Stefan,

Quoted 44 lines · 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 <[email protected]>

Fixes d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")

---
 drivers/cpuidle/cpuidle-arm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-arm.c +2 -2

index 7080c38..52a7505 100644
@@ -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;
 		}
 
 		/*

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 153

Quoted 5 lines

drivers/cpuidle/cpuidle-arm.c +2 -0

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

Quoted 6 lines

drivers/cpuidle/cpuidle-arm.c +0 -0
 out_fail:
 	while (--cpu >= 0) {
 		dev = per_cpu(cpuidle_devices, cpu);
Signature · 1 line
2.7.4

Stefan Wahren

author

2017-09-01 05:49 UTC #2 lore raw

PATCH 1/1 ARM: cpuidle: Avoid memleak if init fail

Hi Leo,

Quoted 52 lines

Leo Yan <[email protected]> hat am 1. September 2017 um 03:33 geschrieben:

Hi Stefan,

Quoted 46 lines · 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 <[email protected]>

Fixes d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")

---
 drivers/cpuidle/cpuidle-arm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-arm.c +2 -2

index 7080c38..52a7505 100644
@@ -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)
 	}

Quoted 7 lines

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 153
Quoted 3 lines
drivers/cpuidle/cpuidle-arm.c +2 -0
 	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

Quoted 15 lines

Thanks,
Leo Yan
Quoted 6 lines
drivers/cpuidle/cpuidle-arm.c +0 -0
 out_fail:
 	while (--cpu >= 0) {
 		dev = per_cpu(cpuidle_devices, cpu);
Signature · 1 line
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Leo Yan

2017-09-01 06:56 UTC #3 lore raw

PATCH 1/1 ARM: cpuidle: Avoid memleak if init fail

Quoted 62 lines · Stefan Wahren

Hi Leo,
Quoted 52 lines
Leo Yan <[email protected]> hat am 1. September 2017 um 03:33 geschrieben:
Hi Stefan,
Quoted 46 lines · 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 <[email protected]>
Fixes d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---
 drivers/cpuidle/cpuidle-arm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/cpuidle/cpuidle-arm.c +2 -2
index 7080c38..52a7505 100644
@@ -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)
 	}
 
Quoted 7 lines
drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 153
Quoted 3 lines
drivers/cpuidle/cpuidle-arm.c +2 -0
 	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.

Quoted 1 line

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

Stefan Wahren

author

2017-09-01 07:08 UTC #4 lore raw

PATCH 1/1 ARM: cpuidle: Avoid memleak if init fail

Hi Leo,

Am 01.09.2017 um 08:56 schrieb Leo Yan:

Quoted 61 lines · Stefan Wahren

Quoted 60 lines
Hi Leo,
Quoted 58 lines
Leo Yan <[email protected]> hat am 1. September 2017 um 03:33 geschrieben:
Hi Stefan,
Quoted 44 lines · 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 <[email protected]>
Fixes d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---
  drivers/cpuidle/cpuidle-arm.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
drivers/cpuidle/cpuidle-arm.c +2 -2
index 7080c38..52a7505 100644
@@ -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;
  		}
 
  		/*
drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 153
Quoted 5 lines
drivers/cpuidle/cpuidle-arm.c +2 -0
@@ -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.

Quoted 7 lines

Quoted 1 line
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

Quoted 8 lines

Thanks,
Leo Yan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Leo Yan

2017-09-01 08:10 UTC #5 lore raw

PATCH 1/1 ARM: cpuidle: Avoid memleak if init fail

Quoted 64 lines · Stefan Wahren

Hi Leo,
Am 01.09.2017 um 08:56 schrieb Leo Yan:
Quoted 61 lines · Stefan Wahren
Quoted 57 lines
Hi Leo,
Quoted 47 lines
Leo Yan <[email protected]> hat am 1. September 2017 um 03:33 geschrieben:
Hi Stefan,
Quoted 41 lines · 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 <[email protected]>
Fixes d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---
 drivers/cpuidle/cpuidle-arm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/cpuidle/cpuidle-arm.c +2 -2
index 7080c38..52a7505 100644
@@ -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;
 		}
 		/*
Quoted 8 lines
drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 152
Quoted 5 lines
drivers/cpuidle/cpuidle-arm.c +2 -0
@@ -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.
Quoted 1 line
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.

Quoted 7 lines

Quoted 4 lines
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

Daniel Lezcano

2017-10-03 13:21 UTC #6 lore raw

PATCH 1/1 ARM: cpuidle: Avoid memleak if init fail

In reply to Stefan Wahren

Quoted 15 lines · 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

Quoted 5 lines

So fix this by freeing the unregistered copy in error case.
Signed-off-by Stefan Wahren <[email protected]>
Fixes d50a7d8acd78 ("ARM: cpuidle: Support asymmetric idle definition")
---

Acked-by Daniel Lezcano <[email protected]>

Signature · 5 lines

 <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