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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux