Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751907AbcJBAe2 (ORCPT ); Sat, 1 Oct 2016 20:34:28 -0400 Received: from sender153-mail.zoho.com ([74.201.84.153]:25417 "EHLO sender153-mail.zoho.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751164AbcJBAeU (ORCPT ); Sat, 1 Oct 2016 20:34:20 -0400 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=zapps768; d=zoho.com; h=subject:to:references:cc:from:message-id:date:user-agent:mime-version:in-reply-to:content-type; b=VDzpRZHKxpwKkz9PXBB5ZWsh02yHKNEiSSX6sAFO6lQ4MmNM4zqQfieYdCs9b/9riwrMVHniM+xv 5fA1ygUB11x1wuUsV61Wxi3WxTyEL/MJhCVHhU+D5BlBW6KB4zuu Subject: Re: [PATCH v2 1/1] mm/percpu.c: fix potential memory leakage for pcpu_embed_first_chunk() To: tj@kernel.org, akpm@linux-foundation.org References: Cc: zijun_hu@htc.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, cl@linux.com From: zijun_hu Message-ID: Date: Sun, 2 Oct 2016 08:34:07 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4277 Lines: 114 Hi Tejun, as we discussed, i include some discussion content in the commit message. could you give some new comments or acknowledgment for this patch? On 2016/9/30 19:30, zijun_hu wrote: > From: zijun_hu > > 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. > > 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 > Tested-by: zijun_hu > --- > this patch is based on mmotm/linux-next branch so can be > applied to them directly > > Changes in v2: > - more detailed commit message is provided as discussed > with tj@kernel.org > > mm/percpu.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > > diff --git a/mm/percpu.c b/mm/percpu.c > index 41d9d0b35801..7a5dae185ce1 100644 > --- a/mm/percpu.c > +++ b/mm/percpu.c > @@ -1963,7 +1963,7 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, size_t dyn_size, > struct pcpu_alloc_info *ai; > size_t size_sum, areas_size; > unsigned long max_distance; > - int group, i, rc; > + int group, i, j, rc; > > ai = pcpu_build_alloc_info(reserved_size, dyn_size, atom_size, > cpu_distance_fn); > @@ -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; > for (group = 0; group < ai->nr_groups; group++) { > struct pcpu_group_info *gi = &ai->groups[group]; > unsigned int cpu = NR_CPUS; > @@ -2000,6 +2001,21 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, size_t dyn_size, > areas[group] = ptr; > > base = min(ptr, base); > + if (ptr > areas[j]) > + j = group; > + } > + max_distance = areas[j] - base; > + max_distance += ai->unit_size * ai->groups[j].nr_units; > + > + /* warn if maximum distance is further than 75% of vmalloc space */ > + if (max_distance > VMALLOC_TOTAL * 3 / 4) { > + pr_warn("max_distance=0x%lx too large for vmalloc space 0x%lx\n", > + max_distance, VMALLOC_TOTAL); > +#ifdef CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK > + /* and fail if we have fallback */ > + rc = -EINVAL; > + goto out_free_areas; > +#endif > } > > /* > @@ -2024,24 +2040,8 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, size_t dyn_size, > } > > /* base address is now known, determine group base offsets */ > - i = 0; > for (group = 0; group < ai->nr_groups; group++) { > ai->groups[group].base_offset = areas[group] - base; > - if (areas[group] > areas[i]) > - i = group; > - } > - max_distance = ai->groups[i].base_offset + > - (unsigned long)ai->unit_size * ai->groups[i].nr_units; > - > - /* warn if maximum distance is further than 75% of vmalloc space */ > - if (max_distance > VMALLOC_TOTAL * 3 / 4) { > - pr_warn("max_distance=0x%lx too large for vmalloc space 0x%lx\n", > - max_distance, VMALLOC_TOTAL); > -#ifdef CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK > - /* and fail if we have fallback */ > - rc = -EINVAL; > - goto out_free; > -#endif > } > > pr_info("Embedded %zu pages/cpu @%p s%zu r%zu d%zu u%zu\n", >