From: Frank Rowand <frowand.list@gmail.com>
To: Stephen Boyd <stephen.boyd@linaro.org>, Rob Herring <robh+dt@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [RFC/PATCH] of: Mark property::value as const
Date: Thu, 23 Feb 2017 15:25:59 -0800 [thread overview]
Message-ID: <58AF6F87.4040005@gmail.com> (raw)
In-Reply-To: <58AF6B88.6020709@gmail.com>
On 02/23/17 15:08, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory. This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.
>
> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
>
>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
>
> Yes, the resolver code needs to adjust phandle values.
>
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node. At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree. These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node. A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.
If I remove the various phandle properties from the live tree,
the one place I can not inspect for impact is the live tree that
is exposed at /proc/device-tree/ I do not know whether that is
a problem or not.
-Frank
WARNING: multiple messages have this Message-ID (diff)
From: Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Stephen Boyd
<stephen.boyd-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [RFC/PATCH] of: Mark property::value as const
Date: Thu, 23 Feb 2017 15:25:59 -0800 [thread overview]
Message-ID: <58AF6F87.4040005@gmail.com> (raw)
In-Reply-To: <58AF6B88.6020709-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 02/23/17 15:08, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory. This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.
>
> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
>
>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
>
> Yes, the resolver code needs to adjust phandle values.
>
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node. At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree. These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node. A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.
If I remove the various phandle properties from the live tree,
the one place I can not inspect for impact is the live tree that
is exposed at /proc/device-tree/ I do not know whether that is
a problem or not.
-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: frowand.list@gmail.com (Frank Rowand)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC/PATCH] of: Mark property::value as const
Date: Thu, 23 Feb 2017 15:25:59 -0800 [thread overview]
Message-ID: <58AF6F87.4040005@gmail.com> (raw)
In-Reply-To: <58AF6B88.6020709@gmail.com>
On 02/23/17 15:08, Frank Rowand wrote:
> On 02/13/17 18:50, Stephen Boyd wrote:
>> The 'blob' we pass into populate_properties() is marked as const,
>> but we cast that const away when we assign the result of
>> fdt_getprop_by_offset() to pp->value. Let's mark value as const
>> instead, so that code can't mistakenly write to the value of the
>> property that we've so far advertised as const.
>
> Instead of struct property field value being a pointer into the
> FDT, I would rather copy the data to newly allocated memory and
> have value be a pointer to that memory. This is required if we
> want to make /sys/firmware/fdt optional, which would allow us to
> free the memory containing the initial boot FDT.
>
> I also do not want overlay live subtrees to have any pointers
> into the FDT that was used to populate the overlay, so copying
> the data solves that problem also.
>
>
>> Unfortunately, this exposes a problem with the fdt resolver code,
>> where we overwrite the value member of properties of phandles to
>> update them with their final value. Add a comment for now to
>> indicate where we're potentially writing over const data.
>
> Yes, the resolver code needs to adjust phandle values.
>
> I think I can get rid of the resolver modifying the various phandle
> values, and instead just modify the phandle value in struct
> device_node. At the same time, I think I can also remove all
> instances of the phandle properties ('linux,phandle', 'ibm,phandle',
> 'phandle') in the live tree. These properties should not be
> accessed directly by any code outside of the device tree framework
> since the phandle is located in the struct device_node. A quick
> grep does not show any such accesses of the phandle properties,
> but I want to look more closely.
If I remove the various phandle properties from the live tree,
the one place I can not inspect for impact is the live tree that
is exposed at /proc/device-tree/ I do not know whether that is
a problem or not.
-Frank
next prev parent reply other threads:[~2017-02-23 23:26 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-14 2:50 [RFC/PATCH] of: Mark property::value as const Stephen Boyd
2017-02-14 2:50 ` Stephen Boyd
2017-02-14 2:50 ` Stephen Boyd
2017-02-23 13:39 ` Rob Herring
2017-02-23 13:39 ` Rob Herring
2017-02-23 13:39 ` Rob Herring
2017-02-23 19:54 ` Frank Rowand
2017-02-23 19:54 ` Frank Rowand
2017-02-23 19:54 ` Frank Rowand
2017-02-23 20:58 ` Frank Rowand
2017-02-23 20:58 ` Frank Rowand
2017-02-23 20:58 ` Frank Rowand
2017-02-23 22:09 ` Rob Herring
2017-02-23 22:09 ` Rob Herring
2017-02-23 22:09 ` Rob Herring
2017-02-23 23:23 ` Frank Rowand
2017-02-23 23:23 ` Frank Rowand
2017-02-23 23:23 ` Frank Rowand
2017-02-23 21:47 ` Frank Rowand
2017-02-23 21:47 ` Frank Rowand
2017-02-23 23:08 ` Frank Rowand
2017-02-23 23:08 ` Frank Rowand
2017-02-23 23:08 ` Frank Rowand
2017-02-23 23:25 ` Frank Rowand [this message]
2017-02-23 23:25 ` Frank Rowand
2017-02-23 23:25 ` Frank Rowand
2017-02-23 23:38 ` Rob Herring
2017-02-23 23:38 ` Rob Herring
2017-02-23 23:38 ` Rob Herring
2017-03-12 6:27 ` Frank Rowand
2017-03-12 6:27 ` Frank Rowand
2017-03-12 6:27 ` Frank Rowand
2017-03-14 19:23 ` Stephen Boyd
2017-03-14 19:23 ` Stephen Boyd
2017-03-14 19:23 ` Stephen Boyd
2017-03-17 7:46 ` [PATCH] " kbuild test robot
2017-03-17 7:46 ` kbuild test robot
2017-03-17 7:46 ` kbuild test robot
2017-03-17 9:17 ` kbuild test robot
2017-03-17 9:17 ` kbuild test robot
2017-03-17 9:17 ` kbuild test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=58AF6F87.4040005@gmail.com \
--to=frowand.list@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=stephen.boyd@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.