Linux-man Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH] mm/mempolicy.c: Remove unnecessary nodemask check in kernel_migrate_pages()
       [not found] <20190806023634.55356-1-wangkefeng.wang@huawei.com>
@ 2019-08-06  8:36 ` Vlastimil Babka
  2019-08-07  0:58   ` Kefeng Wang
  0 siblings, 1 reply; 2+ messages in thread
From: Vlastimil Babka @ 2019-08-06  8:36 UTC (permalink / raw)
  To: Kefeng Wang, Andrew Morton, linux-kernel
  Cc: Andrea Arcangeli, Dan Williams, Michal Hocko, Oscar Salvador,
	linux-mm, Linux API, linux-man

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;
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] mm/mempolicy.c: Remove unnecessary nodemask check in kernel_migrate_pages()
  2019-08-06  8:36 ` [PATCH] mm/mempolicy.c: Remove unnecessary nodemask check in kernel_migrate_pages() Vlastimil Babka
@ 2019-08-07  0:58   ` Kefeng Wang
  0 siblings, 0 replies; 2+ messages in thread
From: Kefeng Wang @ 2019-08-07  0:58 UTC (permalink / raw)
  To: Vlastimil Babka, Andrew Morton, linux-kernel
  Cc: Andrea Arcangeli, Dan Williams, Michal Hocko, Oscar Salvador,
	linux-mm, Linux API, linux-man



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.

> 

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20190806023634.55356-1-wangkefeng.wang@huawei.com>
2019-08-06  8:36 ` [PATCH] mm/mempolicy.c: Remove unnecessary nodemask check in kernel_migrate_pages() Vlastimil Babka
2019-08-07  0:58   ` Kefeng Wang

Linux-man Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-man/0 linux-man/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-man linux-man/ https://lore.kernel.org/linux-man \
		linux-man@vger.kernel.org linux-man@archiver.kernel.org
	public-inbox-index linux-man


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-man


AGPL code for this site: git clone https://public-inbox.org/ public-inbox