Re: [PATCH v3 1/1] of: unittest: overlay: ensure proper alignment of copied FDT

[Frank Rowand <frowand.list@gmail.com>]

On 4/8/21 1:27 PM, Rob Herring wrote:
> On Thu, Apr 8, 2021 at 10:17 AM <frowand.list@gmail.com> wrote:
>>
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> 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-linux@roeck-us.net/
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> changes since version 1:
>>   - use pointer from kmalloc() for kfree() instead of using pointer that
>>     has been modified for FDT alignment
>>
>> changes since version 2:
>>   - version 1 was a work in progress version, I failed to commit the following
>>     final changes
>>   - reorder first two arguments of of_overlay_apply()
>>
>>  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..cf770452e1e5 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:       base of memory allocated to hold *@fdt_align
>> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
>>   * @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, const void *fdt_align,
>> +               struct device_node *tree, int *ovcs_id)
> 
> I think it's better if you move the kfree's out of this function. It
> would be a broken design if this function was public because you'd
> have no idea if 'fdt' could be freed or not. No reason to have that
> bad design just because it's static. If a function returns an error,
> then it should undo everything it did, but nothing more.
> 
> Rob
> 

The pattern of "If a function returns an error, then it should undo
everything it did, but nothing more" is usually what I would expect,
but overlays have more than a bit of bizarro land in their genes.

Once an overlay has been applied, the devicetree subsystem owns and
is responsible for the freeing of the related FDT.

This is noted just after calling of_overlay_apply():

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

and is also noted inside of_overlay_apply():

        /*
         * As of this point, fdt and tree belong to the overlay changeset.
         * overlay changeset code is responsible for freeing them.
         */

of_overlay_apply() is not public on purpose.  of_overlay_fdt_apply() was
created to be the public function and to provide the encapsulation of
the copy of the FDT for which memory is allocated in of_overlay_fdt_apply().

-Frank