* [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path @ 2015-07-29 20:15 ` Vladimir Zapolskiy 0 siblings, 0 replies; 12+ messages in thread From: Vladimir Zapolskiy @ 2015-07-29 20:15 UTC (permalink / raw) To: Kukjin Kim, Krzysztof Kozlowski Cc: linux-samsung-soc, Russell King, linux-arm-kernel, Marek Szyprowski 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. Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> --- arch/arm/mach-exynos/pm_domains.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c index 6001f1c..4a87e86 100644 --- a/arch/arm/mach-exynos/pm_domains.c +++ b/arch/arm/mach-exynos/pm_domains.c @@ -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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path @ 2015-07-29 20:15 ` Vladimir Zapolskiy 0 siblings, 0 replies; 12+ messages in thread From: Vladimir Zapolskiy @ 2015-07-29 20:15 UTC (permalink / raw) To: linux-arm-kernel 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. Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> --- arch/arm/mach-exynos/pm_domains.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c index 6001f1c..4a87e86 100644 --- a/arch/arm/mach-exynos/pm_domains.c +++ b/arch/arm/mach-exynos/pm_domains.c @@ -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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path 2015-07-29 20:15 ` Vladimir Zapolskiy @ 2015-07-29 23:37 ` Krzysztof Kozlowski -1 siblings, 0 replies; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-29 23:37 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc, Russell King, linux-arm-kernel, Marek Szyprowski 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: > 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. This looks good. Can you provide also Cc-stable and fixes tags? > > 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. 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 > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > --- > arch/arm/mach-exynos/pm_domains.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c > index 6001f1c..4a87e86 100644 > --- a/arch/arm/mach-exynos/pm_domains.c > +++ b/arch/arm/mach-exynos/pm_domains.c > @@ -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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path @ 2015-07-29 23:37 ` Krzysztof Kozlowski 0 siblings, 0 replies; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-29 23:37 UTC (permalink / raw) To: linux-arm-kernel 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: > 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. This looks good. Can you provide also Cc-stable and fixes tags? > > 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. 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 > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > --- > arch/arm/mach-exynos/pm_domains.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c > index 6001f1c..4a87e86 100644 > --- a/arch/arm/mach-exynos/pm_domains.c > +++ b/arch/arm/mach-exynos/pm_domains.c > @@ -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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path 2015-07-29 23:37 ` Krzysztof Kozlowski @ 2015-07-30 0:06 ` Vladimir Zapolskiy -1 siblings, 0 replies; 12+ messages in thread From: Vladimir Zapolskiy @ 2015-07-30 0:06 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Kukjin Kim, linux-samsung-soc, Russell King, linux-arm-kernel, Marek Szyprowski On 30.07.2015 02:37, Krzysztof Kozlowski wrote: > 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >> 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. > > 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. >> >> 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. > > 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. > 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 > >> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >> --- >> arch/arm/mach-exynos/pm_domains.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c >> index 6001f1c..4a87e86 100644 >> --- a/arch/arm/mach-exynos/pm_domains.c >> +++ b/arch/arm/mach-exynos/pm_domains.c >> @@ -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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path @ 2015-07-30 0:06 ` Vladimir Zapolskiy 0 siblings, 0 replies; 12+ messages in thread From: Vladimir Zapolskiy @ 2015-07-30 0:06 UTC (permalink / raw) To: linux-arm-kernel On 30.07.2015 02:37, Krzysztof Kozlowski wrote: > 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >> 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. > > 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. >> >> 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. > > 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. > 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 > >> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >> --- >> arch/arm/mach-exynos/pm_domains.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c >> index 6001f1c..4a87e86 100644 >> --- a/arch/arm/mach-exynos/pm_domains.c >> +++ b/arch/arm/mach-exynos/pm_domains.c >> @@ -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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path 2015-07-30 0:06 ` Vladimir Zapolskiy @ 2015-07-30 0:15 ` Krzysztof Kozlowski -1 siblings, 0 replies; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-30 0:15 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Kukjin Kim, linux-samsung-soc, Russell King, linux-arm-kernel, Marek Szyprowski On 30.07.2015 09:06, Vladimir Zapolskiy wrote: > On 30.07.2015 02:37, Krzysztof Kozlowski wrote: >> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >>> 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. >> >> 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. > >>> >>> 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. >> >> 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. > >> 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 > > -- > With best wishes, > Vladimir > >> >>> >>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >>> --- >>> arch/arm/mach-exynos/pm_domains.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c >>> index 6001f1c..4a87e86 100644 >>> --- a/arch/arm/mach-exynos/pm_domains.c >>> +++ b/arch/arm/mach-exynos/pm_domains.c >>> @@ -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@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path @ 2015-07-30 0:15 ` Krzysztof Kozlowski 0 siblings, 0 replies; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-30 0:15 UTC (permalink / raw) To: linux-arm-kernel On 30.07.2015 09:06, Vladimir Zapolskiy wrote: > On 30.07.2015 02:37, Krzysztof Kozlowski wrote: >> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >>> 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. >> >> 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. > >>> >>> 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. >> >> 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. > >> 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 > > -- > With best wishes, > Vladimir > >> >>> >>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >>> --- >>> arch/arm/mach-exynos/pm_domains.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c >>> index 6001f1c..4a87e86 100644 >>> --- a/arch/arm/mach-exynos/pm_domains.c >>> +++ b/arch/arm/mach-exynos/pm_domains.c >>> @@ -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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path 2015-07-30 0:15 ` Krzysztof Kozlowski @ 2015-07-30 0:35 ` Vladimir Zapolskiy -1 siblings, 0 replies; 12+ messages in thread From: Vladimir Zapolskiy @ 2015-07-30 0:35 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Kukjin Kim, linux-samsung-soc, Russell King, linux-arm-kernel, Marek Szyprowski On 30.07.2015 03:15, Krzysztof Kozlowski wrote: > On 30.07.2015 09:06, Vladimir Zapolskiy wrote: >> On 30.07.2015 02:37, Krzysztof Kozlowski wrote: >>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >>>> 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. >>> >>> 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. Okay, I'll resend the split changes tomorrow. >> >>>> >>>> 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. >>> >>> 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. >> >>> 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. 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 >>>> >>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >>>> --- >>>> arch/arm/mach-exynos/pm_domains.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c >>>> index 6001f1c..4a87e86 100644 >>>> --- a/arch/arm/mach-exynos/pm_domains.c >>>> +++ b/arch/arm/mach-exynos/pm_domains.c >>>> @@ -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@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path @ 2015-07-30 0:35 ` Vladimir Zapolskiy 0 siblings, 0 replies; 12+ messages in thread From: Vladimir Zapolskiy @ 2015-07-30 0:35 UTC (permalink / raw) To: linux-arm-kernel On 30.07.2015 03:15, Krzysztof Kozlowski wrote: > On 30.07.2015 09:06, Vladimir Zapolskiy wrote: >> On 30.07.2015 02:37, Krzysztof Kozlowski wrote: >>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >>>> 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. >>> >>> 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. Okay, I'll resend the split changes tomorrow. >> >>>> >>>> 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. >>> >>> 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. >> >>> 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. 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 >>>> >>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >>>> --- >>>> arch/arm/mach-exynos/pm_domains.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c >>>> index 6001f1c..4a87e86 100644 >>>> --- a/arch/arm/mach-exynos/pm_domains.c >>>> +++ b/arch/arm/mach-exynos/pm_domains.c >>>> @@ -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 >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path 2015-07-30 0:35 ` Vladimir Zapolskiy @ 2015-07-30 0:55 ` Krzysztof Kozlowski -1 siblings, 0 replies; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-30 0:55 UTC (permalink / raw) To: Vladimir Zapolskiy Cc: Kukjin Kim, linux-samsung-soc, Russell King, linux-arm-kernel, Marek Szyprowski On 30.07.2015 09:35, Vladimir Zapolskiy wrote: > On 30.07.2015 03:15, Krzysztof Kozlowski wrote: >> On 30.07.2015 09:06, Vladimir Zapolskiy wrote: >>> On 30.07.2015 02:37, Krzysztof Kozlowski wrote: >>>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >>>>> 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. >>>> >>>> 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. > > Okay, I'll resend the split changes tomorrow. > >>> >>>>> >>>>> 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. >>>> >>>> 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. >>> >>>> 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. > > 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? Yes, you are right. Thanks for patience! Best regards, Krzysztof ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path @ 2015-07-30 0:55 ` Krzysztof Kozlowski 0 siblings, 0 replies; 12+ messages in thread From: Krzysztof Kozlowski @ 2015-07-30 0:55 UTC (permalink / raw) To: linux-arm-kernel On 30.07.2015 09:35, Vladimir Zapolskiy wrote: > On 30.07.2015 03:15, Krzysztof Kozlowski wrote: >> On 30.07.2015 09:06, Vladimir Zapolskiy wrote: >>> On 30.07.2015 02:37, Krzysztof Kozlowski wrote: >>>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>: >>>>> 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. >>>> >>>> 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. > > Okay, I'll resend the split changes tomorrow. > >>> >>>>> >>>>> 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. >>>> >>>> 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. >>> >>>> 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. > > 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? Yes, you are right. Thanks for patience! Best regards, Krzysztof ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-07-30 0:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-29 20:15 [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path Vladimir Zapolskiy 2015-07-29 20:15 ` Vladimir Zapolskiy 2015-07-29 23:37 ` Krzysztof Kozlowski 2015-07-29 23:37 ` Krzysztof Kozlowski 2015-07-30 0:06 ` Vladimir Zapolskiy 2015-07-30 0:06 ` Vladimir Zapolskiy 2015-07-30 0:15 ` Krzysztof Kozlowski 2015-07-30 0:15 ` Krzysztof Kozlowski 2015-07-30 0:35 ` Vladimir Zapolskiy 2015-07-30 0:35 ` Vladimir Zapolskiy 2015-07-30 0:55 ` Krzysztof Kozlowski 2015-07-30 0:55 ` Krzysztof Kozlowski
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.