ARM: EXYNOS: pd: fix resource deallocation on error path
Subsystems (3)
PATCH 1/1 ARM: EXYNOS: pd: fix resource deallocation on error path
The change fixes a bug introduced by 2be2a3ff42a5, memory allocated by kstrdup_const() must be always deallocated with kfree_const(), otherwise there is a risk of kfree'ing ro memory.
Also remove unneeded of_node_put(), if for_each_compatible_node() body execution is not terminated, this prevents from double kfree() in OF_DYNAMIC build.
--- arch/arm/mach-exynos/pm_domains.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/arm/mach-exynos/pm_domains.c +1 -2
@@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void) pd->base =of_iomap (np,0 );if (!pd->base) {pr_warn ("%s: failed to map memory\n" , __func__); -kfree (pd->pd.name); +kfree_const (pd->pd.name);kfree (pd); -of_node_put (np);continue ; }
Signature · 1 line
2.1.4
PATCH 1/1 ARM: EXYNOS: pd: fix resource deallocation on error path
2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <[email protected]>:
Quoted 3 lines
The change fixes a bug introduced by
2be2a3ff42a5, memory allocated bykstrdup_const()must be always deallocated withkfree_const(), otherwise there is a risk of kfree'ing ro memory.
This looks good. Can you provide also Cc-stable and fixes tags?
Quoted 4 lines
Also remove unneeded
of_node_put(), iffor_each_compatible_node()body execution is not terminated, this prevents from doublekfree()inOF_DYNAMICbuild.
Each iteration of for_each_compatible_node() has a check:
(dn = of_find_compatible_node(dn, type, compatible))
this increases the references to 'np'. If loop continues then previous 'np' is not of_node_put().
Best regards,
Krzysztof
Quoted 29 lines
Signed-off-by Vladimir Zapolskiy <[email protected]>--- arch/arm/mach-exynos/pm_domains.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)arch/arm/mach-exynos/pm_domains.c +1
-3@@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void) pd->base =of_iomap (np,0 );if (!pd->base) {pr_warn ("%s: failed to map memory\n" , __func__); -kfree (pd->pd.name); +kfree_const (pd->pd.name);kfree (pd); -of_node_put (np);continue ; } --2.1.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 ARM: EXYNOS: pd: fix resource deallocation on error path
Quoted 6 lines · Krzysztof Kozlowski
2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <[email protected]>:
Quoted 3 lines
The change fixes a bug introduced by
2be2a3ff42a5, memory allocated bykstrdup_const()must be always deallocated withkfree_const(), otherwise there is a risk of kfree'ing ro memory.This looks good. Can you provide also Cc-stable and fixes tags?
Since the change fixes two independent issues I decided not to add a particular commit to Fixes tag. I can split the commit of course, but I feel reluctant to send a series in this particular case.
Let me know your decision with respect to my comments.
Quoted 8 lines
Quoted 4 lines
Also remove unneeded
of_node_put(), iffor_each_compatible_node()body execution is not terminated, this prevents from doublekfree()inOF_DYNAMICbuild.Each iteration of
for_each_compatible_node()has a check:
(dn =of_find_compatible_node(dn, type, compatible)) this increases the references to 'np'.
Correct.
Quoted 1 line
If loop continues then previous 'np' is not
of_node_put().
This I don't understand. The previous 'np' is of_node_put() on next iteration of the loop, i.e. if and only if loop continues. Please elaborate.
--
With best wishes,
Vladimir
Quoted 30 lines
Quoted 30 lines
Signed-off-by Vladimir Zapolskiy <[email protected]>--- arch/arm/mach-exynos/pm_domains.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)arch/arm/mach-exynos/pm_domains.c +1
-3@@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void) pd->base =of_iomap (np,0 );if (!pd->base) {pr_warn ("%s: failed to map memory\n" , __func__); -kfree (pd->pd.name); +kfree_const (pd->pd.name);kfree (pd); -of_node_put (np);continue ; } --2.1.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 ARM: EXYNOS: pd: fix resource deallocation on error path
Quoted 6 lines · Vladimir Zapolskiy
Quoted 5 lines · Krzysztof Kozlowski
2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <[email protected]>:
Quoted 3 lines
The change fixes a bug introduced by
2be2a3ff42a5, memory allocated bykstrdup_const()must be always deallocated withkfree_const(), otherwise there is a risk of kfree'ing ro memory.
Quoted 7 lines
Quoted 1 line
This looks good. Can you provide also Cc-stable and fixes tags?
Since the change fixes two independent issues I decided not to add a particular commit to Fixes tag. I can split the commit of course, but I feel reluctant to send a series in this particular case.
Let me know your decision with respect to my comments.
Although this is only error-path but still this applies for backporting to stable. Please split it up and add respective fixes tags. This helps companies/people using stable trees, including LTS.
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Also remove unneeded
of_node_put(), iffor_each_compatible_node()body execution is not terminated, this prevents from doublekfree()inOF_DYNAMICbuild.Each iteration of
for_each_compatible_node()has a check:
(dn =of_find_compatible_node(dn, type, compatible)) this increases the references to 'np'.
Quoted 6 lines
Correct.
Quoted 1 line
If loop continues then previous 'np' is not
of_node_put().This I don't understand. The previous 'np' is
of_node_put()on next iteration of the loop, i.e. if and only if loop continues. Please elaborate.
Step by step, if I get it right:
1. initialization: dn = of_find_compatible_node(NULL, type, compatible); 1a. if (!pd->base) then we want to drop that reference.
1b. if not, then loop itself
3. increase value: dn = of_find_compatible_node(dn, type, compatible)
4. next iteration of loop, now we have 'dn' from last 'increase value'
5. if (!pd->base) then we want to drop that reference.
Best regards,
Krzysztof
Quoted 36 lines
--
With best wishes,
VladimirQuoted 30 lines
Quoted 30 lines
Signed-off-by Vladimir Zapolskiy <[email protected]>--- arch/arm/mach-exynos/pm_domains.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)arch/arm/mach-exynos/pm_domains.c +1
-3@@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void) pd->base =of_iomap (np,0 );if (!pd->base) {pr_warn ("%s: failed to map memory\n" , __func__); -kfree (pd->pd.name); +kfree_const (pd->pd.name);kfree (pd); -of_node_put (np);continue ; } --2.1.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 ARM: EXYNOS: pd: fix resource deallocation on error path
Quoted 13 lines · Krzysztof Kozlowski
Quoted 12 lines · Vladimir Zapolskiy
Quoted 6 lines · Krzysztof Kozlowski
2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <[email protected]>:
Quoted 3 lines
The change fixes a bug introduced by
2be2a3ff42a5, memory allocated bykstrdup_const()must be always deallocated withkfree_const(), otherwise there is a risk of kfree'ing ro memory.This looks good. Can you provide also Cc-stable and fixes tags?
Since the change fixes two independent issues I decided not to add a particular commit to Fixes tag. I can split the commit of course, but I feel reluctant to send a series in this particular case.
Quoted 5 lines
Quoted 1 line
Let me know your decision with respect to my comments.
Although this is only error-path but still this applies for backporting to stable. Please split it up and add respective fixes tags. This helps companies/people using stable trees, including LTS.
Okay, I'll resend the split changes tomorrow.
Quoted 17 lines
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Also remove unneeded
of_node_put(), iffor_each_compatible_node()body execution is not terminated, this prevents from doublekfree()inOF_DYNAMICbuild.Each iteration of
for_each_compatible_node()has a check:
(dn =of_find_compatible_node(dn, type, compatible)) this increases the references to 'np'.Quoted 6 lines
Correct.
Quoted 1 line
If loop continues then previous 'np' is not
of_node_put().This I don't understand. The previous 'np' is
of_node_put()on next iteration of the loop, i.e. if and only if loop continues. Please elaborate.
Quoted 7 lines
Step by step, if I get it right:
1. initialization: dn =of_find_compatible_node(NULL, type, compatible); 1a. if (!pd->base) then we want to drop that reference.
1b. if not, then loop itself
3. increase value: dn =of_find_compatible_node(dn, type, compatible)
4. next iteration of loop, now we have 'dn' from last 'increase value'
5. if (!pd->base) then we want to drop that reference.
It is quite basic but it might be more visual, if the questionable expression is preprocessed and some C99 magic is applied on top:
for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
...
continue;
...
}
stands for
for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
dn; \
dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
...
continue;...
}
stands for
for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
dn; \
dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
...
goto contin;...
contin:
}
stands for
dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
while (dn) {
...
goto contin;
...
contin:
dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")
};then np reference counter is decremented inside closing of_find_compatible_node() as usual, there is no need to decrement it two times.
Do I miss something?
--
With best wishes,
Vladimir
Quoted 31 lines
Quoted 30 lines
Quoted 29 lines
Quoted 29 lines
Signed-off-by Vladimir Zapolskiy <[email protected]>--- arch/arm/mach-exynos/pm_domains.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)arch/arm/mach-exynos/pm_domains.c +1
-3@@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void) pd->base =of_iomap (np,0 );if (!pd->base) {pr_warn ("%s: failed to map memory\n" , __func__); -kfree (pd->pd.name); +kfree_const (pd->pd.name);kfree (pd); -of_node_put (np);continue ; } --2.1.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 ARM: EXYNOS: pd: fix resource deallocation on error path
Quoted 90 lines · Vladimir Zapolskiy
Quoted 13 lines · Krzysztof Kozlowski
Quoted 12 lines · Vladimir Zapolskiy
Quoted 6 lines · Krzysztof Kozlowski
2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <[email protected]>:
Quoted 3 lines
The change fixes a bug introduced by
2be2a3ff42a5, memory allocated bykstrdup_const()must be always deallocated withkfree_const(), otherwise there is a risk of kfree'ing ro memory.This looks good. Can you provide also Cc-stable and fixes tags?
Since the change fixes two independent issues I decided not to add a particular commit to Fixes tag. I can split the commit of course, but I feel reluctant to send a series in this particular case.
Quoted 5 lines
Quoted 1 line
Let me know your decision with respect to my comments.
Although this is only error-path but still this applies for backporting to stable. Please split it up and add respective fixes tags. This helps companies/people using stable trees, including LTS.
Okay, I'll resend the split changes tomorrow.
Quoted 17 lines
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Also remove unneeded
of_node_put(), iffor_each_compatible_node()body execution is not terminated, this prevents from doublekfree()inOF_DYNAMICbuild.Each iteration of
for_each_compatible_node()has a check:
(dn =of_find_compatible_node(dn, type, compatible)) this increases the references to 'np'.Quoted 6 lines
Correct.
Quoted 1 line
If loop continues then previous 'np' is not
of_node_put().This I don't understand. The previous 'np' is
of_node_put()on next iteration of the loop, i.e. if and only if loop continues. Please elaborate.Quoted 7 lines
Step by step, if I get it right:
1. initialization: dn =of_find_compatible_node(NULL, type, compatible); 1a. if (!pd->base) then we want to drop that reference.
1b. if not, then loop itself
3. increase value: dn =of_find_compatible_node(dn, type, compatible)
4. next iteration of loop, now we have 'dn' from last 'increase value'
5. if (!pd->base) then we want to drop that reference.It is quite basic but it might be more visual, if the questionable expression is preprocessed and some C99 magic is applied on top:
for_each_compatible_node(np, NULL, "samsung,exynos4210-pd"){
...continue;...
}stands for
for (dn =
of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");dn; \ dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) { ... continue;...
}stands for
for (dn =
of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");dn; \ dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) { ... goto contin;...
contin:
}stands for
dn =
of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");while (dn) { ... goto contin; ... contin: dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd") };
Quoted 5 lines
then np reference counter is decremented inside closing
of_find_compatible_node()as usual, there is no need to decrement it two times.Do I miss something?
Yes, you are right. Thanks for patience!
Best regards,
Krzysztof