On 03/01/18 12:59, Laurent Pinchart wrote:
> Hi Frank,
>
> Thank you for the patch.
>
> On Thursday, 1 March 2018 20:00:53 EET [email protected] wrote:
>> From: Frank Rowand <[email protected]>
>>
>> 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.
>>
>> Update unittest.c to use of_overlay_fdt_apply() instead of
>> of_overlay_apply().
>>
>> Move overlay fragments from artificial locations in
>> drivers/of/unittest-data/tests-overlay.dtsi into one devicetree
>> source file per overlay. This led to changes in
>> drivers/of/unitest-data/Makefile and drivers/of/unitest.c.
>>
>> - Add overlay directives to the overlay devicetree source files so
>> that dtc will compile them as true overlays into one FDT data
>> chunk per overlay.
>>
>> - Set CFLAGS for drivers/of/unittest-data/testcases.dts so that
>> symbols will be generated for overlay resolution of overlays
>> that are no longer artificially contained in testcases.dts
>>
>> - Unflatten and apply each unittest overlay FDT using
>> of_overlay_fdt_apply().
>>
>> - Enable the of_resolve_phandles() check for whether the unflattened
>> overlay is detached. This check was previously disabled because the
>> overlays from tests-overlay.dtsi were not unflattened into detached
>> trees.
>>
>> - Other changes to unittest.c infrastructure to manage multiple test
>> FDTs built into the kernel image (access by name instead of
>> arbitrary number).
>>
>> - of_unittest_overlay_high_level(): previously unused code to add
>> properties from the overlay_base devicetree to the live tree
>> was triggered by the restructuring of tests-overlay.dtsi and thus
>> testcases.dts. This exposed two bugs: (1) the need to dup a
>> property before adding it, and (2) property 'name' is
>> auto-generated in the unflatten code and thus will be a duplicate
>> in the __symbols__ node - do not treat this duplicate as an error.
>>
>> Signed-off-by: Frank Rowand <[email protected]>
>> ---
>>
>> There are checkpatch warnings. I have reviewed them and feel they
>> can be ignored. They are "line over 80 characters" for either
>> pre-existing long lines, or lines in devicetree source files.
>>
>> Changes from v3:
>> - OF_OVERLAY: add select OF_FLATTREE
>>
>> Changes from v1:
>> - rebase on v4.16-rc1
>>
>> drivers/of/Kconfig | 1 +
>> drivers/of/of_private.h | 1 +
>> drivers/of/overlay.c | 107 +++++++++-
>> drivers/of/resolver.c | 6 -
>> drivers/of/unittest-data/Makefile | 28 ++-
>> drivers/of/unittest-data/overlay_0.dts | 14 ++
>> drivers/of/unittest-data/overlay_1.dts | 14 ++
>> drivers/of/unittest-data/overlay_10.dts | 34 ++++
>> drivers/of/unittest-data/overlay_11.dts | 34 ++++
>> drivers/of/unittest-data/overlay_12.dts | 14 ++
>> drivers/of/unittest-data/overlay_13.dts | 14 ++
>> drivers/of/unittest-data/overlay_15.dts | 35 ++++
>> drivers/of/unittest-data/overlay_2.dts | 14 ++
>> drivers/of/unittest-data/overlay_3.dts | 14 ++
>> drivers/of/unittest-data/overlay_4.dts | 23 +++
>> drivers/of/unittest-data/overlay_5.dts | 14 ++
>> drivers/of/unittest-data/overlay_6.dts | 15 ++
>> drivers/of/unittest-data/overlay_7.dts | 15 ++
>> drivers/of/unittest-data/overlay_8.dts | 15 ++
>> drivers/of/unittest-data/overlay_9.dts | 15 ++
>> drivers/of/unittest-data/tests-overlay.dtsi | 213 --------------------
>> drivers/of/unittest.c | 294 ++++++++++++------------
>> include/linux/of.h | 7 -
>> 23 files changed, 552 insertions(+), 389 deletions(-)
>> create mode 100644 drivers/of/unittest-data/overlay_0.dts
>> create mode 100644 drivers/of/unittest-data/overlay_1.dts
>> create mode 100644 drivers/of/unittest-data/overlay_10.dts
>> create mode 100644 drivers/of/unittest-data/overlay_11.dts
>> create mode 100644 drivers/of/unittest-data/overlay_12.dts
>> create mode 100644 drivers/of/unittest-data/overlay_13.dts
>> create mode 100644 drivers/of/unittest-data/overlay_15.dts
>> create mode 100644 drivers/of/unittest-data/overlay_2.dts
>> create mode 100644 drivers/of/unittest-data/overlay_3.dts
>> create mode 100644 drivers/of/unittest-data/overlay_4.dts
>> create mode 100644 drivers/of/unittest-data/overlay_5.dts
>> create mode 100644 drivers/of/unittest-data/overlay_6.dts
>> create mode 100644 drivers/of/unittest-data/overlay_7.dts
>> create mode 100644 drivers/of/unittest-data/overlay_8.dts
>> create mode 100644 drivers/of/unittest-data/overlay_9.dts
>>
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 783e0870bd22..ad3fcad4d75b 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -92,6 +92,7 @@ config OF_RESOLVE
>> config OF_OVERLAY
>> bool "Device Tree overlays"
>> select OF_DYNAMIC
>> + select OF_FLATTREE
>> select OF_RESOLVE
>> help
>> Overlays are a method to dynamically modify part of the kernel's
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index 0c609e7d0334..6e39dce3a979 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -77,6 +77,7 @@ static inline void __of_detach_node_sysfs(struct
>> device_node *np) {} #endif
>>
>> #if defined(CONFIG_OF_OVERLAY)
>> +int of_overlay_fdt_apply(void *overlay_fdt, int *ovcs_id);
>
> As discussed on IRC, could you move this to include/linux/of.h ?
Yes, got it.
>
>> void of_overlay_mutex_lock(void);
>> void of_overlay_mutex_unlock(void);
>> #else
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 3397d7642958..5f6c5569e97d 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -12,10 +12,12 @@
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/of_fdt.h>
>> #include <linux/string.h>
>> #include <linux/ctype.h>
>> #include <linux/errno.h>
>> #include <linux/slab.h>
>> +#include <linux/libfdt.h>
>> #include <linux/err.h>
>> #include <linux/idr.h>
>>
>> @@ -33,7 +35,9 @@ struct fragment {
>>
>> /**
>> * struct overlay_changeset
>> + * @id: changeset identifier
>> * @ovcs_list: list on which we are located
>> + * @fdt: FDT that was unflattened to create @overlay_tree
>> * @overlay_tree: expanded device tree that contains the fragment nodes
>> * @count: count of fragment structures
>> * @fragments: fragment nodes in the overlay expanded device
>> tree
>> @@ -43,6 +47,7 @@ struct fragment {
>> struct overlay_changeset {
>> int id;
>> struct list_head ovcs_list;
>> + const void *fdt;
>> struct device_node *overlay_tree;
>> int count;
>> struct fragment *fragments;
>> @@ -503,7 +508,8 @@ static struct device_node *find_target_node(struct
>> device_node *info_node)
>>
>> /**
>> * init_overlay_changeset() - initialize overlay changeset from overlay
>> tree
>> - * @ovcs Overlay changeset to build
>> + * @ovcs: Overlay changeset to build
>> + * @fdt: the FDT that was unflattened to create @tree
>> * @tree: Contains all the overlay fragments and overlay fixup nodes
>> *
>> * Initialize @ovcs. Populate @ovcs->fragments with node information from
>> @@ -514,7 +520,7 @@ static struct device_node *find_target_node(struct
>> device_node *info_node) * detected in @tree, or -ENOSPC if idr_alloc()
>> error.
>> */
>> static int init_overlay_changeset(struct overlay_changeset *ovcs,
>> - struct device_node *tree)
>> + const void *fdt, struct device_node *tree)
>> {
>> struct device_node *node, *overlay_node;
>> struct fragment *fragment;
>> @@ -535,6 +541,7 @@ static int init_overlay_changeset(struct
>> overlay_changeset *ovcs, pr_debug("%s() tree is not root\n", __func__);
>>
>> ovcs->overlay_tree = tree;
>> + ovcs->fdt = fdt;
>>
>> INIT_LIST_HEAD(&ovcs->ovcs_list);
>>
>> @@ -606,6 +613,7 @@ static int init_overlay_changeset(struct
>> overlay_changeset *ovcs, }
>>
>> if (!cnt) {
>> + pr_err("no fragments or symbols in overlay\n");
>> ret = -EINVAL;
>> goto err_free_fragments;
>> }
>> @@ -642,11 +650,24 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs) }
>> kfree(ovcs->fragments);
>>
>> + /*
>> + * 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
>> + */
>> +
>
> I assume you'll fix it at some point ? :-)
Long term project. It's not easy. But that is my intent.
>> kfree(ovcs);
>> }
>>
>> -/**
>> +/*
>> + * internal documentation
>> + *
>> * of_overlay_apply() - Create and apply an overlay changeset
>> + * @fdt: the FDT that was unflattened to create @tree
>> * @tree: Expanded overlay device tree
>> * @ovcs_id: Pointer to overlay changeset id
>> *
>> @@ -685,21 +706,29 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs) * id is returned to *ovcs_id.
>> */
>>
>> -int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>> +static int of_overlay_apply(const void *fdt, struct device_node *tree,
>> + int *ovcs_id)
>> {
>> struct overlay_changeset *ovcs;
>> int ret = 0, ret_revert, ret_tmp;
>>
>> - *ovcs_id = 0;
>> + /*
>> + * As of this point, fdt and tree belong to the overlay changeset.
>> + * overlay changeset code is responsible for freeing them.
>> + */
>>
>> if (devicetree_corrupt()) {
>> pr_err("devicetree state suspect, refuse to apply overlay\n");
>> + kfree(fdt);
>> + kfree(tree);
>> ret = -EBUSY;
>> goto out;
>> }
>>
>> ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
>> if (!ovcs) {
>> + kfree(fdt);
>> + kfree(tree);
>> ret = -ENOMEM;
>> goto out;
>> }
>> @@ -709,12 +738,17 @@ int of_overlay_apply(struct device_node *tree, int
>> *ovcs_id)
>>
>> ret = of_resolve_phandles(tree);
>> if (ret)
>> - goto err_free_overlay_changeset;
>> + goto err_free_tree;
>>
>> - ret = init_overlay_changeset(ovcs, tree);
>> + ret = init_overlay_changeset(ovcs, fdt, tree);
>> if (ret)
>> - goto err_free_overlay_changeset;
>> + goto err_free_tree;
>>
>> + /*
>> + * after overlay_notify(), ovcs->overlay_tree related pointers may have
>> + * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
>> + * and can not free fdt, aka ovcs->fdt
>> + */
>> ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>> if (ret) {
>> pr_err("overlay changeset pre-apply notify error %d\n", ret);
>> @@ -754,6 +788,9 @@ int of_overlay_apply(struct device_node *tree, int
>> *ovcs_id)
>>
>> goto out_unlock;
>>
>> +err_free_tree:
>> + kfree(tree);
>> +
>> err_free_overlay_changeset:
>> free_overlay_changeset(ovcs);
>>
>> @@ -766,7 +803,59 @@ int of_overlay_apply(struct device_node *tree, int
>> *ovcs_id)
>>
>> return ret;
>> }
>> -EXPORT_SYMBOL_GPL(of_overlay_apply);
>> +
>> +int of_overlay_fdt_apply(void *overlay_fdt, int *ovcs_id)
>
> Can you make the overlay_fdt pointer const ?
It looks like I should be able to.
> Apart from that the patch looks OK to me.
>
>> +{
>> + const void *new_fdt;
>> + int ret;
>> + u32 size;
>> + struct device_node *overlay_root;
>> +
>> + *ovcs_id = 0;
>> + ret = 0;
>> +
>> + if (fdt_check_header(overlay_fdt)) {
>> + pr_err("Invalid overlay_fdt header\n");
>> + return -EINVAL;
>> + }
>> +
>> + size = fdt_totalsize(overlay_fdt);
>> +
>> + /*
>> + * Must create permanent copy of FDT because of_fdt_unflatten_tree()
>> + * will create pointers to the passed in FDT in the unflattened tree.
>> + */
>> + new_fdt = kmemdup(overlay_fdt, size, GFP_KERNEL);
>> + if (!new_fdt)
>> + return -ENOMEM;
>> +
>> + of_fdt_unflatten_tree(new_fdt, NULL, &overlay_root);
>> + if (!overlay_root) {
>> + pr_err("unable to unflatten overlay_fdt\n");
>> + ret = -EINVAL;
>> + goto out_free_new_fdt;
>> + }
>> +
>> + ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
>> + if (ret < 0) {
>> + /*
>> + * new_fdt and overlay_root now belong to the overlay
>> + * changeset.
>> + * overlay changeset code is responsible for freeing them.
>> + */
>> + goto out;
>> + }
>> +
>> + return 0;
>> +
>> +
>> +out_free_new_fdt:
>> + kfree(new_fdt);
>> +
>> +out:
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
>>
>> /*
>> * Find @np in @tree.
>
