Re: [PATCH v7 2/5] of: change overlay apply input data from unflattened to FDT

[Jan Kiszka <jan.kiszka@web.de>]

On 2018-04-24 00:38, Frank Rowand wrote:
> Hi Jan,
> 
> + Alan Tull for fpga perspective
> 
> On 04/22/18 03:30, Jan Kiszka wrote:
>> On 2018-04-11 07:42, Jan Kiszka wrote:
>>> On 2018-04-05 23:12, Rob Herring wrote:
>>>> On Thu, Apr 5, 2018 at 2:28 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>>>>> On 04/05/18 12:13, Jan Kiszka wrote:
>>>>>> On 2018-04-05 20:59, Frank Rowand wrote:
>>>>>>> Hi Jan,
>>>>>>>
>>>>>>> On 04/04/18 15:35, Jan Kiszka wrote:
>>>>>>>> Hi Frank,
>>>>>>>>
>>>>>>>> On 2018-03-04 01:17, frowand.list@gmail.com wrote:
>>>>>>>>> From: Frank Rowand <frank.rowand@sony.com>
>>>>>>>>>
>>>>>>>>> Move duplicating and unflattening of an overlay flattened devicetree
>>>>>>>>> (FDT) into the overlay application code.  To accomplish this,
>>>>>>>>> of_overlay_apply() is replaced by of_overlay_fdt_apply().
>>>>>>>>>
>>>>>>>>> The copy of the FDT (aka "duplicate FDT") now belongs to devicetree
>>>>>>>>> code, which is thus responsible for freeing the duplicate FDT.  The
>>>>>>>>> caller of of_overlay_fdt_apply() remains responsible for freeing the
>>>>>>>>> original FDT.
>>>>>>>>>
>>>>>>>>> The unflattened devicetree now belongs to devicetree code, which is
>>>>>>>>> thus responsible for freeing the unflattened devicetree.
>>>>>>>>>
>>>>>>>>> These ownership changes prevent early freeing of the duplicated FDT
>>>>>>>>> or the unflattened devicetree, which could result in use after free
>>>>>>>>> errors.
>>>>>>>>>
>>>>>>>>> of_overlay_fdt_apply() is a private function for the anticipated
>>>>>>>>> overlay loader.
>>>>>>>>
>>>>>>>> We are using of_fdt_unflatten_tree + of_overlay_apply in the
>>>>>>>> (out-of-tree) Jailhouse loader driver in order to register a virtual
>>>>>>>> device during hypervisor activation with Linux. The DT overlay is
>>>>>>>> created from a a template but modified prior to application to account
>>>>>>>> for runtime-specific parameters. See [1] for the current implementation.
>>>>>>>>
>>>>>>>> I'm now wondering how to model that scenario best with the new API.
>>>>>>>> Given that the loader lost ownership of the unflattened tree but the
>>>>>>>> modification API exist only for the that DT state, I'm not yet seeing a
>>>>>>>> clear solution. Should we apply the template in disabled form (status =
>>>>>>>> "disabled"), modify it, and then activate it while it is already applied?
>>>>>>>
>>>>>>> Thank you for the pointer to the driver - that makes it much easier to
>>>>>>> understand the use case and consider solutions.
>>>>>>>
>>>>>>> If you can make the changes directly on the FDT instead of on the
>>>>>>> expanded devicetree, then you could move to the new API.
>>>>>>
>>>>>> Are there some examples/references on how to edit FDTs in-place in the
>>>>>> kernel? I'd like to avoid writing the n-th FDT parser/generator.
>>>>>
>>>>> I don't know of any existing in-kernel edits of the FDT (but they might
>>>>> exist).  The functions to access an FDT are in libfdt, which is in
>>>>> scripts/dtc/libfdt/.
>>>>
>>>> Let's please not go down that route of doing FDT modifications. There
>>>> is little reason to other than for early boot changes. And it is much
>>>> easier to work on unflattened trees.
>>>
>>> I just briefly looked into libfdt, and it would have meant building it
>>> into the module as there are no library functions exported by the kernel
>>> either. Another reason to drop that.
>>>
>>> What's apparently working now is the pattern I initially suggested:
>>> Register template with status = "disabled" as overlay, then prepare and
>>> apply changeset that contains all needed modifications and sets the
>>> status to "ok". I might be leaking additional resources, but to find
>>> that out, I will now finally have to resolve clean unbinding of the
>>> generic PCI host controller [1] first.
>>
>> static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> {
>> 	[...]
>> 	/*
>> 	 * 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);
>> }
>>
>> What's this? I have kmemleak now jumping at me over this. Who is suppose
>> to plug these leaks? The caller of of_overlay_fdt_apply has no pointers
>> to those objects. I would say that's a regression of the new API.
> 
> The problem already existed but it was hidden.  We have never been able to
> kfree() these object because we do not know if there are any pointers into
> these objects.  The new API makes the problem visible to kmemleak.

My old code didn't have the problem because there was no one steeling
pointers to my overlay, and I was able to safely release all the
resources that I or the core on my behalf allocated. In fact, I recently
even dropped the duplication the fdt prior to unflattening it because I
got its lifecycle under control (and both kmemleak as well as kasan
confirmed this). I still consider this intentional leak a regression of
the new API.

> 
> The reason that we do not know if there are any pointers into these objects
> is that devicetree access APIs return pointers into the devicetree internal
> data structures (that is, into the overlay unflattened devicetree).  If we
> want to be able to do the kfree()s, we could change the devicetree access
> APIs.
> 
> The reason that pointers into the overlay flattened tree (ovcs->fdt) are
> also exposed is that the overlay unflattened devicetree property values
> are pointers into the overlay fdt.
> 
> ** This paragraph becomes academic (and not needed) if the fix in the next
> paragraph can be implemented. **
> I _think_ that the fdt issue __for overlays__ can be fixed somewhat easily.
> (I would want to read through the code again to make sure I'm not missing
> any issues.)  If the of_fdt_unflatten_tree() called by of_overlay_fdt_apply()
> was modified so that property values were copied into newly allocated memory
> and the live tree property pointers were set to the copy instead of to
> the value in the fdt, then I _think_ the fdt could be freed in
> of_overlay_fdt_apply() after calling of_overlay_apply().  The code that

I don't see yet how more duplicating of objects would help. Then we
would not leak the fdt or the unflattened tree on overlay destruction
but that duplicates, no?

> frees a devicetree would also have to be aware of this change -- I'm not
> sure if that leads to ugly complications or if it is easy.  The other
> question to consider is whether to make the same change to
> of_fdt_unflatten_tree() when it is called in early boot to unflatten
> the base devicetree.  Doing so would increase the memory usage of the
> live tree (we would not be able to free the base fdt after unflattening
> it because we make the fdt visible in /sys/firmware/fdt -- though
> _maybe_ that could be conditioned on CONFIG_KEXEC).
> 
> But all of the complexity of that fix is _only_ because of_overlay_apply()
> and of_overlay_remove() call overlay_notify(), passing in the overlay
> unflattened devicetree (which has pointers into the overlay fdt). Pointers
> into the overlay unflattened devicetree are then passed to the notifiers.
> (Again, I may be missing some other place that the overlay unflattened
> devicetree is made visible to other code -- a more thorough reading of
> the code is needed.) If the notifiers could be modified to accept the
> changeset list instead of of pointers to the fragments in the overlay
> unflattened devicetree then there would be no possibility of the notifiers
> keeping a pointer into the overlay fdt. I do not know if this is a

But then again the convention has to be that those changeset pointers
must not be kept - because the changeset is history after of_overlay_remove.

> practical change for the notifiers -- there are no callers of
> of_overlay_notifier_register() in the mainline kernel source. My
> recollection is that the overlay notifiers were added for the fpga
> subsystem.

We have drivers/fpga/of-fpga-region.c in-tree, and that does not seem to
store any pointers to objects, rather consumes them in-place. And I
would consider it fair to impose such a limitation on the notifier
interface.

> 
> Why is overlay_notify() the only issue related to unknown users having
> pointers into the overlay fdt?  The answer is that the overlay code
> does not directly expose the overlay unflattened devicetree (and thus
> indirectly the overlay fdt) to the live devicetree -- when the
> overlay code creates the overlay changeset, it copies from the
> overlay unflattened devicetree and overlay fdt and only exposes
> pointers to the copies.
> 
> And hopefully the issues with the overlay unflattened devicetree can
> be resolved in the same way as for the overlay fdt.

As noted above, I don't see there is a technical solution to this issue
but it's rather a matter of convention: no overlay notifier callback is
allowed to keep references to the passed tree content (unless it
reference-counts some tree nodes) beyond the execution of the callback.
With that in place, we can safely drop the backing memory IMHO.

Jan