[PATCH] of: overlay: Stop leaking resources on overlay removal

[PATCH] of: overlay: Stop leaking resources on overlay removal

[Jan Kiszka]

Only the overlay notifier callbacks have a chance to potentially get
hold of references to those two resources, but they do not store them.
So it is safe to stop the intentional leaking.

See also https://lkml.org/lkml/2018/4/23/1063 and following.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Ideally, we sort out any remaining worries during the 4.17-rc cycle.

 drivers/of/overlay.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index b35fe88f1851..3553f1f57a62 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -671,17 +671,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 		of_node_put(ovcs->fragments[i].overlay);
 	}
 	kfree(ovcs->fragments);
-
-	/*
-	 * TODO
-	 *
-	 * would like to: kfree(ovcs->overlay_tree);
-	 * but can not since drivers may have pointers into this data
-	 *
-	 * would like to: kfree(ovcs->fdt);
-	 * but can not since drivers may have pointers into this data
-	 */
-
+	kfree(ovcs->overlay_tree);
+	kfree(ovcs->fdt);
 	kfree(ovcs);
 }
 
-- 
2.13.6

Re: [PATCH] of: overlay: Stop leaking resources on overlay removal

[Frank Rowand]

On 04/24/18 09:19, Jan Kiszka wrote:
> Only the overlay notifier callbacks have a chance to potentially get
> hold of references to those two resources, but they do not store them.
> So it is safe to stop the intentional leaking.
> 
> See also https://lkml.org/lkml/2018/4/23/1063 and following.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Ideally, we sort out any remaining worries during the 4.17-rc cycle.
> 
>  drivers/of/overlay.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index b35fe88f1851..3553f1f57a62 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -671,17 +671,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  		of_node_put(ovcs->fragments[i].overlay);
>  	}
>  	kfree(ovcs->fragments);
> -
> -	/*
> -	 * TODO
> -	 *
> -	 * would like to: kfree(ovcs->overlay_tree);
> -	 * but can not since drivers may have pointers into this data
> -	 *
> -	 * would like to: kfree(ovcs->fdt);
> -	 * but can not since drivers may have pointers into this data
> -	 */
> -
> +	kfree(ovcs->overlay_tree);
> +	kfree(ovcs->fdt);
>  	kfree(ovcs);
>  }
>  
> 

Nack.  It is premature to submit this while the conversation is
continuing in the other thread.

I'll continue the conversation in the other thread.

-Frank

Re: [PATCH] of: overlay: Stop leaking resources on overlay removal

[Jan Kiszka]

On 2018-04-24 19:44, Frank Rowand wrote:
> On 04/24/18 09:19, Jan Kiszka wrote:
>> Only the overlay notifier callbacks have a chance to potentially get
>> hold of references to those two resources, but they do not store them.
>> So it is safe to stop the intentional leaking.
>>
>> See also https://lkml.org/lkml/2018/4/23/1063 and following.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> Ideally, we sort out any remaining worries during the 4.17-rc cycle.
>>
>>  drivers/of/overlay.c | 13 ++-----------
>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index b35fe88f1851..3553f1f57a62 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -671,17 +671,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>  		of_node_put(ovcs->fragments[i].overlay);
>>  	}
>>  	kfree(ovcs->fragments);
>> -
>> -	/*
>> -	 * TODO
>> -	 *
>> -	 * would like to: kfree(ovcs->overlay_tree);
>> -	 * but can not since drivers may have pointers into this data
>> -	 *
>> -	 * would like to: kfree(ovcs->fdt);
>> -	 * but can not since drivers may have pointers into this data
>> -	 */
>> -
>> +	kfree(ovcs->overlay_tree);
>> +	kfree(ovcs->fdt);
>>  	kfree(ovcs);
>>  }
>>  
>>
> 
> Nack.  It is premature to submit this while the conversation is
> continuing in the other thread.
> 
> I'll continue the conversation in the other thread.
> 

Well, at least the strongest argument has been resolved now, the
notifier topic. Curious to learn what remains. As I noted, we should
work hard to sort out the API regression prior to the release.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] of: overlay: Stop leaking resources on overlay removal

[Frank Rowand]

On 04/24/18 10:50, Jan Kiszka wrote:
> On 2018-04-24 19:44, Frank Rowand wrote:
>> On 04/24/18 09:19, Jan Kiszka wrote:
>>> Only the overlay notifier callbacks have a chance to potentially get
>>> hold of references to those two resources, but they do not store them.
>>> So it is safe to stop the intentional leaking.
>>>
>>> See also https://lkml.org/lkml/2018/4/23/1063 and following.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> Ideally, we sort out any remaining worries during the 4.17-rc cycle.
>>>
>>>  drivers/of/overlay.c | 13 ++-----------
>>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index b35fe88f1851..3553f1f57a62 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -671,17 +671,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>  		of_node_put(ovcs->fragments[i].overlay);
>>>  	}
>>>  	kfree(ovcs->fragments);
>>> -
>>> -	/*
>>> -	 * TODO
>>> -	 *
>>> -	 * would like to: kfree(ovcs->overlay_tree);
>>> -	 * but can not since drivers may have pointers into this data
>>> -	 *
>>> -	 * would like to: kfree(ovcs->fdt);
>>> -	 * but can not since drivers may have pointers into this data
>>> -	 */
>>> -
>>> +	kfree(ovcs->overlay_tree);
>>> +	kfree(ovcs->fdt);
>>>  	kfree(ovcs);
>>>  }
>>>  
>>>
>>
>> Nack.  It is premature to submit this while the conversation is
>> continuing in the other thread.
>>
>> I'll continue the conversation in the other thread.
>>
> 
> Well, at least the strongest argument has been resolved now, the
> notifier topic. Curious to learn what remains. As I noted, we should
> work hard to sort out the API regression prior to the release.

Nope, the notifier discussion continues in the other thread.

Re: [PATCH] of: overlay: Stop leaking resources on overlay removal

[Frank Rowand]

Hi Jan,

On 04/24/18 13:58, Frank Rowand wrote:
> On 04/24/18 10:50, Jan Kiszka wrote:
>> On 2018-04-24 19:44, Frank Rowand wrote:
>>> On 04/24/18 09:19, Jan Kiszka wrote:
>>>> Only the overlay notifier callbacks have a chance to potentially get
>>>> hold of references to those two resources, but they do not store them.
>>>> So it is safe to stop the intentional leaking.
>>>>
>>>> See also https://lkml.org/lkml/2018/4/23/1063 and following.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> Ideally, we sort out any remaining worries during the 4.17-rc cycle.
>>>>
>>>>  drivers/of/overlay.c | 13 ++-----------
>>>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index b35fe88f1851..3553f1f57a62 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -671,17 +671,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>>  		of_node_put(ovcs->fragments[i].overlay);
>>>>  	}
>>>>  	kfree(ovcs->fragments);
>>>> -
>>>> -	/*
>>>> -	 * TODO
>>>> -	 *
>>>> -	 * would like to: kfree(ovcs->overlay_tree);
>>>> -	 * but can not since drivers may have pointers into this data
>>>> -	 *
>>>> -	 * would like to: kfree(ovcs->fdt);
>>>> -	 * but can not since drivers may have pointers into this data
>>>> -	 */
>>>> -
>>>> +	kfree(ovcs->overlay_tree);
>>>> +	kfree(ovcs->fdt);
>>>>  	kfree(ovcs);
>>>>  }
>>>>  
>>>>
>>>
>>> Nack.  It is premature to submit this while the conversation is
>>> continuing in the other thread.
>>>
>>> I'll continue the conversation in the other thread.
>>>
>>
>> Well, at least the strongest argument has been resolved now, the
>> notifier topic. Curious to learn what remains. As I noted, we should
>> work hard to sort out the API regression prior to the release.
> 
> Nope, the notifier discussion continues in the other thread.

Thanks for your patience in the other thread.

As I noted there, I am now willing to accept this patch with some
small changes.  Please add a minimal section to
Documentation/devicetree/overlay-notes.txt about overlay notifiers.
The most important thing to note there is that the overlay notifiers
are not allowed to retain any pointers into the overlay devicetree.
Also, instead of removing the "TODO" comment in free_overlay_changeset(),
change it to say something to the effect of "there should be no live pointers
into ovcs->overlay_tree and ovcs->fdt due to the policy that overlay
notifiers are not allowed to retain pointers into the overlay devicetree".

I will also add myself to the OPEN FIRMWARE AND DEVICE TREE OVERLAYS
entry of MAINTAINERS and add a keyword line to catch overlay notifiers.

I am not happy about freeing the overlay devicetree and overlay fdt
while overlay notifiers are able to retain pointers into the overlay
devicetree and overlay fdt, but am willing to accept documentation and
review as a partial protection until the devicetree access APIs can be
modified to prevent the notifiers from accessing the pointers.  The
volume of overlay notifier patches should be small enough to not be
a review burden.

-Frank

Re: [PATCH] of: overlay: Stop leaking resources on overlay removal

[Frank Rowand]

On 04/25/18 17:32, Frank Rowand wrote:
> Hi Jan,
> 
> On 04/24/18 13:58, Frank Rowand wrote:
>> On 04/24/18 10:50, Jan Kiszka wrote:
>>> On 2018-04-24 19:44, Frank Rowand wrote:
>>>> On 04/24/18 09:19, Jan Kiszka wrote:
>>>>> Only the overlay notifier callbacks have a chance to potentially get
>>>>> hold of references to those two resources, but they do not store them.
>>>>> So it is safe to stop the intentional leaking.
>>>>>
>>>>> See also https://lkml.org/lkml/2018/4/23/1063 and following.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> Ideally, we sort out any remaining worries during the 4.17-rc cycle.
>>>>>
>>>>>  drivers/of/overlay.c | 13 ++-----------
>>>>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>> index b35fe88f1851..3553f1f57a62 100644
>>>>> --- a/drivers/of/overlay.c
>>>>> +++ b/drivers/of/overlay.c
>>>>> @@ -671,17 +671,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>>>  		of_node_put(ovcs->fragments[i].overlay);
>>>>>  	}
>>>>>  	kfree(ovcs->fragments);
>>>>> -
>>>>> -	/*
>>>>> -	 * TODO
>>>>> -	 *
>>>>> -	 * would like to: kfree(ovcs->overlay_tree);
>>>>> -	 * but can not since drivers may have pointers into this data
>>>>> -	 *
>>>>> -	 * would like to: kfree(ovcs->fdt);
>>>>> -	 * but can not since drivers may have pointers into this data
>>>>> -	 */
>>>>> -
>>>>> +	kfree(ovcs->overlay_tree);
>>>>> +	kfree(ovcs->fdt);
>>>>>  	kfree(ovcs);
>>>>>  }
>>>>>  
>>>>>
>>>>
>>>> Nack.  It is premature to submit this while the conversation is
>>>> continuing in the other thread.
>>>>
>>>> I'll continue the conversation in the other thread.
>>>>
>>>
>>> Well, at least the strongest argument has been resolved now, the
>>> notifier topic. Curious to learn what remains. As I noted, we should
>>> work hard to sort out the API regression prior to the release.
>>
>> Nope, the notifier discussion continues in the other thread.
> 
> Thanks for your patience in the other thread.
> 
> As I noted there, I am now willing to accept this patch with some
> small changes.  Please add a minimal section to
> Documentation/devicetree/overlay-notes.txt about overlay notifiers.
> The most important thing to note there is that the overlay notifiers
> are not allowed to retain any pointers into the overlay devicetree.

Please also add a function header comment to of_overlay_notifier_register()
in drivers/of/overlay.c that notes the restriction on the overlay notifier.


> Also, instead of removing the "TODO" comment in free_overlay_changeset(),
> change it to say something to the effect of "there should be no live pointers
> into ovcs->overlay_tree and ovcs->fdt due to the policy that overlay
> notifiers are not allowed to retain pointers into the overlay devicetree".
> 
> I will also add myself to the OPEN FIRMWARE AND DEVICE TREE OVERLAYS
> entry of MAINTAINERS and add a keyword line to catch overlay notifiers.
> 
> I am not happy about freeing the overlay devicetree and overlay fdt
> while overlay notifiers are able to retain pointers into the overlay
> devicetree and overlay fdt, but am willing to accept documentation and
> review as a partial protection until the devicetree access APIs can be
> modified to prevent the notifiers from accessing the pointers.  The
> volume of overlay notifier patches should be small enough to not be
> a review burden.
> 
> -Frank
> 
> 

Re: [PATCH] of: overlay: Stop leaking resources on overlay removal

[Jan Kiszka]

On 2018-04-26 02:44, Frank Rowand wrote:
> On 04/25/18 17:32, Frank Rowand wrote:
>> Hi Jan,
>>
>> On 04/24/18 13:58, Frank Rowand wrote:
>>> On 04/24/18 10:50, Jan Kiszka wrote:
>>>> On 2018-04-24 19:44, Frank Rowand wrote:
>>>>> On 04/24/18 09:19, Jan Kiszka wrote:
>>>>>> Only the overlay notifier callbacks have a chance to potentially get
>>>>>> hold of references to those two resources, but they do not store them.
>>>>>> So it is safe to stop the intentional leaking.
>>>>>>
>>>>>> See also https://lkml.org/lkml/2018/4/23/1063 and following.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>
>>>>>> Ideally, we sort out any remaining worries during the 4.17-rc cycle.
>>>>>>
>>>>>>  drivers/of/overlay.c | 13 ++-----------
>>>>>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>>> index b35fe88f1851..3553f1f57a62 100644
>>>>>> --- a/drivers/of/overlay.c
>>>>>> +++ b/drivers/of/overlay.c
>>>>>> @@ -671,17 +671,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>>>>  		of_node_put(ovcs->fragments[i].overlay);
>>>>>>  	}
>>>>>>  	kfree(ovcs->fragments);
>>>>>> -
>>>>>> -	/*
>>>>>> -	 * TODO
>>>>>> -	 *
>>>>>> -	 * would like to: kfree(ovcs->overlay_tree);
>>>>>> -	 * but can not since drivers may have pointers into this data
>>>>>> -	 *
>>>>>> -	 * would like to: kfree(ovcs->fdt);
>>>>>> -	 * but can not since drivers may have pointers into this data
>>>>>> -	 */
>>>>>> -
>>>>>> +	kfree(ovcs->overlay_tree);
>>>>>> +	kfree(ovcs->fdt);
>>>>>>  	kfree(ovcs);
>>>>>>  }
>>>>>>  
>>>>>>
>>>>>
>>>>> Nack.  It is premature to submit this while the conversation is
>>>>> continuing in the other thread.
>>>>>
>>>>> I'll continue the conversation in the other thread.
>>>>>
>>>>
>>>> Well, at least the strongest argument has been resolved now, the
>>>> notifier topic. Curious to learn what remains. As I noted, we should
>>>> work hard to sort out the API regression prior to the release.
>>>
>>> Nope, the notifier discussion continues in the other thread.
>>
>> Thanks for your patience in the other thread.
>>
>> As I noted there, I am now willing to accept this patch with some
>> small changes.  Please add a minimal section to
>> Documentation/devicetree/overlay-notes.txt about overlay notifiers.
>> The most important thing to note there is that the overlay notifiers
>> are not allowed to retain any pointers into the overlay devicetree.
> 
> Please also add a function header comment to of_overlay_notifier_register()
> in drivers/of/overlay.c that notes the restriction on the overlay notifier.
> 
> 
>> Also, instead of removing the "TODO" comment in free_overlay_changeset(),
>> change it to say something to the effect of "there should be no live pointers
>> into ovcs->overlay_tree and ovcs->fdt due to the policy that overlay
>> notifiers are not allowed to retain pointers into the overlay devicetree".
>>
>> I will also add myself to the OPEN FIRMWARE AND DEVICE TREE OVERLAYS
>> entry of MAINTAINERS and add a keyword line to catch overlay notifiers.
>>
>> I am not happy about freeing the overlay devicetree and overlay fdt
>> while overlay notifiers are able to retain pointers into the overlay
>> devicetree and overlay fdt, but am willing to accept documentation and
>> review as a partial protection until the devicetree access APIs can be
>> modified to prevent the notifiers from accessing the pointers.  The
>> volume of overlay notifier patches should be small enough to not be
>> a review burden.
>>
>> -Frank
>>
>>
> 

Thanks for reconsidering! I'll look into your change requests soon and
come up with v2.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux