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 1441BC433EF for ; Wed, 16 Mar 2022 09:56:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2A2778D0002; Wed, 16 Mar 2022 05:56:19 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2500A8D0001; Wed, 16 Mar 2022 05:56:19 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 118BC8D0002; Wed, 16 Mar 2022 05:56:19 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0227.hostedemail.com [216.40.44.227]) by kanga.kvack.org (Postfix) with ESMTP id F36438D0001 for ; Wed, 16 Mar 2022 05:56:18 -0400 (EDT) Received: from smtpin31.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id AA6641821CBF7 for ; Wed, 16 Mar 2022 09:56:18 +0000 (UTC) X-FDA: 79249794036.31.CC12684 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by imf24.hostedemail.com (Postfix) with ESMTP id 0CBAF180003 for ; Wed, 16 Mar 2022 09:56:17 +0000 (UTC) Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id A073421125; Wed, 16 Mar 2022 09:56:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1647424576; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Dd5vdp3Qi8IT4o7kjNbpz9TrWOznkW6WDVyec0y1JZ4=; b=QySUcHD0bhNZjxz2GTyKnYnGVe64FKvnBSTmONm+Q42Uii5EL8zO9dHCKf5+pvBq5tvjcK Aopt5JEJeVMeO9ta0p7FczU+qeNjAYMo7m9YJcXdsRCV6td4NC4ISQx3SOa+wS6/KBV8IO 4aAWVqSGzIqNy+marQwyT0hohQ0/F00= Received: from suse.cz (unknown [10.100.201.86]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 6CEF4A3B9E; Wed, 16 Mar 2022 09:56:16 +0000 (UTC) Date: Wed, 16 Mar 2022 10:56:14 +0100 From: Michal Hocko To: Miaohe Lin Cc: akpm@linux-foundation.org, kosaki.motohiro@jp.fujitsu.com, mgorman@suse.de, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm/mempolicy: fix potential mpol_new leak in shared_policy_replace Message-ID: References: <20220311093624.39546-1-linmiaohe@huawei.com> <26577566-ae1e-801c-8c64-89c2c89a487d@huawei.com> <24b2a9ef-eea0-09bd-6842-121d8436e56a@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <24b2a9ef-eea0-09bd-6842-121d8436e56a@huawei.com> X-Rspamd-Queue-Id: 0CBAF180003 X-Stat-Signature: c1p8psnmatc53ghxsddjzkd36nt71tas X-Rspam-User: Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=QySUcHD0; dmarc=pass (policy=quarantine) header.from=suse.com; spf=pass (imf24.hostedemail.com: domain of mhocko@suse.com designates 195.135.220.28 as permitted sender) smtp.mailfrom=mhocko@suse.com X-Rspamd-Server: rspam02 X-HE-Tag: 1647424577-812393 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: On Wed 16-03-22 14:39:37, Miaohe Lin wrote: > On 2022/3/15 23:27, Michal Hocko wrote: > > On Tue 15-03-22 21:42:29, Miaohe Lin wrote: > >> On 2022/3/15 0:44, Michal Hocko wrote: > >>> On Fri 11-03-22 17:36:24, Miaohe Lin wrote: > >>>> If mpol_new is allocated but not used in restart loop, mpol_new will be > >>>> freed via mpol_put before returning to the caller. But refcnt is not > >>>> initialized yet, so mpol_put could not do the right things and might > >>>> leak the unused mpol_new. > >>> > >>> The code is really hideous but is there really any bug there? AFAICS the > >>> new policy is only allocated in if (n->end > end) branch and that one > >>> will set the reference count on the retry. Or am I missing something? > >>> > >> > >> Many thanks for your comment. > >> IIUC, new policy is allocated via the below code: > >> > >> shared_policy_replace: > >> alloc_new: > >> write_unlock(&sp->lock); > >> ret = -ENOMEM; > >> n_new = kmem_cache_alloc(sn_cache, GFP_KERNEL); > >> if (!n_new) > >> goto err_out; > >> mpol_new = kmem_cache_alloc(policy_cache, GFP_KERNEL); > >> if (!mpol_new) > >> goto err_out; > >> goto restart; > >> > >> And mpol_new' reference count will be set before used in n->end > end case. But > >> if that is "not" the case, i.e. mpol_new is not inserted into the rb_tree, mpol_new > >> will be freed via mpol_put before return: > > > > One thing I have missed previously is that the lock is dropped during > > the allocation so I guess the memory policy could have been changed > > during that time. Is this possible? Have you explored this possibility? > > Is this a theoretical problem or it can be triggered intentionally. > > > > This is found via code investigation. I think this could be triggered if there > are many concurrent mpol_set_shared_policy in place. But the user-visible effect > might be obscure as only sizeof(struct mempolicy) bytes leaks possiblely every time. > > > These details would be really interesting for the changelog so that we > > can judge how important this would be. > > This might not be that important as this issue should have been well-concealed for > almost ten years (since commit 42288fe366c4 ("mm: mempolicy: Convert shared_policy mutex to spinlock")). I think it is really worth to drill down to the bottom of the issue. While theoretically possible can be a good enough to justify the change it is usually preferred to describe the underlying problem for future maintainability. -- Michal Hocko SUSE Labs