lkml.org 
[lkml]   [2018]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v7 2/5] of: change overlay apply input data from unflattened to FDT
    From
    Date
    On 04/25/18 07:59, Alan Tull wrote:
    > On Tue, Apr 24, 2018 at 3:56 PM, Frank Rowand <frowand.list@gmail.com> 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?
    >
    > I'll look into it. Just to be clear, are you suggesting passing
    > struct overlay_changeset instead in the notifier?

    Ah, poor phrasing on my part. I meant a "struct of_changeset", as is
    passed into __of_changeset_apply_entries(), which is called from
    of_overlay_apply(). This means that the call to overlay_notify()
    would have to move down a few lines to just after calling
    build_changeset().


    > struct overlay_changeset and struct fragment would have to be moved to a header.
    >
    >>
    >> 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).
    >>
    >> -Frank
    >>
    >>> 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.
    >>>
    >>> -Frank
    >>>
    >>>
    >>>
    >>
    >

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