| Message ID | 20220425132723.34824-2-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup and fixup patches for migration | expand |
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 <songmuchun@bytedance.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > Cc: Huang Ying <ying.huang@intel.com> > Cc: David Howells <dhowells@redhat.com> > Cc: Christoph Lameter <cl@linux.com> > --- > 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?
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 <songmuchun@bytedance.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > Cc: Huang Ying <ying.huang@intel.com> > Cc: David Howells <dhowells@redhat.com> > Cc: Christoph Lameter <cl@linux.com> > --- > 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 <songmuchun@bytedance.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> Cc: Huang Ying <ying.huang@intel.com> >> Cc: David Howells <dhowells@redhat.com> >> Cc: Christoph Lameter <cl@linux.com> >> --- >> 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? > Sorry for late respond. I think it's fine to do all of this together. But this patch is indeed under verifying now. I will try to do that after verified. Thanks!
On 2022/5/6 11:23, ying.huang@intel.com wrote: > 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 <songmuchun@bytedance.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> Cc: Huang Ying <ying.huang@intel.com> >> Cc: David Howells <dhowells@redhat.com> >> Cc: Christoph Lameter <cl@linux.com> >> --- >> 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. > Sorry for late respond. It's a pity that this change is still not verified by the relevant experts. I will try to give my analysis in my change log instead if the original race condition is invalid now. Many thanks! > 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 <songmuchun@bytedance.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> Cc: Huang Ying <ying.huang@intel.com> >> Cc: David Howells <dhowells@redhat.com> >> Cc: Christoph Lameter <cl@linux.com> >> --- >> 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!
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))