lkml.org 
[lkml]   [2018]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v7 2/5] of: change overlay apply input data from unflattened to FDT
    On Wed, Apr 25, 2018 at 3:07 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
    > On 2018-04-25 20:40, Frank Rowand wrote:
    >> On 04/24/18 22:23, Jan Kiszka wrote:
    >>> On 2018-04-24 22:56, Frank Rowand wrote:
    >>>> Hi Alan,
    >>>>
    >>>> On 04/23/18 15: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.
    >>>>>
    >>>>> 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
    >>>>> 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).
    >>>>
    >>>> Question added below this paragraph.
    >>>>
    >>>>
    >>>>> 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
    >>>>> 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.
    >>>>
    >>>> Can the fpga notifiers be changed to have the changeset as an input
    >>>> instead of having the overlay devicetree fragment and target as an
    >>>> input?
    >>>>
    >>>> The changeset lists nodes and properties to be added, but does not
    >>>> expose any pointers to the overlay fdt or the overlay unflattened
    >>>> devicetree. This guarantees no leakage of pointers into the overlay
    >>>> fdt or the overlay unflattened devicetree. The changeset contains
    >>>> pointers to copies of data, but those copies are never freed (and
    >>>> thus they are yet another existing memory leak).
    >>>
    >>> Also they are freed, of course: When the last reference to the node they
    >>> point to reaches 0 (e.g. triggered by of_changeset_destroy), that node
    >>> goes away and takes down remaining dead properties. I've ran through
    >>> this already. And I also made sure that my code is not triggering such
    >>> kind of leaks as well.
    >>
    >> mea culpa. I go around in circles while trying to remember all the
    >> overlay related issues. I needed to go back and read the code to
    >> refresh my memory. Thanks for the prod to re-read the code.
    >>
    >> Yes, of_changeset_destroy() will lead to the kfree() of the node and
    >> it's properties _if_ the node reference count is correct. So what I
    >> said about a memory leak was incorrect in a perfect world (and my
    >> memory was wrong). However, this is not a perfect world and we know
    >> that the reference count on devicetree nodes is often incorrect due
    >> to bugs in common infrastructure and drivers. This issue will not
    >> be resolved until we pull all reference count manipulation into the
    >> devicetree core.
    >
    > I don't get this yet. When I want some value from life tree, I do a node
    > search, get a pointer and the core incremented its reference, can query
    > the node and its properties, and when I'm done, I call of_node_put and
    > forget about all pointers I got. What would you do differently?
    >
    >> The net result is that we should not expect
    >> overlay removal to correctly free all memory that was allocated
    >> when applying the overlay.
    >
    > Depends on the overlay. If you do not modify existing nodes but only add
    > new ones, it is fair to expect complete removal.
    >
    >>
    >> I _think_ (but did not spend the time to verify) that there is a small
    >> corner case memory leak even if the reference count on devicetree
    >> nodes is correct. If an overlay adds a property to an existing node
    >> then removing the overlay will not kfree() the property, and it
    >> will remain on the deadprops list. There are some places that
    >> properties are removed from deadprops, but I don't think they fully
    >> resolve the issue. Again, this is a corner case, and I am willing
    >> to document it as a limitation until it gets fixed.

    This doesn't solve all of your concern, but it gets me wondering
    whether overlay_notify should add a of_node_get(fragment->overlay)
    before doing the blocking_notifier_call_chain and a of_node_put
    afterwards.

    >
    > I ran into this the other day: If you modify an existing property, the
    > old value will be put into deadprops and only be freed when the node is
    > freed. It may come back from deadprops if a changeset comes around with
    > the very same property object for another modification.
    >
    > But that means: if your overlay just adds nodes, all of them, including
    > their deadprops from potential changes on top, will go away on overlay
    > removal.
    >
    > BTW, here is my new code that exploits this to be leak-free:
    > https://github.com/siemens/jailhouse/blob/156a93fcc02585d78d4418d3e6761cd72a65b359/driver/pci.c#L296
    >
    >>
    >> Then returning to me going around in circles... This thread led me to
    >> think that since since the overlay apply code copied data into never
    >> freed memory (false premise, as you pointed out) that we did not
    >> have to worry about drivers retaining pointers into overlay data
    >> after the overlay had been freed (with the one remaining exposure
    >> being via the overlay notifiers, which _might_ be easily resolved,
    >> pending Alan's analysis) -- this would have been great news for
    >> removing an issue for general use of overlays.
    >>
    >> But now we are back to the long-standing problem that we have no way
    >> of knowing whether there are any live pointers to the memory that is
    >> freed by of_changeset_destroy(). And I am not aware of any solution
    >> to this problem other than changing the devicetree access API so that
    >> it never returns any pointer into the live devicetree.
    >
    > I don't agree yet with this drastic measure until you can point me to
    > code that pulls and stores pointers to arbitrary devicetree content
    > without that node reference counting. The pattern we otherwise see all
    > around it you get a pointer (or a set of them) along with the duty to
    > explicitly drop it again by some put() operation.
    >
    >>
    >> The practical impact of all of this, is if we can change the overlay
    >> notifier parameters to include the overlay changeset instead of
    >> the overlay devicetree, then I think that of_overlay_apply() will
    >> be able to kfree() the overlay fdt and overlay devicetree. And
    >> if not of_overlay_apply(), then free_overlay_changeset().
    >
    > Isn't that just s/node/changeset/ without any other semantic changes? If
    > the receiver of the changeset reference does not take care of lifecycle
    > management for that object either, we are back at square #1. A changeset
    > is just a gate to the nodes and properties that are currently passed
    > directly.
    >
    > Jan

    \
     
     \ /
      Last update: 2018-04-25 22:28    [W:2.989 / U:0.352 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site