Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210408151736.2216238-1-frowand.list@gmail.com>
In-Reply-To: <20210408151736.2216238-1-frowand.list@gmail.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Thu, 8 Apr 2021 13:27:22 -0500
Message-ID: <CAL_JsqLJP5Q8R+4yQOhiG_xTA6CHLSWfu-0hFzmZtiv96Do_eA@mail.gmail.com>
Subject: Re: [PATCH v3 1/1] of: unittest: overlay: ensure proper alignment of
 copied FDT
To:     Frank Rowand <frowand.list@gmail.com>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
        devicetree@vger.kernel.org,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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