From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Kozlowski Subject: Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path Date: Thu, 30 Jul 2015 09:55:13 +0900 Message-ID: <55B975F1.5080408@samsung.com> References: <1438200913-10532-1-git-send-email-vz@mleia.com> <55B96A86.8000502@mleia.com> <55B96C9C.8010908@samsung.com> <55B97147.7000202@mleia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mailout3.w1.samsung.com ([210.118.77.13]:54261 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751371AbbG3AzO (ORCPT ); Wed, 29 Jul 2015 20:55:14 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NS900A62ZW0ZY20@mailout3.w1.samsung.com> for linux-samsung-soc@vger.kernel.org; Thu, 30 Jul 2015 01:55:12 +0100 (BST) In-reply-to: <55B97147.7000202@mleia.com> Sender: linux-samsung-soc-owner@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org To: Vladimir Zapolskiy Cc: Kukjin Kim , linux-samsung-soc@vger.kernel.org, Russell King , linux-arm-kernel@lists.infradead.org, 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 : >>>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: k.kozlowski@samsung.com (Krzysztof Kozlowski) Date: Thu, 30 Jul 2015 09:55:13 +0900 Subject: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path In-Reply-To: <55B97147.7000202@mleia.com> References: <1438200913-10532-1-git-send-email-vz@mleia.com> <55B96A86.8000502@mleia.com> <55B96C9C.8010908@samsung.com> <55B97147.7000202@mleia.com> Message-ID: <55B975F1.5080408@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 : >>>>> 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