From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752141AbdJSIlZ (ORCPT ); Thu, 19 Oct 2017 04:41:25 -0400 Received: from mail-wr0-f194.google.com ([209.85.128.194]:47352 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751860AbdJSIlU (ORCPT ); Thu, 19 Oct 2017 04:41:20 -0400 X-Google-Smtp-Source: ABhQp+RIvBNA7CPztwa1UjlMf8XeWZvHCQ4PlsbG6JfrTb05NkC/PvE50RYvHAwQuKZ7el/3JDe6jQ== Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name From: Pantelis Antoniou In-Reply-To: <59E7CBAF.9080501@gmail.com> Date: Thu, 19 Oct 2017 11:41:16 +0300 Cc: Rob Herring , Alan Tull , "devicetree@vger.kernel.org" , Michael Ellerman , linuxppc-dev , linux-kernel , Benjamin Herrenschmidt , Paul Mackerras , David Laight , linux-fpga@vger.kernel.org Message-Id: References: <20170821151651.25096-1-robh@kernel.org> <20170821151651.25096-6-robh@kernel.org> <59E69786.2030406@gmail.com> <1508341985.25643.12.camel@hp800z> <59E7CBAF.9080501@gmail.com> To: Frank Rowand X-Mailer: Apple Mail (2.3273) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v9J8fUXG027047 Hi Frank, > On Oct 19, 2017, at 00:46 , Frank Rowand wrote: > > On 10/18/17 11:30, Rob Herring wrote: >> On Wed, Oct 18, 2017 at 10:53 AM, Pantelis Antoniou >> wrote: >>> On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote: >>>> On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull wrote: >>>>> On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand wrote: >>>>>> On 10/17/17 14:46, Rob Herring wrote: >>>>>>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull wrote: >>>>>>>> On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring 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 >>>>>>>>> --- >>>>>>>>> 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 >> >