From: k.kozlowski@samsung•com (Krzysztof Kozlowski)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path
Date: Thu, 30 Jul 2015 09:55:13 +0900 [thread overview]
Message-ID: <55B975F1.5080408@samsung.com> (raw)
In-Reply-To: <55B97147.7000202@mleia.com>
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
prev parent reply other threads:[~2015-07-30 0:55 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-29 20:15 [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path Vladimir Zapolskiy
2015-07-29 23:37 ` Krzysztof Kozlowski
2015-07-30 0:06 ` Vladimir Zapolskiy
2015-07-30 0:15 ` Krzysztof Kozlowski
2015-07-30 0:35 ` Vladimir Zapolskiy
2015-07-30 0:55 ` Krzysztof Kozlowski [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55B975F1.5080408@samsung.com \
--to=k.kozlowski@samsung$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox