From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4ABD6FA373D for ; Fri, 28 Oct 2022 02:18:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 91A516B0072; Thu, 27 Oct 2022 22:18:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8CA546B0073; Thu, 27 Oct 2022 22:18:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7B9046B0074; Thu, 27 Oct 2022 22:18:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 6C9B06B0072 for ; Thu, 27 Oct 2022 22:18:00 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 462791C6C19 for ; Fri, 28 Oct 2022 02:18:00 +0000 (UTC) X-FDA: 80068747920.11.6536196 Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by imf23.hostedemail.com (Postfix) with ESMTP id 18FB6140020 for ; Fri, 28 Oct 2022 02:17:58 +0000 (UTC) Received: from kwepemi500012.china.huawei.com (unknown [172.30.72.57]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4Mz5fy2Fznz15MG0; Fri, 28 Oct 2022 10:12:58 +0800 (CST) Received: from [10.67.111.176] (10.67.111.176) 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; Fri, 28 Oct 2022 10:17:52 +0800 Message-ID: <66633c9c-f00e-742b-4572-cc0efb153c50@huawei.com> Date: Fri, 28 Oct 2022 10:17:52 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH] mm/mmap: Fix memory leak in mmap_region() To: Liam Howlett CC: "akpm@linux-foundation.org" , "willy@infradead.org" , "catalin.marinas@arm.com" , "lizetao1@huawei.com" , Mike Kravetz , "cmllamas@google.com" , "linux-mm@kvack.org" , "linux-kernel@vger.kernel.org" References: <20221027025837.136492-1-lizetao1@huawei.com> <20221027073029.dyo2p2kearlutizq@revolver> Content-Language: en-US From: Li Zetao In-Reply-To: <20221027073029.dyo2p2kearlutizq@revolver> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.111.176] X-ClientProxiedBy: dggpeml100005.china.huawei.com (7.185.36.185) To kwepemi500012.china.huawei.com (7.221.188.12) X-CFilter-Loop: Reflected ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1666923480; a=rsa-sha256; cv=none; b=rlH0hDlb8p9/nLH+Y4oFYRbmg2rQpo7kVJyMwFT4NyYaG/WsFl+Avz7IAZ94aue934IfOT RRK9RmPKDoCPdv2SINTC0jQDrFgXm+pAMdtY8VzwCU+5FfCF97N6LQKgrM1S6tcPJH0EUX KzeQ/QFhFrDKlR4JK1mAycwAW+bjMtU= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf23.hostedemail.com: domain of lizetao1@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=lizetao1@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1666923480; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=prq7RP7cwM6YljNV63Vrl4usRe2Y5FNeBy1CaaXb+js=; b=i1fZ/XPOgA7I3q7q/uRZvkE0NBZ6dnPKJdJMRCDgivumsppVXpsOS1iz3NbGvKVV7HP1x4 ghRWEBg1cCFE5WfbYpA2CqnLT999AYFrfo632kJW/LngRU8n8nzTe12QXyyAUmSOMMqcEr JqAbtIEKelWj58X9FhGLcBrX3Pb+Tbw= X-Stat-Signature: qw79tws7diy83c9e6hr7y8iobd3so7rm X-Rspamd-Queue-Id: 18FB6140020 X-Rspam-User: Authentication-Results: imf23.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf23.hostedemail.com: domain of lizetao1@huawei.com designates 45.249.212.255 as permitted sender) smtp.mailfrom=lizetao1@huawei.com X-Rspamd-Server: rspam09 X-HE-Tag: 1666923478-784445 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi, On 2022/10/27 15:30, Liam Howlett wrote: > * Li Zetao [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: >> [] __alloc_file+0x21/0x250 >> [] alloc_empty_file+0x41/0xf0 >> [] alloc_file+0x59/0x710 >> [] alloc_file_pseudo+0x154/0x210 >> [] __shmem_file_setup+0xff/0x2a0 >> [] shmem_zero_setup+0x8d/0x160 >> [] mmap_region+0x1075/0x19d0 >> [] do_mmap+0x727/0x1110 >> [] vm_mmap_pgoff+0x112/0x1e0 >> [] do_syscall_64+0x35/0x80 >> [] 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 >> --- >> 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