linux-kernel - Re: [PATCH RFC v2 4/4] mm/mempolicy: add nodes_empty check in SYSC_migrate

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:   Mon, 6 Nov 2017 09:31:44 +0800
From:   Yisheng Xie <xieyisheng1@...wei.com>
To:     Vlastimil Babka <vbabka@...e.cz>, <akpm@...ux-foundation.org>,
        <mhocko@...e.com>, <mingo@...nel.org>, <rientjes@...gle.com>,
        <n-horiguchi@...jp.nec.com>, <salls@...ucsb.edu>
CC:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        <tanxiaojun@...wei.com>, <linux-api@...r.kernel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Christoph Lameter <cl@...ux.com>
Subject: Re: [PATCH RFC v2 4/4] mm/mempolicy: add nodes_empty check in
 SYSC_migrate_pages

Hi Vlastimil,

On 2017/10/31 17:46, Vlastimil Babka wrote:
> +CC Andi and Christoph
> 
> On 10/27/2017 12:14 PM, Yisheng Xie wrote:
>> As manpage of migrate_pages, the errno should be set to EINVAL when none
>> of the specified nodes contain memory. However, when new_nodes is null,
>> i.e. the specified nodes also do not have memory, as the following case:
>>
>> 	new_nodes = 0;
>> 	old_nodes = 0xf;
>> 	ret = migrate_pages(pid, old_nodes, new_nodes, MAX);
>>
>> The ret will be 0 and no errno is set.
>>
>> This patch is to add nodes_empty check to fix above case.
> 
> Hmm, I think we have a bigger problem than "empty set is a subset of
> anything" here.
> 
> The existing checks are:
> 
>         task_nodes = cpuset_mems_allowed(task);
>         if (!nodes_subset(*new, task_nodes) && !capable(CAP_SYS_NICE)) {
>                 err = -EPERM;
>                 goto out_put;
>         }
> 
>         if (!nodes_subset(*new, node_states[N_MEMORY])) {
>                 err = -EINVAL;
>                 goto out_put;
>         }
> 
> 
> And manpage says:
> 
>        EINVAL The value specified by maxnode exceeds a kernel-imposed
> limit.  Or, old_nodes or new_nodes specifies one or more node IDs that
> are greater than the maximum supported node
>               ID.  *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.*
> 
>        EPERM  Insufficient privilege (CAP_SYS_NICE) to move pages of the
> process specified by pid, or insufficient privilege (CAP_SYS_NICE) to
> access the specified target nodes.
> 
> - it says "none ... are allowed", but checking for subset means we check
> if "all ... are allowed". Shouldn't we be checking for a non-empty
> intersection?

You are absolutely right. To follow the manpage, we should check non-empty
of intersection instead of subset. I mean：
         nodes_and(*new, *new, task_nodes);
         if (!node_empty(*new) && !capable(CAP_SYS_NICE)) {
                 err = -EPERM;
                 goto out_put;
         }

         nodes_and(*new, *new, node_states[N_MEMORY]);
         if (!node_empty(*new)) {
                 err = -EINVAL;
                 goto out_put;
         }

So finally, we should only migrate the smallest intersection of all the node
set, right?

> - there doesn't seem to be any EINVAL check for "process's current
> cpuset context", there's just an EPERM check for "target process's
> cpuset context".

This also need to be checked as manpage.

Thanks
Yisheng Xie