Re: [PATCH v2 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
On 4/8/21 7:43 AM, frowand.l...@gmail.com wrote: > From: Frank Rowand > > The Devicetree standard specifies an 8 byte alignment of the FDT. > Code in libfdt expects this alignment for an FDT image in memory. > kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup() > with kmalloc(), align pointer, memcpy() to get proper alignment. > > The 4 byte alignment exposed a related bug which triggered a crash > on openrisc with: > commit 79edff12060f ("scripts/dtc: Update to upstream version > v1.6.0-51-g183df9e9c2b9") > as reported in: > https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/ > > Reported-by: Guenter Roeck > Signed-off-by: Frank Rowand > > --- > > Please review carefully, I am not yet fully awake... > > changes since version 1: > - use pointer from kmalloc() for kfree() instead of using pointer that > has been modified for FDT alignment > > drivers/of/of_private.h | 2 ++ > drivers/of/overlay.c| 28 +--- > drivers/of/unittest.c | 12 +--- > 3 files changed, 28 insertions(+), 14 deletions(-) > > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h > index d9e6a324de0a..d717efbd637d 100644 > --- a/drivers/of/of_private.h > +++ b/drivers/of/of_private.h > @@ -8,6 +8,8 @@ > * Copyright (C) 1996-2005 Paul Mackerras. > */ > > +#define FDT_ALIGN_SIZE 8 > + Wasn't there a define for that elsewhere ? > /** > * struct alias_prop - Alias property in 'aliases' node > * @link:List node to link the structure in aliases_lookup list > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > index 50bbe0edf538..e0397d70d531 100644 > --- a/drivers/of/overlay.c > +++ b/drivers/of/overlay.c > @@ -57,7 +57,7 @@ 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 > + * @fdt: base of memory allocated to hold aligned 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 > @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node > *info_node) > /** > * init_overlay_changeset() - initialize overlay changeset from overlay tree > * @ovcs:Overlay changeset to build > - * @fdt: the FDT that was unflattened to create @tree > - * @tree:Contains all the overlay fragments and overlay fixup nodes > + * @fdt: base of memory allocated to hold aligned FDT that was > unflattened to create @tree > + * @tree:Contains the overlay fragments and overlay fixup nodes > * > * Initialize @ovcs. Populate @ovcs->fragments with node information from > * the top level of @tree. The relevant top level nodes are the fragment > @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct > overlay_changeset *ovcs) > * internal documentation > * > * of_overlay_apply() - Create and apply an overlay changeset > - * @fdt: the FDT that was unflattened to create @tree > + * @fdt_align: the FDT that was unflattened to create @tree, aligned > + * @fdt: base of memory allocated to hold *@fdt_align > * @tree:Expanded overlay device tree > * @ovcs_id: Pointer to overlay changeset id > * > @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct > overlay_changeset *ovcs) > * id is returned to *ovcs_id. > */ > > -static int of_overlay_apply(const void *fdt, struct device_node *tree, > - int *ovcs_id) > +static int of_overlay_apply(const void *fdt_align, const void *fdt, > + struct device_node *tree, int *ovcs_id) Is fdt_align used anywhere in this function ? On a side note, it seems messy that of_overlay_apply() calls kfree on error. That would probably be better handled in the calling code. > { > struct overlay_changeset *ovcs; > int ret = 0, ret_revert, ret_tmp; > @@ -953,7 +954,7 @@ static int of_overlay_apply(const void *fdt, struct > device_node *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 > + * and can not free memory containing aligned fdt, aka ovcs->fdt fdt doesn't point to the aligned fdt, though. ovcs->fdt is the allocated fdt. >*/ > ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY); > if (ret) { > @@ -1014,7 +1015,8 @@ static int of_overlay_apply(const void *fdt, struct > device_node *tree, > int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, >int *ovcs_id) > { > - const void *new_fdt; > + void *new_fdt; > + void *new_fdt_align; > in
Re: [PATCH v2 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
Hi Rob, I had a git hiccup, this is not the version I meant to send. v3 coming shortly. -Frank On 4/8/21 9:43 AM, frowand.l...@gmail.com wrote: > From: Frank Rowand > > The Devicetree standard specifies an 8 byte alignment of the FDT. > Code in libfdt expects this alignment for an FDT image in memory. > kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup() > with kmalloc(), align pointer, memcpy() to get proper alignment. > > The 4 byte alignment exposed a related bug which triggered a crash > on openrisc with: > commit 79edff12060f ("scripts/dtc: Update to upstream version > v1.6.0-51-g183df9e9c2b9") > as reported in: > https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/ > > Reported-by: Guenter Roeck > Signed-off-by: Frank Rowand > > --- > > Please review carefully, I am not yet fully awake... > > changes since version 1: > - use pointer from kmalloc() for kfree() instead of using pointer that > has been modified for FDT alignment > > drivers/of/of_private.h | 2 ++ > drivers/of/overlay.c| 28 +--- > drivers/of/unittest.c | 12 +--- > 3 files changed, 28 insertions(+), 14 deletions(-) > > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h > index d9e6a324de0a..d717efbd637d 100644 > --- a/drivers/of/of_private.h > +++ b/drivers/of/of_private.h > @@ -8,6 +8,8 @@ > * Copyright (C) 1996-2005 Paul Mackerras. > */ > > +#define FDT_ALIGN_SIZE 8 > + > /** > * struct alias_prop - Alias property in 'aliases' node > * @link:List node to link the structure in aliases_lookup list > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > index 50bbe0edf538..e0397d70d531 100644 > --- a/drivers/of/overlay.c > +++ b/drivers/of/overlay.c > @@ -57,7 +57,7 @@ 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 > + * @fdt: base of memory allocated to hold aligned 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 > @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node > *info_node) > /** > * init_overlay_changeset() - initialize overlay changeset from overlay tree > * @ovcs:Overlay changeset to build > - * @fdt: the FDT that was unflattened to create @tree > - * @tree:Contains all the overlay fragments and overlay fixup nodes > + * @fdt: base of memory allocated to hold aligned FDT that was > unflattened to create @tree > + * @tree:Contains the overlay fragments and overlay fixup nodes > * > * Initialize @ovcs. Populate @ovcs->fragments with node information from > * the top level of @tree. The relevant top level nodes are the fragment > @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct > overlay_changeset *ovcs) > * internal documentation > * > * of_overlay_apply() - Create and apply an overlay changeset > - * @fdt: the FDT that was unflattened to create @tree > + * @fdt_align: the FDT that was unflattened to create @tree, aligned > + * @fdt: base of memory allocated to hold *@fdt_align > * @tree:Expanded overlay device tree > * @ovcs_id: Pointer to overlay changeset id > * > @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct > overlay_changeset *ovcs) > * id is returned to *ovcs_id. > */ > > -static int of_overlay_apply(const void *fdt, struct device_node *tree, > - int *ovcs_id) > +static int of_overlay_apply(const void *fdt_align, const void *fdt, > + struct device_node *tree, int *ovcs_id) > { > struct overlay_changeset *ovcs; > int ret = 0, ret_revert, ret_tmp; > @@ -953,7 +954,7 @@ static int of_overlay_apply(const void *fdt, struct > device_node *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 > + * and can not free memory containing aligned fdt, aka ovcs->fdt >*/ > ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY); > if (ret) { > @@ -1014,7 +1015,8 @@ static int of_overlay_apply(const void *fdt, struct > device_node *tree, > int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, >int *ovcs_id) > { > - const void *new_fdt; > + void *new_fdt; > + void *new_fdt_align; > int ret; > u32 size; > struct device_node *overlay_root; > @@ -1036,18 +1038,22 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 > overlay_fdt_size, >* Must create permanent copy of FD
[PATCH v2 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
From: Frank Rowand The Devicetree standard specifies an 8 byte alignment of the FDT. Code in libfdt expects this alignment for an FDT image in memory. kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup() with kmalloc(), align pointer, memcpy() to get proper alignment. The 4 byte alignment exposed a related bug which triggered a crash on openrisc with: commit 79edff12060f ("scripts/dtc: Update to upstream version v1.6.0-51-g183df9e9c2b9") as reported in: https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/ Reported-by: Guenter Roeck Signed-off-by: Frank Rowand --- Please review carefully, I am not yet fully awake... changes since version 1: - use pointer from kmalloc() for kfree() instead of using pointer that has been modified for FDT alignment drivers/of/of_private.h | 2 ++ drivers/of/overlay.c| 28 +--- drivers/of/unittest.c | 12 +--- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h index d9e6a324de0a..d717efbd637d 100644 --- a/drivers/of/of_private.h +++ b/drivers/of/of_private.h @@ -8,6 +8,8 @@ * Copyright (C) 1996-2005 Paul Mackerras. */ +#define FDT_ALIGN_SIZE 8 + /** * struct alias_prop - Alias property in 'aliases' node * @link: List node to link the structure in aliases_lookup list diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index 50bbe0edf538..e0397d70d531 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -57,7 +57,7 @@ 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 + * @fdt: base of memory allocated to hold aligned 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 @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node *info_node) /** * init_overlay_changeset() - initialize overlay changeset from overlay tree * @ovcs: Overlay changeset to build - * @fdt: the FDT that was unflattened to create @tree - * @tree: Contains all the overlay fragments and overlay fixup nodes + * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @tree + * @tree: Contains the overlay fragments and overlay fixup nodes * * Initialize @ovcs. Populate @ovcs->fragments with node information from * the top level of @tree. The relevant top level nodes are the fragment @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * internal documentation * * of_overlay_apply() - Create and apply an overlay changeset - * @fdt: the FDT that was unflattened to create @tree + * @fdt_align: the FDT that was unflattened to create @tree, aligned + * @fdt: base of memory allocated to hold *@fdt_align * @tree: Expanded overlay device tree * @ovcs_id: Pointer to overlay changeset id * @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * id is returned to *ovcs_id. */ -static int of_overlay_apply(const void *fdt, struct device_node *tree, - int *ovcs_id) +static int of_overlay_apply(const void *fdt_align, const void *fdt, + struct device_node *tree, int *ovcs_id) { struct overlay_changeset *ovcs; int ret = 0, ret_revert, ret_tmp; @@ -953,7 +954,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *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 +* and can not free memory containing aligned fdt, aka ovcs->fdt */ ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY); if (ret) { @@ -1014,7 +1015,8 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree, int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, int *ovcs_id) { - const void *new_fdt; + void *new_fdt; + void *new_fdt_align; int ret; u32 size; struct device_node *overlay_root; @@ -1036,18 +1038,22 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, * 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); + size += FDT_ALIGN_SIZE; + new_fdt = kmalloc(size, GFP_KERNEL); if (!new_fdt) return -ENOMEM; - of_fdt_unf