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 12090C433EF for ; Tue, 31 May 2022 09:01:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8F5AE6B0074; Tue, 31 May 2022 05:01:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8A50A6B0075; Tue, 31 May 2022 05:01:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 767FE6B0078; Tue, 31 May 2022 05:01:15 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 699B26B0074 for ; Tue, 31 May 2022 05:01:15 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 3EC0833D47 for ; Tue, 31 May 2022 09:01:15 +0000 (UTC) X-FDA: 79525444110.18.9F6688A Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by imf06.hostedemail.com (Postfix) with ESMTP id 39FD9180048 for ; Tue, 31 May 2022 09:01:10 +0000 (UTC) Received: from canpemm500002.china.huawei.com (unknown [172.30.72.54]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4LC5lc5Yl6zRhXZ; Tue, 31 May 2022 16:58:04 +0800 (CST) Received: from [10.174.177.76] (10.174.177.76) by canpemm500002.china.huawei.com (7.192.104.244) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 31 May 2022 17:01:09 +0800 Subject: Re: [PATCH v4 1/4] mm: reduce the rcu lock duration To: Ying Huang CC: , , , , , , , , , , "Eric W. Biederman" , , References: <20220530113016.16663-1-linmiaohe@huawei.com> <20220530113016.16663-2-linmiaohe@huawei.com> From: Miaohe Lin Message-ID: Date: Tue, 31 May 2022 17:01:09 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.177.76] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To canpemm500002.china.huawei.com (7.192.104.244) X-CFilter-Loop: Reflected X-Rspam-User: X-Rspamd-Queue-Id: 39FD9180048 X-Stat-Signature: 6szhkpuwm3ox9x414aaegir1149qrqax Authentication-Results: imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of linmiaohe@huawei.com designates 45.249.212.188 as permitted sender) smtp.mailfrom=linmiaohe@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com X-Rspamd-Server: rspam09 X-HE-Tag: 1653987670-668875 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 2022/5/31 14:06, Ying Huang wrote: > On Mon, 2022-05-30 at 19:30 +0800, Miaohe Lin wrote: >> Commit 3268c63eded4 ("mm: fix move/migrate_pages() race on task struct") >> extends the period of the rcu_read_lock until after the permissions checks >> are done to prevent the task pointed to from changing from under us. But >> the task_struct refcount is also taken at that time, the reference to task >> is guaranteed to be stable. So it's unnecessary to extend the period of >> the rcu_read_lock. Release the rcu lock after task refcount is successfully >> grabbed to reduce the rcu holding time. > > Sorry for late reply, I am busy on something else recently. That's all right. Many thanks for your hard work. :) > > I have just read the whole thread of the original patch discussion. > During discussion, in > > https://lore.kernel.org/lkml/alpine.DEB.2.00.1202241131400.3726@router.home/ > > a patch that is same as your one is proposed. Then in the following > message, Eric think that the rcu read lock should be released until > permission is checked, > > https://lore.kernel.org/lkml/87sjhzun47.fsf@xmission.com/ > > " > At the moment I suspect the permissions checks are not safe unless > performed under both rcu_read_lock and task_lock to ensure that > the task<->mm association does not change on us while we are > working. Even with that the cred can change under us but at least > we know the cred will be valid until rcu_read_unlock happens. > " > > So the rcu lock duration is enlarged in the following message. > > https://lore.kernel.org/lkml/alpine.DEB.2.00.1202271238450.32410@router.home/ > > But, after some thought, I don't think extended rcu read lock adds much > value. Because after permission checking the permission may still be > changed. There's no much difference. > > So, I have no objection to the patch itself. But you should add more > information in patch description about why the RCU proected region is > extended and why we can reduce it. Does below patch description makes sense for you? " Commit 3268c63eded4 ("mm: fix move/migrate_pages() race on task struct") extends the period of the rcu_read_lock until after the permissions checks are done because it suspects the permissions checks are not safe unless performed under both rcu_read_lock and task_lock to ensure the task<->mm association does not change on us while we are working [1]. But extended rcu read lock does not add much value. Because after permission checking the permission may still be changed. There's no much difference. So it's unnecessary to extend the period of the rcu_read_lock. Release the rcu lock after task refcount is successfully grabbed to reduce the rcu holding time. [1] https://lore.kernel.org/lkml/87sjhzun47.fsf@xmission.com/ " > > Best Regards, > Huang, Ying Thanks again! > >> Reviewed-by: Muchun Song >> Reviewed-by: Christoph Hellwig >> Reviewed-by: Oscar Salvador >> Signed-off-by: Miaohe Lin >> Cc: Huang Ying >> Cc: David Howells >> Cc: Christoph Lameter >> --- >>  mm/mempolicy.c | 3 +-- >>  mm/migrate.c | 3 +-- >>  2 files changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >> index 0b4ba3ee810e..2dad094177bf 100644 >> --- a/mm/mempolicy.c >> +++ b/mm/mempolicy.c >> @@ -1609,6 +1609,7 @@ static int kernel_migrate_pages(pid_t pid, unsigned long maxnode, >>   goto out; >>   } >>   get_task_struct(task); >> + rcu_read_unlock(); >>   >> >> >> >>   err = -EINVAL; >>   >> >> >> >> @@ -1617,11 +1618,9 @@ static int kernel_migrate_pages(pid_t pid, unsigned long maxnode, >>   * Use the regular "ptrace_may_access()" checks. >>   */ >>   if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) { >> - rcu_read_unlock(); >>   err = -EPERM; >>   goto out_put; >>   } >> - rcu_read_unlock(); >>   >> >> >> >>   task_nodes = cpuset_mems_allowed(task); >>   /* Is the user allowed to access the target nodes? */ >> diff --git a/mm/migrate.c b/mm/migrate.c >> index e51588e95f57..e88ebb88fa6f 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1902,17 +1902,16 @@ static struct mm_struct *find_mm_struct(pid_t pid, nodemask_t *mem_nodes) >>   return ERR_PTR(-ESRCH); >>   } >>   get_task_struct(task); >> + rcu_read_unlock(); >>   >> >> >> >>   /* >>   * Check if this process has the right to modify the specified >>   * process. Use the regular "ptrace_may_access()" checks. >>   */ >>   if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) { >> - rcu_read_unlock(); >>   mm = ERR_PTR(-EPERM); >>   goto out; >>   } >> - rcu_read_unlock(); >>   >> >> >> >>   mm = ERR_PTR(security_task_movememory(task)); >>   if (IS_ERR(mm)) > > > > . >