Re: [PATCH v4 1/4] of: change overlay apply input data from unflattened to FDT

From: Laurent Pinchart
Date: Thu Mar 01 2018 - 15:58:24 EST

Hi Frank,

Thank you for the patch.

On Thursday, 1 March 2018 20:00:53 EET frowand.list@xxxxxxxxx wrote:
> From: Frank Rowand <frank.rowand@xxxxxxxx>
>
> 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 <frank.rowand@xxxxxxxx>
> ---
>
> 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 ?

> 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 ? :-)

> 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 ?

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.

--
Regards,

Laurent Pinchart