Message ID | 20221027025837.136492-1-lizetao1@huawei.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp586890wru; Wed, 26 Oct 2022 18:55:57 -0700 (PDT) X-Google-Smtp-Source: AMsMyM73OkMKyH0LSjwEJiUNK6YMF2PNKCzMFH6jjHOVDjzrZAFlxtT+qI3T0r8JboMFda94hzip X-Received: by 2002:a17:902:76c3:b0:17a:68:767d with SMTP id j3-20020a17090276c300b0017a0068767dmr47327246plt.109.1666835756747; Wed, 26 Oct 2022 18:55:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666835756; cv=none; d=google.com; s=arc-20160816; b=Ky3IB6q3pGOTdwPr8hgzAjhi723GHC4mC5OoWrrGuxntJKDYdYbbzYalEhEWvhOgxr UYsOHsf/fPff45VTFcyJjLmXzsjUHsp0T9Ig8W1+jux8lGIoy5cbvzhUvNechunP+Eqa yqJeFsUze517Ymv8fkXFbtRixi0QElbtqAQo8JmkJWRM2DH6k7b+42claeYbX48wX113 symiZw0DYLX0iHeia8srW5fnWpfC0THomwO9m/EZWsDcNf7ZFCudBly8HptRosQ52Qsk s8N0mYhcwNcrj+l3gtEz9q4JRhiWD7Y5AsaWjaTtHl7+gzvv/0h4vDLLTElYTr0FKBP0 U3mQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=K5kxLSvsaRAKBCCdhwDuoW1PdJLxVODfQ1k4sVAD1FQ=; b=HGhC71trjDqtANBhrS5oRJbBEkAT63NLFJz9hlda3h0dLBA2BUVLeUhADanOU/BrSY xg5DWn7YkadaVN46szwQT8X0cj3ByQXuo3GbxK9xRGsOfdrt7v81Bj1v40R1Q7BuSwjZ ulNF1dT9sFJYrKk4HkBmQcytZVgKPo4y9wUYn0HAynIfTc1iu0ArxsvSl1REEyWDj73X wtTTIikMuXhmxyAZjhAZKG6hW7Sqa7SXJMMVjaCkb3RiLpf4U1jgQ7gDfppRd40BDlcc KJ77DryYSblCeutctAvlnXXg2iYSHVI1djIgvDkbaRRmb02A+lJPacg9/zsgSldoFKr/ IDqQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u24-20020a656718000000b0046e9babe7a1si8813484pgf.54.2022.10.26.18.55.43; Wed, 26 Oct 2022 18:55:56 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=huawei.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233774AbiJ0Bzj (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Wed, 26 Oct 2022 21:55:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48120 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233745AbiJ0Bz2 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 26 Oct 2022 21:55:28 -0400 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 428497F0B9 for <linux-kernel@vger.kernel.org>; Wed, 26 Oct 2022 18:55:27 -0700 (PDT) Received: from kwepemi500012.china.huawei.com (unknown [172.30.72.53]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4MyTJw0lVNzHvMP; Thu, 27 Oct 2022 09:55:12 +0800 (CST) Received: from huawei.com (10.175.101.6) by kwepemi500012.china.huawei.com (7.221.188.12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Thu, 27 Oct 2022 09:55:24 +0800 From: Li Zetao <lizetao1@huawei.com> To: <akpm@linux-foundation.org>, <Liam.Howlett@Oracle.com>, <willy@infradead.org>, <catalin.marinas@arm.com> CC: <lizetao@huawei.com>, <mike.kravetz@oracle.com>, <cmllamas@google.com>, <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, Li Zetao <lizetao1@huawei.com> Subject: [PATCH] mm/mmap: Fix memory leak in mmap_region() Date: Thu, 27 Oct 2022 10:58:37 +0800 Message-ID: <20221027025837.136492-1-lizetao1@huawei.com> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.175.101.6] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemi500012.china.huawei.com (7.221.188.12) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747803970546828142?= X-GMAIL-MSGID: =?utf-8?q?1747803970546828142?= |
Series |
mm/mmap: Fix memory leak in mmap_region()
|
|
Commit Message
Li Zetao
Oct. 27, 2022, 2:58 a.m. UTC
There is a memory leak reported by kmemleak:
unreferenced object 0xffff88817231ce40 (size 224):
comm "mount.cifs", pid 19308, jiffies 4295917571 (age 405.880s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
60 c0 b2 00 81 88 ff ff 98 83 01 42 81 88 ff ff `..........B....
backtrace:
[<ffffffff81936171>] __alloc_file+0x21/0x250
[<ffffffff81937051>] alloc_empty_file+0x41/0xf0
[<ffffffff81937159>] alloc_file+0x59/0x710
[<ffffffff81937964>] alloc_file_pseudo+0x154/0x210
[<ffffffff81741dbf>] __shmem_file_setup+0xff/0x2a0
[<ffffffff817502cd>] shmem_zero_setup+0x8d/0x160
[<ffffffff817cc1d5>] mmap_region+0x1075/0x19d0
[<ffffffff817cd257>] do_mmap+0x727/0x1110
[<ffffffff817518b2>] vm_mmap_pgoff+0x112/0x1e0
[<ffffffff83adf955>] do_syscall_64+0x35/0x80
[<ffffffff83c0006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0
The root cause was traced to an error handing path in mmap_region()
when arch_validate_flags() or mas_preallocate() fails. In the shared
anonymous mapping sence, vma will be setuped and mapped with a new
shared anonymous file via shmem_zero_setup(). So in this case, the
file resource needs to be released.
Fix it by calling fput(vma->vm_file) when arch_validate_flags() or
mas_preallocate() returns an error. And for the beauty of the code,
put fput() under mapping_unmap_writable().
Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
Fixes: c462ac288f2c ("mm: Introduce arch_validate_flags()")
Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
mm/mmap.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Comments
* Li Zetao <lizetao1@huawei.com> [221026 21:55]: > There is a memory leak reported by kmemleak: > > unreferenced object 0xffff88817231ce40 (size 224): > comm "mount.cifs", pid 19308, jiffies 4295917571 (age 405.880s) > hex dump (first 32 bytes): > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > 60 c0 b2 00 81 88 ff ff 98 83 01 42 81 88 ff ff `..........B.... > backtrace: > [<ffffffff81936171>] __alloc_file+0x21/0x250 > [<ffffffff81937051>] alloc_empty_file+0x41/0xf0 > [<ffffffff81937159>] alloc_file+0x59/0x710 > [<ffffffff81937964>] alloc_file_pseudo+0x154/0x210 > [<ffffffff81741dbf>] __shmem_file_setup+0xff/0x2a0 > [<ffffffff817502cd>] shmem_zero_setup+0x8d/0x160 > [<ffffffff817cc1d5>] mmap_region+0x1075/0x19d0 > [<ffffffff817cd257>] do_mmap+0x727/0x1110 > [<ffffffff817518b2>] vm_mmap_pgoff+0x112/0x1e0 > [<ffffffff83adf955>] do_syscall_64+0x35/0x80 > [<ffffffff83c0006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > The root cause was traced to an error handing path in mmap_region() > when arch_validate_flags() or mas_preallocate() fails. In the shared > anonymous mapping sence, vma will be setuped and mapped with a new > shared anonymous file via shmem_zero_setup(). So in this case, the > file resource needs to be released. > > Fix it by calling fput(vma->vm_file) when arch_validate_flags() or > mas_preallocate() returns an error. And for the beauty of the code, > put fput() under mapping_unmap_writable(). It does look like the unrolling is in the wrong order in that section. > > Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree") > Fixes: c462ac288f2c ("mm: Introduce arch_validate_flags()") > Signed-off-by: Li Zetao <lizetao1@huawei.com> > --- > mm/mmap.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index e270057ed04e..8530195b3ec5 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -2674,6 +2674,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > error = -EINVAL; > if (file) > goto close_and_free_vma; > + else if (vm_flags & VM_SHARED) > + goto put_vma_file; > else > goto free_vma; > } > @@ -2682,6 +2684,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > error = -ENOMEM; > if (file) > goto close_and_free_vma; > + else if (vm_flags & VM_SHARED) > + goto put_vma_file; > else > goto free_vma; > } I am not happy about this getting more complex as it already duplicates too much code. > @@ -2746,13 +2750,13 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > if (vma->vm_ops && vma->vm_ops->close) > vma->vm_ops->close(vma); > unmap_and_free_vma: > - fput(vma->vm_file); > - vma->vm_file = NULL; > - > /* Undo any partial mapping done by a device driver. */ > unmap_region(mm, mas.tree, vma, prev, next, vma->vm_start, vma->vm_end); > if (vm_flags & VM_SHARED) Could we change the above if to "if (file && vm_flags & VM_SHARED)" and jump to unmap_and_free_vma "if (vma->vm_file)"? We could then drop your new goto label. I still think the reodering is correct and worth while. Or am I missing something? > mapping_unmap_writable(file->f_mapping); > +put_vma_file: > + fput(vma->vm_file); > + vma->vm_file = NULL; > free_vma: > vm_area_free(vma); > unacct_error: > -- > 2.31.1 >
Hi, On 2022/10/27 15:30, Liam Howlett wrote: > * Li Zetao <lizetao1@huawei.com> [221026 21:55]: >> There is a memory leak reported by kmemleak: >> >> unreferenced object 0xffff88817231ce40 (size 224): >> comm "mount.cifs", pid 19308, jiffies 4295917571 (age 405.880s) >> hex dump (first 32 bytes): >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> 60 c0 b2 00 81 88 ff ff 98 83 01 42 81 88 ff ff `..........B.... >> backtrace: >> [<ffffffff81936171>] __alloc_file+0x21/0x250 >> [<ffffffff81937051>] alloc_empty_file+0x41/0xf0 >> [<ffffffff81937159>] alloc_file+0x59/0x710 >> [<ffffffff81937964>] alloc_file_pseudo+0x154/0x210 >> [<ffffffff81741dbf>] __shmem_file_setup+0xff/0x2a0 >> [<ffffffff817502cd>] shmem_zero_setup+0x8d/0x160 >> [<ffffffff817cc1d5>] mmap_region+0x1075/0x19d0 >> [<ffffffff817cd257>] do_mmap+0x727/0x1110 >> [<ffffffff817518b2>] vm_mmap_pgoff+0x112/0x1e0 >> [<ffffffff83adf955>] do_syscall_64+0x35/0x80 >> [<ffffffff83c0006a>] entry_SYSCALL_64_after_hwframe+0x46/0xb0 >> >> The root cause was traced to an error handing path in mmap_region() >> when arch_validate_flags() or mas_preallocate() fails. In the shared >> anonymous mapping sence, vma will be setuped and mapped with a new >> shared anonymous file via shmem_zero_setup(). So in this case, the >> file resource needs to be released. >> >> Fix it by calling fput(vma->vm_file) when arch_validate_flags() or >> mas_preallocate() returns an error. And for the beauty of the code, >> put fput() under mapping_unmap_writable(). > It does look like the unrolling is in the wrong order in that section. > >> Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree") >> Fixes: c462ac288f2c ("mm: Introduce arch_validate_flags()") >> Signed-off-by: Li Zetao <lizetao1@huawei.com> >> --- >> mm/mmap.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/mm/mmap.c b/mm/mmap.c >> index e270057ed04e..8530195b3ec5 100644 >> --- a/mm/mmap.c >> +++ b/mm/mmap.c >> @@ -2674,6 +2674,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, >> error = -EINVAL; >> if (file) >> goto close_and_free_vma;I will modify it in the second version >> + else if (vm_flags & VM_SHARED) >> + goto put_vma_file; >> else >> goto free_vma; >> } >> @@ -2682,6 +2684,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, >> error = -ENOMEM; >> if (file) >> goto close_and_free_vma; >> + else if (vm_flags & VM_SHARED) >> + goto put_vma_file; >> else >> goto free_vma; >> } > I am not happy about this getting more complex as it already duplicates > too much code. It's true that my modification would cause the code to further complicate. I will rework it in the next version. >> @@ -2746,13 +2750,13 @@ unsigned long mmap_region(struct file *file, unsigned long addr, >> if (vma->vm_ops && vma->vm_ops->close) >> vma->vm_ops->close(vma); >> unmap_and_free_vma: >> - fput(vma->vm_file); >> - vma->vm_file = NULL; >> - >> /* Undo any partial mapping done by a device driver. */ >> unmap_region(mm, mas.tree, vma, prev, next, vma->vm_start, vma->vm_end); >> if (vm_flags & VM_SHARED) > Could we change the above if to "if (file && vm_flags & VM_SHARED)" and > jump to unmap_and_free_vma "if (vma->vm_file)"? We could then drop your > new goto label. I still think the reodering is correct and worth while. > > Or am I missing something? Thanks for your constructive comments. >> mapping_unmap_writable(file->f_mapping); >> +put_vma_file: >> + fput(vma->vm_file); >> + vma->vm_file = NULL; >> free_vma: >> vm_area_free(vma); >> unacct_error: >> -- >> 2.31.1 Regards, Li Zetao
diff --git a/mm/mmap.c b/mm/mmap.c index e270057ed04e..8530195b3ec5 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -2674,6 +2674,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, error = -EINVAL; if (file) goto close_and_free_vma; + else if (vm_flags & VM_SHARED) + goto put_vma_file; else goto free_vma; } @@ -2682,6 +2684,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, error = -ENOMEM; if (file) goto close_and_free_vma; + else if (vm_flags & VM_SHARED) + goto put_vma_file; else goto free_vma; } @@ -2746,13 +2750,13 @@ unsigned long mmap_region(struct file *file, unsigned long addr, if (vma->vm_ops && vma->vm_ops->close) vma->vm_ops->close(vma); unmap_and_free_vma: - fput(vma->vm_file); - vma->vm_file = NULL; - /* Undo any partial mapping done by a device driver. */ unmap_region(mm, mas.tree, vma, prev, next, vma->vm_start, vma->vm_end); if (vm_flags & VM_SHARED) mapping_unmap_writable(file->f_mapping); +put_vma_file: + fput(vma->vm_file); + vma->vm_file = NULL; free_vma: vm_area_free(vma); unacct_error: