linux-kernel - Re: [PATCH 04/16] mm/migration: reduce the rcu lock duration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Mar 2022 20:09:15 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     "Huang, Ying" <ying.huang@...el.com>
CC:     <akpm@...ux-foundation.org>, <mike.kravetz@...cle.com>,
        <shy828301@...il.com>, <willy@...radead.org>, <ziy@...dia.com>,
        <minchan@...nel.org>, <apopple@...dia.com>,
        <ave.hansen@...ux.intel.com>, <o451686892@...il.com>,
        <almasrymina@...gle.com>, <jhubbard@...dia.com>,
        <rcampbell@...dia.com>, <peterx@...hat.com>,
        <naoya.horiguchi@....com>, <mhocko@...e.com>, <riel@...hat.com>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Christoph Lameter <cl@...ux.com>,
        David Howells <dhowells@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 04/16] mm/migration: reduce the rcu lock duration

On 2022/3/7 10:32, Huang, Ying wrote:
> Miaohe Lin <linmiaohe@...wei.com> writes:
> 
>> rcu_read_lock is required by grabbing the task refcount but it's not
>> needed for ptrace_may_access. So we could release the rcu lock after
>> task refcount is successfully grabbed to reduce the rcu holding time.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>> ---
>>  mm/migrate.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index da5a81052468..26943bd819e8 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1907,17 +1907,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))
> 
> Digged some history via `git blame`, found that the RCU read lock is
> extended in the following commit,
> 
> "
> 3268c63eded4612a3d07b56d1e02ce7731e6608e
> Author:     Christoph Lameter <cl@...ux.com>
> AuthorDate: Wed Mar 21 16:34:06 2012 -0700
> Commit:     Linus Torvalds <torvalds@...ux-foundation.org>
> CommitDate: Wed Mar 21 17:54:58 2012 -0700
> 
> mm: fix move/migrate_pages() race on task struct
> 
> Migration functions perform the rcu_read_unlock too early.  As a result
> the task pointed to may change from under us.  This can result in an oops,
> as reported by Dave Hansen in https://lkml.org/lkml/2012/2/23/302.
> 
> The following patch extend the period of the rcu_read_lock until after the
> permissions checks are done.  We also take a refcount so that the task
> reference is stable when calling security check functions and performing
> cpuset node validation (which takes a mutex).
> 
> The refcount is dropped before actual page migration occurs so there is no
> change to the refcounts held during page migration.
> 
> Also move the determination of the mm of the task struct to immediately
> before the do_migrate*() calls so that it is clear that we switch from
> handling the task during permission checks to the mm for the actual
> migration.  Since the determination is only done once and we then no
> longer use the task_struct we can be sure that we operate on a specific
> address space that will not change from under us.
> "
> 
> After that, the permission checking has been changed from __task_cred()
> to ptrace_may_access().  So the situation may change somewhat.  Cced

In ptrace_may_access, __task_cred is access while holding the rcu read lock.
It seems this is ensured by the ptrace_may_access itself.

> some names found in git history to verify.

Thanks for your carefulness.

> 
> Best Regards,
> Huang, Ying
> .
>