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.
Reviewed-by: Muchun Song <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Miaohe Lin <[email protected]>
Cc: Huang Ying <[email protected]>
Cc: David Howells <[email protected]>
Cc: Christoph Lameter <[email protected]>
---
mm/migrate.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index b2678279eb43..b779646665fe 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))
--
2.23.0
On 25.04.22 15:27, Miaohe Lin wrote:
> 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.
>
> Reviewed-by: Muchun Song <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Miaohe Lin <[email protected]>
> Cc: Huang Ying <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> ---
> mm/migrate.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index b2678279eb43..b779646665fe 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))
Similar pattern in:
mm/mempolicy.c:kernel_migrate_pages()
kernel/futex/syscalls.c:get_robust_list()
kernel/nsproxy.c:validate_nsset()
Exception:
sched/core_sched.c:sched_core_share_pid()
Should we unify -- i.e., adjust the remaining 3 as well?
--
Thanks,
David / dhildenb
On Mon, 2022-04-25 at 21:27 +0800, Miaohe Lin wrote:
> 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.
>
> Reviewed-by: Muchun Song <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Miaohe Lin <[email protected]>
> Cc: Huang Ying <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> ---
> mm/migrate.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index b2678279eb43..b779646665fe 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))
Hi, Miaohe,
Please check the previous discussion and verify whether the original
reported race condition is stll valid by yourself before resending this
patch again. If you find that the original race condition isn't
possible now, please add the analysis in your change log.
Best Regards,
Huang, Ying
On 2022/4/29 17:54, David Hildenbrand wrote:
> On 25.04.22 15:27, Miaohe Lin wrote:
>> 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.
>>
>> Reviewed-by: Muchun Song <[email protected]>
>> Reviewed-by: Christoph Hellwig <[email protected]>
>> Signed-off-by: Miaohe Lin <[email protected]>
>> Cc: Huang Ying <[email protected]>
>> Cc: David Howells <[email protected]>
>> Cc: Christoph Lameter <[email protected]>
>> ---
>> mm/migrate.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index b2678279eb43..b779646665fe 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))
>
> Similar pattern in:
>
> mm/mempolicy.c:kernel_migrate_pages()
> kernel/futex/syscalls.c:get_robust_list()
> kernel/nsproxy.c:validate_nsset()
>
> Exception:
>
> sched/core_sched.c:sched_core_share_pid()
>
>
> Should we unify -- i.e., adjust the remaining 3 as well?
>
I verified that this code change applies to kernel_migrate_pages(), but not get_robust_list()
and validate_nsset(). It's because task_struct reference is not grabbed for later ones. Will
send the new patch soon.
Thanks!