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