Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     frowand.list@gmail.com
Cc:     Rob Herring <robh+dt@kernel.org>, pantelis.antoniou@konsulko.com,
        Pantelis Antoniou <panto@antoniou-consulting.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        geert@linux-m68k.org, laurent.pinchart+renesas@ideasonboard.com
Subject: Re: [PATCH v4 1/4] of: change overlay apply input data from unflattened to FDT
Date:   Thu, 01 Mar 2018 22:59:08 +0200
Message-ID: <18109035.ZrMSJvtVAx@avalon>
Organization: Ideas on Board Oy
In-Reply-To: <1519927256-4868-2-git-send-email-frowand.list@gmail.com>
References: <1519927256-4868-1-git-send-email-frowand.list@gmail.com> <1519927256-4868-2-git-send-email-frowand.list@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Frank,

Thank you for the patch.

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