Re: [PATCH v4 2/2] of: overlay: rework overlay apply and remove kfree()s

[Rob Herring]

On Wed, 20 Apr 2022 17:25:05 -0500, frowand.list@xxxxxxxxx wrote:
> From: Frank Rowand <frank.rowand@xxxxxxxx>
> 
> Fix various kfree() issues related to of_overlay_apply().
>   - Double kfree() of fdt and tree when init_overlay_changeset()
>     returns an error.
>   - free_overlay_changeset() free the root of the unflattened
>     overlay (variable tree) instead of the memory that contains
>     the unflattened overlay.
>   - For the case of a failure during applying an overlay, move kfree()
>     of new_fdt and overlay_mem into free_overlay_changeset(), which
>     is called by the function that allocated them.
>   - For the case of removing an overlay, the kfree() of new_fdt and
>     overlay_mem remains in free_overlay_changeset().
>   - Check return value of of_fdt_unflatten_tree() for error instead
>     of checking the returned value of overlay_root.
>   - When storing pointers to allocated objects in ovcs, do so as
>     near to the allocation as possible instead of in deeply layered
>     function.
> 
> More clearly document policy related to lifetime of pointers into
> overlay memory.
> 
> Double kfree()
> Reported-by: Slawomir Stepien <slawomir.stepien@xxxxxxxxx>
> 
> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
> 
> ---
> 
> Chose value of zero for OF_OVERLAY_INIT instead of -1, so that the
> value of ovcs->notify_state would be properly initialed by the
> kzalloc() of ovcs.
> 
> My first reply to this email will be a diff of this patch vs
> v3 of this patch.
> 
> Changes since v3:
>    - Instead of field kfree_unsafe in struct overlay_changeset, add
>      field notify_state to track action of most recent notification,
>      allowing the free_overlay_changeset() code to choose whether
>      to free overlay_mem and new_fdt.
>    - Add new value OF_OVERLAY_INIT to enum of_overlay_notify_action
> 
> Changes since v2:
>   - A version 2 review comment correctly said "This screams hack".
>     Restructure as listed below in response to the comment.
>   - Quit passing kfree_unsafe in function parameters, move it to
>     be a field of ovcs
>   - Quit passing a bunch of objects as function parameters, which
>     were used only for saving in ovcs
>   - Save pointers to allocated objects as near to the allocation as
>     possible instead of in a different function.
>   - Move object allocation as early in the calling stack (starting
>     at of_overlay_fdt_apply()) and move object freeing more fully
>     into free_overlay_changeset(), which is called in two places:
>     (1) on error path of applying overlay and (2) removal of an
>     overlay by of_overlay_remove().
>   - Additional notes in the overlay documentation regarding pointers
>     into overlay nodes and data.
> 
> Changes since v1:
>   - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
>   - Better document lifetime of pointers into overlay, both in overlay.c
>     and Documentation/devicetree/overlay-notes.rst
> 
>  Documentation/devicetree/overlay-notes.rst |  30 ++-
>  drivers/of/overlay.c                       | 263 ++++++++++-----------
>  include/linux/of.h                         |   3 +-
>  3 files changed, 153 insertions(+), 143 deletions(-)
> 

Applied, thanks!