Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Pantelis Antoniou]

Hi Frank,

> On Oct 19, 2017, at 00:46 , Frank Rowand <frowand.list@xxxxxxxxx> wrote:
> 
> On 10/18/17 11:30, Rob Herring wrote:
>> On Wed, Oct 18, 2017 at 10:53 AM, Pantelis Antoniou
>> <pantelis.antoniou@xxxxxxxxxxxx> wrote:
>>> On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote:
>>>> On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull <atull@xxxxxxxxxx> wrote:
>>>>> On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>>>>>> On 10/17/17 14:46, Rob Herring wrote:
>>>>>>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull <atull@xxxxxxxxxx> wrote:
>>>>>>>> On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring <robh@xxxxxxxxxx> wrote:
>>>>>>>> 
>>>>>>>> Hi Rob,
>>>>>>>> 
>>>>>>>>> With dependencies on a statically allocated full path name converted to
>>>>>>>>> use %pOF format specifier, we can store just the basename of node, and
>>>>>>>>> the unflattening of the FDT can be simplified.
>>>>>>>>> 
>>>>>>>>> This commit will affect the remaining users of full_name. After
>>>>>>>>> analyzing these users, the remaining cases should only change some print
>>>>>>>>> messages. The main users of full_name are providing a name for struct
>>>>>>>>> resource. The resource names shouldn't be important other than providing
>>>>>>>>> /proc/iomem names.
>>>>>>>>> 
>>>>>>>>> We no longer distinguish between pre and post 0x10 dtb formats as either
>>>>>>>>> a full path or basename will work. However, less than 0x10 formats have
>>>>>>>>> been broken since the conversion to use libfdt (and no one has cared).
>>>>>>>>> The conversion of the unflattening code to be non-recursive also broke
>>>>>>>>> pre 0x10 formats as the populate_node function would return 0 in that
>>>>>>>>> case.
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>>>>>>>>> ---
>>>>>>>>> v2:
>>>>>>>>> - rebase to linux-next
>>>>>>>>> 
>>>>>>>>> drivers/of/fdt.c | 69 +++++++++-----------------------------------------------
>>>>>>>>> 1 file changed, 11 insertions(+), 58 deletions(-)
>>>>>>>> 
>>>>>>>> I've just updated to the latest next branch and am finding problems
>>>>>>>> applying overlays.   Reverting this commit alleviates things.  The
>>>>>>>> errors I get are:
>>>>>>>> 
>>>>>>>> [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
>>>>>>>> [   88.513447] OF: overlay: apply failed '/__symbols__'
>>>>>>>> [   88.518423] create_overlay: Failed to create overlay (err=-12)
>>>>>>> 
>>>>>>> Frank's series with overlay updates should fix this.
>>>>>> 
>>>>>> Yes, it does:
>>>>>> 
>>>>>>  [PATCH v3 11/12] of: overlay: remove a dependency on device node full_name
>>>>> 
>>>>> Thanks for the fast response.  I fetched the dt/next branch to test
>>>>> this but there are sufficient changes that Pantelis' "OF: DT-Overlay
>>>>> configfs interface (v7)" is broken now.  I've been adding that
>>>>> downstream since 4.4.  We're using it as an interface for applying
>>>>> overlays to program FPGAs.  If we fix it again, is there any chance
>>>>> that can go upstream now?
>>>> 
>>>> With a drive-by posting once every few years, no.
>>>> 
>>> 
>>> I take offense to that. There's nothing changed in the patch for years.
>>> Reposting the same patch without changes would achieve nothing.
>> 
>> Are you still expecting review comments on it or something?
>> Furthermore, If something is posted infrequently, then I'm not
>> inclined to comment or care if the next posting is going to be after I
>> forget what I previously said (which is not very long).
>> 
>> I'm just saying, don't expect to forward port, post and it will be
>> accepted. Below is minimally one of the issues that needs to be
>> addressed.
>> 
> 
> 
>>>> The issue remains that the kernel is not really setup to deal with any
>>>> random property or node to be changed at any point in run-time. I
>>>> think there needs to be some restrictions around what the overlays can
>>>> touch. We can't have it be wide open and then lock things down later
>>>> and break users.
> 
> That paragraph is key to any discussion of accepting code to apply overlays.
> Solving that issue has been stated to be a gating factor for such code from
> the beginning of overlay development.
> 

Overlays are not only used only for cases where you have external expansion boards, or
FPGAs where every change is contained under a few designated nodes, so thatâs why Iâm
pushing for a in-kernel validator thatâs more flexible than a single whitelist.

An eBPF validator would handle a whitelist trivially easy, and would be flexible enough
for any other more complicated use case.

> 
>>>> One example of what you could do is you can only add
>>>> sub-trees to whitelisted nodes. That's probably acceptable for your
>>>> usecase.
>>>> 
>>> 
>>> Defining what can and what cannot be changed is not as trivial as a
>>> list of white-listed nodes.
>> 
>> No, but we have to start somewhere and we are not starting with any
>> change allowed anywhere at anytime. If that is what people want, then
>> they are going to get to maintain that out of tree.
>> 
>>> In some cases there is a whole node hierarchy being inserted (like in
>>> a FPGA).
>> 
>> Yes, so you'd have a target fpga region. That sounds fine to me. Maybe
>> its not a static whitelist, but drivers have to register target
>> nodes/paths.
>> 
>>> In others, it's merely changing a status property to "okay" and
>>> a few device parameters.
>> 
>> That seems fine too. Disabled nodes could be allowed. But what if you
>> add/change properties on a node that is not disabled? Once a node is
>> enabled, who is responsible for registering the device?
>> 
>> What about changing a node from enabled to disabled? The kernel would
>> need to handle that or not allow it.
>> 
>>> The real issue is that the kernel has no way to verify that a given
>>> device tree, either at boot time or at overlay application time, is
>>> correct.
>>> 
>>> When the tree is wrong at boot-time you'll hang (if you're lucky).
>>> If the tree is wrong at run-time you'll get some into some unidentified
>>> funky state.
>> 
>> Or have some security hole or a mechanism for userspace to crash the system.
>> 
>>> Finally what is, and what is not 'correct' is not for the kernel to
>>> decide arbitrarily, it's a matter of policy, different for each
>>> use-case.
>> 
>> It is if the kernel will break doing so.
>> 
>> Rob
>> 
>