Re: [PATCH v2 1/1] mm/percpu.c: fix potential memory leakage for pcpu_embed_first_chunk()

[Tejun Heo]

Hello,

Sorry about the delay, have been traveling.

On Fri, Sep 30, 2016 at 07:30:28PM +0800, zijun_hu wrote:
> From: zijun_hu <zijun_hu@xxxxxxx>
> 
> it will cause memory leakage for pcpu_embed_first_chunk() to go to
> label @out_free if the chunk spans over 3/4 VMALLOC area. all memory
> are allocated and recorded into array @areas for each CPU group, but
> the memory allocated aren't be freed before returning after going to
> label @out_free.
> 
> in order to fix this bug, we check chunk spanned area immediately
> after completing memory allocation for all CPU group, we go to label
> @out_free_areas other than @out_free to free all memory allocated if
> the checking is failed.

It's often helpful to include what the impact of the bug is (here it's
fairly inconsequential) so that people reading the changelog can
decide how important the commit is.

> in order to verify the approach, we dump all memory allocated then
> enforce the jump then dump all memory freed, the result is okay after
> checking whether we free all memory we allocate in this function.
> 
> BTW, The approach is chosen after thinking over the below scenes
>  - we don't go to label @out_free directly to fix this issue since we
>    maybe free several allocated memory blocks twice
>  - the aim of jumping after pcpu_setup_first_chunk() is bypassing free
>    usable memory other than handling error, moreover, the function does
>    not return error code in any case, it either panics due to BUG_ON()
>    or return 0.
> 
> Signed-off-by: zijun_hu <zijun_hu@xxxxxxx>
> Tested-by: zijun_hu <zijun_hu@xxxxxxx>

Generally looks good to me but it's on top of the max_distance fix
patch which isn't applied yet and I think is pending update.  Can you
please update the previous patch or merge the two into one patch?

> @@ -1979,7 +1979,8 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, size_t dyn_size,
>  		goto out_free;
>  	}
>  
> -	/* allocate, copy and determine base address */
> +	/* allocate, copy and determine base address & max_distance */
> +	j = 0;

It'd be great if we can use a variable name which is more descriptive.
Something like highest_idx, maybe?

Thanks.

-- 
tejun