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;
From:   Liam Howlett <liam.howlett@oracle.com>
To:     Li Zetao <lizetao1@huawei.com>
CC:     "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "willy@infradead.org" <willy@infradead.org>,
        "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "lizetao@huawei.com" <lizetao@huawei.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        "cmllamas@google.com" <cmllamas@google.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mm/mmap: Fix memory leak in mmap_region()
Thread-Topic: [PATCH] mm/mmap: Fix memory leak in mmap_region()
Thread-Index: AQHY6ac25Z1iEvRtV0CDMyzjjpSBZK4h2KuA
Date:   Thu, 27 Oct 2022 07:30:44 +0000
Message-ID: <20221027073029.dyo2p2kearlutizq@revolver>
References: <20221027025837.136492-1-lizetao1@huawei.com>
In-Reply-To: <20221027025837.136492-1-lizetao1@huawei.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?b1jH1S6gr+yROvDSP7XKDB/VKJxACuG1UYuXnNmOPlZvN1E9pUvXGzoQWXOo?=
 =?us-ascii?Q?hbUZOl621mykiMfeLGFN/zUxwUIH3Jl4IQHxMjt4Q/zDKyHDnCp6OhKIBfFc?=
 =?us-ascii?Q?kAQ05chF668pQeGzBahvpfH5/miL+bGcxyzgcmaPBKZi0N5CQxg7VYecpfso?=
 =?us-ascii?Q?vwzLx6OIhxbyV6r4Z7raBCLN4MWMCfnOa9rTp+PH4NvrE85cSRV16rVfK3aT?=
 =?us-ascii?Q?YdZ6ih+fCGuUhpJxBT5BZ9jv8TzGV3L6xFme+mguY35oPpHYOWqMGpjbLBL9?=
 =?us-ascii?Q?Mv3WCXW28y3jwvKrHwPsxf4hsHtLIEVKpPEi73wb2dprQ+FLweewywzSgLEb?=
 =?us-ascii?Q?eKREunjMKRxGB+LWcASaBTpRbH/HYj7Tq1MIMyeYW7axjKgGrvqQe34ef8Ag?=
 =?us-ascii?Q?rfXwJAH7OibJUznkMaLJQfB2bPU+jqJeHquBp3/hFk2YrDlv6pq6yhJAgrpJ?=
 =?us-ascii?Q?jh4Fh9Rv744SF9pFGG7wSsX8MaNQ6obz4k8dtFXV10qvhQKs/8Hd5bm3lCOW?=
 =?us-ascii?Q?rkxU7oKObYI1vfRjD2BePVfwJMff/SuCogvy2/7vLf3havgpXzdAuZpDXBQT?=
 =?us-ascii?Q?ufAFsykdVcAIzPeKcm2hhYvOhiG5HB9qRPVrqdVvy48Ak89ETiDdruUL+eo7?=
 =?us-ascii?Q?d6yZaUPYytmz4u579jJM5vH+P8apJwwqW0nO3Lweubj6reJWQw2Z3W3l+ub9?=
 =?us-ascii?Q?u/o2KxxxMsQ+TsYQDzBOsgNIjDRf2QD2FP+cmEvQymL4qX+FdyS6aSkdptlv?=
 =?us-ascii?Q?Ptn5tytHbkOFkcMeMUZ+ZXkJBsj+vqHjZWRSJCNRcm+kA0kXxo1UvB03Qu5g?=
 =?us-ascii?Q?a6OFhjGbahefUPSWgm4hZBZ0WJbYWLKAc0c+rFPdR1L29vLETIh3a9JOrqys?=
 =?us-ascii?Q?yLB0DX2naGqmXFauxbzPSuceSDcX85PYxeGmhIFRAVEgWbu7CiYuxRqq4Lmn?=
 =?us-ascii?Q?pZPf3tbAz6aNIYMysBN2TAeYo19lW1CAQ5pAOFrff+KecG98nRQY8ttP6GS5?=
 =?us-ascii?Q?NwnM3v/ZzXCSEHSV6ylnJ9X5o5KCMnOp5Tn9h2tFD8tEqem6fFxuaGegloBp?=
 =?us-ascii?Q?00TgAbezpcEBfXdRg06HD/QrlXBbbIsLyqo6xgYEPM2lAPWiPtpZE/SxOPXf?=
 =?us-ascii?Q?Gr1qx0c+tV+ixM2mGJc2TOPHC2TBZpul7WvkPvRbEKAn9bxyR1Ch//GyPoMI?=
 =?us-ascii?Q?NU+n+VmNASJ7/fo/yuB0kXc37gEKFQlLkOhbfJSWtcl0AVyvScTQ9A8YigC2?=
 =?us-ascii?Q?zHabxg3v7gwJJ+49plwF61eREsSaWe6J6Ysyz8XH2sfu1p0pTuukPrd3wzyU?=
 =?us-ascii?Q?2nHVis5G4oGqXBUyrjYu7x9DoiVPioF309rWxlk2E3ycZbAR6TfRHg4Kdnef?=
 =?us-ascii?Q?/MzrbMv05atrIpVrxihWIy+Ukr4qpengLY4sGgh1c/7+9kD2q98rJ3mdHNRD?=
 =?us-ascii?Q?Eluz5EnDmFuEyjegHb8Zf/YZisRulBCYrS05Qmw3Gh+OJFpIWgxFwWQhqnyM?=
 =?us-ascii?Q?7xwF78M4lhnLu8lxhid1oXX2AgpJb024HtzjaQsUea+iRChbHhkJt5rG235w?=
 =?us-ascii?Q?MyZUvu4STSNzYRT8TRSA6bFlXy/2OY8CtedaPskKj8QdvNh5vMIvCb7vhuOt?=
 =?us-ascii?Q?iw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C013221BAC643E488838B3FB50A6E5A9@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9d156f98-4728-46de-1819-08dab7ed2882
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Oct 2022 07:30:44.0780
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: V8LBBTJAxNsZgcHd4d3jHQMtSvaa1iJxV4d9QMw9SpF//WLWYnlPTTjzILa2nKT0BZ1iiwMS+7WnvK5QNCs93g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR10MB7004
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-27_03,2022-10-26_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0
 mlxlogscore=995 malwarescore=0 suspectscore=0 phishscore=0 mlxscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2210270041
X-Proofpoint-ORIG-GUID: oBtlmwyJqT_G_tHfNlnd0hHsFjh0VxDD
X-Proofpoint-GUID: oBtlmwyJqT_G_tHfNlnd0hHsFjh0VxDD
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE 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

* Li Zetao <lizetao1@huawei.com> [221026 21:55]:
> There is a memory leak reported by kmemleak:
>=20
>   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
>=20
> 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.
>=20
> 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.

>=20
> 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(-)
>=20
> 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, unsign=
ed long addr,
>  		error =3D -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, unsign=
ed long addr,
>  		error =3D -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, unsi=
gned 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 =3D 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 =3D NULL;
>  free_vma:
>  	vm_area_free(vma);
>  unacct_error:
> --=20
> 2.31.1
> =