| Message ID | 20190806023634.55356-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/mempolicy.c: Remove unnecessary nodemask check in kernel_migrate_pages() | expand |
On 8/6/19 4:36 AM, Kefeng Wang wrote: > 1) task_nodes = cpuset_mems_allowed(current); > -> cpuset_mems_allowed() guaranteed to return some non-empty > subset of node_states[N_MEMORY]. Right, there's an explicit guarantee. > 2) nodes_and(*new, *new, task_nodes); > -> after nodes_and(), the 'new' should be empty or appropriate > nodemask(online node and with memory). > > After 1) and 2), we could remove unnecessary check whether the 'new' > AND node_states[N_MEMORY] is empty. Yeah looks like the check is there due to evolution of the code, where initially it was added to prevent calling the syscall with bogus nodes, but now that's achieved by cpuset_mems_allowed(). > Cc: Andrea Arcangeli <aarcange@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: linux-mm@kvack.org > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > > [QUESTION] > > SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode, > const unsigned long __user *, old_nodes, > const unsigned long __user *, new_nodes) > { > return kernel_migrate_pages(pid, maxnode, old_nodes, new_nodes); > } > > The migrate_pages() takes pid argument, witch is the ID of the process > whose pages are to be moved. should the cpuset_mems_allowed(current) be > cpuset_mems_allowed(task)? The check for cpuset_mems_allowed(task) is just above the code you change, so the new nodes have to be subset of the target task's cpuset. But they also have to be allowed by the calling task's cpuset. In manpage of migrate_pages(2), this is hinted by the NOTES "Use get_mempolicy(2) with the MPOL_F_MEMS_ALLOWED flag to obtain the set of nodes that are allowed by the calling process's cpuset..." But perhaps the manpage should be better clarified: - the EINVAL case includes "Or, none of the node IDs specified by new_nodes are on-line and allowed by the process's current cpuset context, or none of the specified nodes contain memory." - this should probably say "calling process" to disambiguate - the EPERM case should mention that new_nodes have to be subset of the target process' cpuset context. The caller should also have CAP_SYS_NICE and ptrace_may_access() > mm/mempolicy.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index f48693f75b37..fceb44066184 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1467,10 +1467,6 @@ static int kernel_migrate_pages(pid_t pid, unsigned long maxnode, > if (nodes_empty(*new)) > goto out_put; > > - nodes_and(*new, *new, node_states[N_MEMORY]); > - if (nodes_empty(*new)) > - goto out_put; > - > err = security_task_movememory(task); > if (err) > goto out_put; >
On 2019/8/6 16:36, Vlastimil Babka wrote: > On 8/6/19 4:36 AM, Kefeng Wang wrote: [...] >> >> [QUESTION] >> >> SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode, >> const unsigned long __user *, old_nodes, >> const unsigned long __user *, new_nodes) >> { >> return kernel_migrate_pages(pid, maxnode, old_nodes, new_nodes); >> } >> >> The migrate_pages() takes pid argument, witch is the ID of the process >> whose pages are to be moved. should the cpuset_mems_allowed(current) be >> cpuset_mems_allowed(task)? > > The check for cpuset_mems_allowed(task) is just above the code you change, so > the new nodes have to be subset of the target task's cpuset. > But they also have to be allowed by the calling task's cpuset. In manpage of > migrate_pages(2), this is hinted by the NOTES "Use get_mempolicy(2) with the > MPOL_F_MEMS_ALLOWED flag to obtain the set of nodes that are allowed by the > calling process's cpuset..." > > But perhaps the manpage should be better clarified: > > - the EINVAL case includes "Or, none of the node IDs specified by new_nodes are > on-line and allowed by the process's current cpuset context, or none of the > specified nodes contain memory." - this should probably say "calling process" to > disambiguate > - the EPERM case should mention that new_nodes have to be subset of the target > process' cpuset context. The caller should also have CAP_SYS_NICE and > ptrace_may_access() Get it, thanks for your detail explanation. >
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index f48693f75b37..fceb44066184 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1467,10 +1467,6 @@ static int kernel_migrate_pages(pid_t pid, unsigned long maxnode, if (nodes_empty(*new)) goto out_put; - nodes_and(*new, *new, node_states[N_MEMORY]); - if (nodes_empty(*new)) - goto out_put; - err = security_task_movememory(task); if (err) goto out_put;
1) task_nodes = cpuset_mems_allowed(current); -> cpuset_mems_allowed() guaranteed to return some non-empty subset of node_states[N_MEMORY]. 2) nodes_and(*new, *new, task_nodes); -> after nodes_and(), the 'new' should be empty or appropriate nodemask(online node and with memory). After 1) and 2), we could remove unnecessary check whether the 'new' AND node_states[N_MEMORY] is empty. Cc: Andrea Arcangeli <aarcange@redhat.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Michal Hocko <mhocko@suse.com> Cc: Oscar Salvador <osalvador@suse.de> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: linux-mm@kvack.org Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- [QUESTION] SYSCALL_DEFINE4(migrate_pages, pid_t, pid, unsigned long, maxnode, const unsigned long __user *, old_nodes, const unsigned long __user *, new_nodes) { return kernel_migrate_pages(pid, maxnode, old_nodes, new_nodes); } The migrate_pages() takes pid argument, witch is the ID of the process whose pages are to be moved. should the cpuset_mems_allowed(current) be cpuset_mems_allowed(task)? mm/mempolicy.c | 4 ---- 1 file changed, 4 deletions(-)