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

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

On 4/19/22 13:56, Rob Herring wrote:
> On Mon, Apr 18, 2022 at 07:52:41PM -0500, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> 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().
> 
> You never set kfree_unsafe back to false anywhere, so after removing you 
> still leak memory.

Embarrassing and ironic that I would leave that line out.  There should
be an ovcs->kfree_unsafe = false immediately before
"overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE)" in of_overlay_remove().

> 
>>   - 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@nokia.com>
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> 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
> 
> What I meant was store the notifier state and from that imply when kfree 
> is unsafe. Something like this patch on top of yours (untested and still 
> some kfree_unsafe comments need to be updated):

Got it.  I like the approach you show below.  Patch v4 should be appearing
Tuesday.

-Frank

> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 3072dfeca8e8..53c616f576d2 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -63,7 +63,7 @@ struct fragment {
>   * @count:		count of fragment structures
>   * @fragments:		fragment nodes in the overlay expanded device tree
>   * @symbols_fragment:	last element of @fragments[] is the  __symbols__ node
> - * @kfree_unsafe:	pointers into the @new_fdt or @overlay_mem may exist
> + * @notify_state:	the last successful notifier called
>   * @cset:		changeset to apply fragments to live device tree
>   */
>  struct overlay_changeset {
> @@ -75,7 +75,7 @@ struct overlay_changeset {
>  	int count;
>  	struct fragment *fragments;
>  	bool symbols_fragment;
> -	bool kfree_unsafe;
> +	enum of_overlay_notify_action notify_state;
>  	struct of_changeset cset;
>  };
>  
> @@ -183,6 +183,8 @@ static int overlay_notify(struct overlay_changeset *ovcs,
>  		}
>  	}
>  
> +	ovcs->notify_state = action;
> +
>  	return 0;
>  }
>  
> @@ -831,6 +833,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs)
>  	}
>  
>  	ovcs->count = cnt;
> +	ovcs->notify_state = OF_OVERLAY_INIT;
>  
>  	return 0;
>  
> @@ -866,15 +869,14 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  	 * allowed to retain pointers into the overlay devicetree other
>  	 * than during the window between OF_OVERLAY_PRE_APPLY overlay
>  	 * notifiers and the OF_OVERLAY_POST_REMOVE overlay notifiers.
> -	 * During the window, ovcs->kfree_unsafe will be true.
>  	 *
>  	 * A memory leak will occur here if ovcs->kfree_unsafe is true.
>  	 */
>  
> -	if (!ovcs->kfree_unsafe)
> +	if (ovcs->notify_state == OF_OVERLAY_INIT || ovcs->notify_state == OF_OVERLAY_POST_REMOVE) {
>  		kfree(ovcs->overlay_mem);
> -	if (!ovcs->kfree_unsafe)
>  		kfree(ovcs->new_fdt);
> +	}
>  	kfree(ovcs);
>  }
>  
> @@ -926,12 +928,6 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
>  	if (ret)
>  		goto out;
>  
> -	/*
> -	 * After overlay_notify(), ovcs->overlay_root related pointers may have
> -	 * leaked to drivers, so can not kfree() ovcs->overlay_mem and
> -	 * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
> -	 */
> -	ovcs->kfree_unsafe = true;
>  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>  	if (ret) {
>  		pr_err("overlay changeset pre-apply notify error %d\n", ret);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 04971e85fbc9..b7b095593eec 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1543,6 +1543,7 @@ static inline bool of_device_is_system_power_controller(const struct device_node
>   */
>  
>  enum of_overlay_notify_action {
> +	OF_OVERLAY_INIT = -1,
>  	OF_OVERLAY_PRE_APPLY = 0,
>  	OF_OVERLAY_POST_APPLY,
>  	OF_OVERLAY_PRE_REMOVE,