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 3B063C433F5 for ; Tue, 31 May 2022 06:06:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A9AA96B0072; Tue, 31 May 2022 02:06:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A4A8D6B0073; Tue, 31 May 2022 02:06:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 933926B0074; Tue, 31 May 2022 02:06:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 7EEE06B0072 for ; Tue, 31 May 2022 02:06:40 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 4ABB833DC1 for ; Tue, 31 May 2022 06:06:40 +0000 (UTC) X-FDA: 79525004160.16.A7E605C Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf02.hostedemail.com (Postfix) with ESMTP id 7E1FE8005B for ; Tue, 31 May 2022 06:06:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1653977199; x=1685513199; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=eTcJ59lDbXzwiy/p35YXCSINz+6XPaJju471Z3f6x5w=; b=UAOfkbFQyaAyu6qN9Ovx61YSGcM+fcc5ocXLGlIJ6TJJ3dVlKHSGzM17 RMEVAOsGdBOjZGW92Q08fy2g9dtzm+Kstg1Fy19z4WMxvICB+Xgzlxi/O 19ttigfeOKdLP0x/R8ZqADlqHYRyE/wv71F3uEpouk7OsL0xJ1ijtdFOv FM0VnwNtwJTe1UzCt71R3RhOihDndguCa+f/cI/G4xGWv6MOY2DLiL2Nu TeNz+aY6j7YwuQRLVXHh40pQJvqyCC8J2k5ZpCUm5OdiBd+EeYcCNz8ru pQVywYHoyyR1X37RZmmzxSqQRktQUBhGngYyvaPiVZqFYMcKTZGVNJk3o w==; X-IronPort-AV: E=McAfee;i="6400,9594,10363"; a="338193269" X-IronPort-AV: E=Sophos;i="5.91,264,1647327600"; d="scan'208";a="338193269" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 May 2022 23:06:37 -0700 X-IronPort-AV: E=Sophos;i="5.91,264,1647327600"; d="scan'208";a="551626763" Received: from quanliu1-mobl.ccr.corp.intel.com ([10.254.215.142]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 May 2022 23:06:33 -0700 Message-ID: Subject: Re: [PATCH v4 1/4] mm: reduce the rcu lock duration From: Ying Huang To: Miaohe Lin , akpm@linux-foundation.org, naoya.horiguchi@nec.com Cc: peterx@redhat.com, apopple@nvidia.com, osalvador@suse.de, mike.kravetz@oracle.com, songmuchun@bytedance.com, hch@lst.de, dhowells@redhat.com, cl@linux.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, "Eric W. Biederman" Date: Tue, 31 May 2022 14:06:30 +0800 In-Reply-To: <20220530113016.16663-2-linmiaohe@huawei.com> References: <20220530113016.16663-1-linmiaohe@huawei.com> <20220530113016.16663-2-linmiaohe@huawei.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3-1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Stat-Signature: cda1wi45k9ff564uj35i98ch3mju4a3t Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=UAOfkbFQ; dmarc=pass (policy=none) header.from=intel.com; spf=none (imf02.hostedemail.com: domain of ying.huang@intel.com has no SPF policy when checking 134.134.136.100) smtp.mailfrom=ying.huang@intel.com X-Rspam-User: X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 7E1FE8005B X-HE-Tag: 1653977194-189287 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 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. 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. Best Regards, Huang, Ying > 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))