libfdt: overlay: Skip phandles not in subnodes

[pelwell]

A Raspberry Pi user reported (raspberrypi/firmware#1718) that some overlays that work with the Pi tools can't be applied by fdtoverlay (even ignoring the non-standard parameter stuff). After some digging I traced the problem to unwanted phandles being copied into the base dtb, overwriting an existing phandle and breaking references to that node.

This patch filters out phandles in the outer level of overlay nodes (i.e. not in subnodes).

[pelwell]

Hmm - am I right in thinking that an overlay:

1. Is allowed to add a label to an existing node that doesn't already have one?
2. Is not really allowed to add a second label to a node that does have one?
3. Has no way to restrict the labels that are copied to the base dtb (ignoring those that reference something that never appears in the base dtb)?
4. As a result, my patch is not acceptable?

[ukleinek]

You might want to check #98 which (I think) addresses the problem you describe in a more robust way.

[dgibson]

Hmm - am I right in thinking that an overlay:

1. Is allowed to add a label to an existing node that doesn't already have one?

Yes, that's allowed.

2. Is not really allowed to add a second label to a node that does have one?

Hm, I think that should be allowed. In general nodes are allowed to have multiple labels.

3. Has no way to restrict the labels that are copied to the base dtb (ignoring those that reference something that never appears in the base dtb)?

I'm not really sure what you mean by this. If the label is not copied to the base dtb it has no effect.

Note that it's not the act of adding the label that causes the extra phandle to be assigned. The extra phandle is assigned when the node is referenced by a label that dtc thinks is local at compile time, but turns out not to be at overlay apply time.

4. As a result, my patch is not acceptable?

No, but I'm afraid it's not acceptable because it's very fragile. Although existing nodes being the top-level of a single overlay fragment is the most common case, it's entirely possible for overlay fragments to make tweaks to an entire tree of existing nodes, so checking the depth doesn't really help us.

[dgibson]

So, there's a real issue, but how to deal with it is pretty tricky. This patch doesn't work because it relies on an inaccurate heuristic to work out whether we can apply the phandle change. #98 sent by @ukleinek doesn't work because it relies on dynamic allocation which is not allowed in libfdt.

Option 1

In fdt_apply_overlay() simply prohibit rewriting phandles: throw an error if attempting to update an existing phandle property in the base tree. This would effectively make any overlay which refers to a label defined in that overlay illegal if applied to a tree which already has a phandle on that node. Roughly speaking if you want to refer to a node in a overlay you must use it's original base tree label, not a new label. We can't tell that at overlay compile time, so it's pretty awful.

Option 1a

Ban overlays which rewrite phandles as in Option 1, also change the order of things so we merge the symbol tables before merging the trees themselves. On the dtc side, completely deprecate use of __local_fixups__. Where we would have emitted a local fixup, instead use a regular fixup based on the label added in the overlay.

• Newly generated overlays wouldn't work with older libfdt (pretty sure existing libfdt won't resolve fixups to labels that were only added in the same overlay). That's pretty bad.
• What if the overlay is using a path reference, so there is new label (I suspect this might already be broken though).
• Not entirely sure I haven't missed something else that will break this.

Option 2

Resolve the new and existing phandles at overlay apply time. I think it's possible, but it's pretty darn complex.

1. For each local_fixup in the overlay (O(n)-ish)
   1. Find the target property P of the fixup in the overlay (O(n)-ish)
   2. Read the (overlay numbering) phandle from the fixed up property
   3. Find the node N in the overlay with that phandle (O(n)-ish)
   4. Find the corresponding node B in the base tree, if any (O(n)-ish, though pretty awkward, I think)
   5. If B exists and has a phandle, modify P to refer to that base tree phandle, and clobber the local fixup entry (e.g. overwrite offset with 0xffffffff)
2. Adjust remaining phandles and local fixups (we can think of these as truly local fixups) by delta as we do now, skipping any local fixups we clobbered above.
3. Merge trees, but skip updates of phandle properties

I think that will do it. It's at least O(n^2)-ish, but that might already be true of overlay application. I'm a bit worried I've missed something that will make it O(n^3) or worse.

Maybe there's a clever way to make that more elegant, but I haven't spotted it.

[ukleinek]

I think this problem is solved on current HEAD, isn't it?