Re: [PATCH] mm/mempolicy: add node_empty check in SYSC_migrate_pages

Re: [PATCH] mm/mempolicy: add node_empty check in SYSC_migrate_pages

[Vlastimil Babka]

+CC linux-api

On 10/18/2017 03:37 AM, Yisheng Xie wrote:
> As Xiaojun reported the ltp of migrate_pages01 will failed on ARCH arm64
> system whoes has 4 nodes[0...3], all have memory and CONFIG_NODES_SHIFT=2:
> 
> migrate_pages01    0  TINFO  :  test_invalid_nodes
> migrate_pages01   14  TFAIL  :  migrate_pages_common.c:45: unexpected failure - returned value = 0, expected: -1
> migrate_pages01   15  TFAIL  :  migrate_pages_common.c:55: call succeeded unexpectedly
> 
> In this case the test_invalid_nodes of migrate_pages01 will call:
> SYSC_migrate_pages as:
> 
> migrate_pages(0, , {0x0000000000000001}, 64, , {0x0000000000000010}, 64) = 0

is 64 here the maxnode parameter of migrate_pages() ?

> For MAX_NUMNODES is 4, so 0x10 nodemask will tread as empty set which makes
> 	nodes_subset(*new, node_states[N_MEMORY])

According to manpage of migrate_pages:

        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.

if maxnode parameter is 64, but MAX_NUMNODES ("kernel-imposed limit") is
4, we should get EINVAL just because of that. I don't see such check in
the migrate_pages implementation though. But then at least the
"new_nodes specifies one or more node IDs that are greater than the
maximum supported node ID" part should trigger here, because you have
node number 8 set in the new_nodes nodemask, right?
get_nodes() should be checking this according to comment:

        /* When the user specified more nodes than supported just check
           if the non supported part is all zero. */

Somehow that doesn't seem to work then? I think we should look into
this. Your patch may still be needed, or not, after that is resolved.

> return true, as empty set is subset of any set.
> 
> So this is a common issue which also can happens in X86_64 system eg. 8 nodes[0..7],
> all with memory and CONFIG_NODES_SHIFT=3. Fix it by adding node_empty check in
> SYSC_migrate_pages.
> 
> Reported-by: Tan Xiaojun <tanxiaojun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Yisheng Xie <xieyisheng1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>  mm/mempolicy.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index a2af6d5..1dfd3cc 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1388,6 +1388,11 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode,
>  	if (err)
>  		goto out;
>  
> +	if (nodes_empty(*new)) {
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
>  	/* Find the mm_struct */
>  	rcu_read_lock();
>  	task = pid ? find_task_by_vpid(pid) : current;
> 

Re: [PATCH] mm/mempolicy: add node_empty check in SYSC_migrate_pages

[Yisheng Xie]

Hi Vlastimil,

Thanks for your comment!
On 2017/10/18 15:54, Vlastimil Babka wrote:
> +CC linux-api
> 
> On 10/18/2017 03:37 AM, Yisheng Xie wrote:
>> As Xiaojun reported the ltp of migrate_pages01 will failed on ARCH arm64
>> system whoes has 4 nodes[0...3], all have memory and CONFIG_NODES_SHIFT=2:
>>
>> migrate_pages01    0  TINFO  :  test_invalid_nodes
>> migrate_pages01   14  TFAIL  :  migrate_pages_common.c:45: unexpected failure - returned value = 0, expected: -1
>> migrate_pages01   15  TFAIL  :  migrate_pages_common.c:55: call succeeded unexpectedly
>>
>> In this case the test_invalid_nodes of migrate_pages01 will call:
>> SYSC_migrate_pages as:
>>
>> migrate_pages(0, , {0x0000000000000001}, 64, , {0x0000000000000010}, 64) = 0
> 
> is 64 here the maxnode parameter of migrate_pages() ?

Yes, I have print it in the kernel.

> 
>> For MAX_NUMNODES is 4, so 0x10 nodemask will tread as empty set which makes
>> 	nodes_subset(*new, node_states[N_MEMORY])
> 
> According to manpage of migrate_pages:
> 
>         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.
> 
> if maxnode parameter is 64, but MAX_NUMNODES ("kernel-imposed limit") is
> 4, we should get EINVAL just because of that. I don't see such check in
> the migrate_pages implementation though.

Yes, that is what manpage said, but I have a question about this: if user
set maxnode exceeds a kernel-imposed and try to access node without enough
privilege, which errors values we should return ? For I have seen that all
of the ltp migrate_pages01 will set maxnode to 64 in my system.

> But then at least the
> "new_nodes specifies one or more node IDs that are greater than the
> maximum supported node ID" part should trigger here, because you have
> node number 8 set in the new_nodes nodemask, right?
> get_nodes() should be checking this according to comment:
> 
>         /* When the user specified more nodes than supported just check
>            if the non supported part is all zero. */
> 
> Somehow that doesn't seem to work then? I think we should look into
> this. Your patch may still be needed, or not, after that is resolved.

OK, I will check why get_nodes do not works as it comments.

Thanks
Yisheng Xie

> 
>> return true, as empty set is subset of any set.
>>
>> So this is a common issue which also can happens in X86_64 system eg. 8 nodes[0..7],
>> all with memory and CONFIG_NODES_SHIFT=3. Fix it by adding node_empty check in
>> SYSC_migrate_pages.
>>
>> Reported-by: Tan Xiaojun <tanxiaojun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Yisheng Xie <xieyisheng1-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>  mm/mempolicy.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index a2af6d5..1dfd3cc 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -1388,6 +1388,11 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode,
>>  	if (err)
>>  		goto out;
>>  
>> +	if (nodes_empty(*new)) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>>  	/* Find the mm_struct */
>>  	rcu_read_lock();
>>  	task = pid ? find_task_by_vpid(pid) : current;
>>
> 
> 
> .
> 

Re: [PATCH] mm/mempolicy: add node_empty check in SYSC_migrate_pages

[Vlastimil Babka]

On 10/18/2017 11:34 AM, Yisheng Xie wrote:
>>> For MAX_NUMNODES is 4, so 0x10 nodemask will tread as empty set which makes
>>> 	nodes_subset(*new, node_states[N_MEMORY])
>>
>> According to manpage of migrate_pages:
>>
>>         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.
>>
>> if maxnode parameter is 64, but MAX_NUMNODES ("kernel-imposed limit") is
>> 4, we should get EINVAL just because of that. I don't see such check in
>> the migrate_pages implementation though.
> 
> Yes, that is what manpage said, but I have a question about this: if user
> set maxnode exceeds a kernel-imposed and try to access node without enough
> privilege, which errors values we should return ? For I have seen that all
> of the ltp migrate_pages01 will set maxnode to 64 in my system.

Hm I don't think it matters much and don't know if there's some commonly
used priority. Personally I would do the checks resulting in EINVAL
first, before EPERM, but if the code is structured differently, it may
stay as it is.

Re: [PATCH] mm/mempolicy: add node_empty check in SYSC_migrate_pages

[Yisheng Xie]

Hi Vlastimil,

Thanks for you comment!
On 2017/10/18 17:34, Yisheng Xie wrote:
> Hi Vlastimil,
> 
> Thanks for your comment!
> On 2017/10/18 15:54, Vlastimil Babka wrote:
>> +CC linux-api
>>
>> On 10/18/2017 03:37 AM, Yisheng Xie wrote:
>>> As Xiaojun reported the ltp of migrate_pages01 will failed on ARCH arm64
>>> system whoes has 4 nodes[0...3], all have memory and CONFIG_NODES_SHIFT=2:
>>>
>>> migrate_pages01    0  TINFO  :  test_invalid_nodes
>>> migrate_pages01   14  TFAIL  :  migrate_pages_common.c:45: unexpected failure - returned value = 0, expected: -1
>>> migrate_pages01   15  TFAIL  :  migrate_pages_common.c:55: call succeeded unexpectedly
>>>
>>> In this case the test_invalid_nodes of migrate_pages01 will call:
>>> SYSC_migrate_pages as:
>>>
>>> migrate_pages(0, , {0x0000000000000001}, 64, , {0x0000000000000010}, 64) = 0
>>
>> is 64 here the maxnode parameter of migrate_pages() ?
> 
> Yes, I have print it in the kernel.
> 
>>
>>> For MAX_NUMNODES is 4, so 0x10 nodemask will tread as empty set which makes
>>> 	nodes_subset(*new, node_states[N_MEMORY])
>>
>> According to manpage of migrate_pages:
>>
>>         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.
>>
>> if maxnode parameter is 64, but MAX_NUMNODES ("kernel-imposed limit") is
>> 4, we should get EINVAL just because of that. I don't see such check in
>> the migrate_pages implementation though.
I agree that we should add this check,  howver, I'm doubt about what
"kernel-imposed limit" in the manpage, does it really what your said
MAX_NUMNODES? or BITS_PER_LONG * BITS_TO_LONGS(MAX_NUMNODES),
we used unsigned long to store node bitmap, so the limit should count in
multiple of BITS_PER_LONG, is this fare?

> 
> Yes, that is what manpage said, but I have a question about this: if user
> set maxnode exceeds a kernel-imposed and try to access node without enough
> privilege, which errors values we should return ? For I have seen that all
> of the ltp migrate_pages01 will set maxnode to 64 in my system.
> 
>> But then at least the
>> "new_nodes specifies one or more node IDs that are greater than the
>> maximum supported node ID" part should trigger here, because you have
>> node number 8 set in the new_nodes nodemask, right?
>> get_nodes() should be checking this according to comment:
>>
>>         /* When the user specified more nodes than supported just check
>>            if the non supported part is all zero. */

here, "nodes than supported" also means BITS_PER_LONG * BITS_TO_LONGS(MAX_NUMNODES),
it check whether user specified more than BITS_PER_LONG * BITS_TO_LONGS(MAX_NUMNODES)
is zero or no. And if "kernel-imposed limit" means MAX_NUMNODES this check is no need
at all, we can just check if maxnode > MAX_NUMNODES, for bits higher than maxnode is
invalid which should be masked after taken from user:
       The old_nodes and new_nodes arguments are pointers to bit masks of
       node numbers, with up to maxnode bits in each mask.  These masks are
       maintained as arrays of unsigned long integers (in the last long
       integer, the bits beyond those specified by maxnode are ignored).
       The maxnode argument is the maximum node number in the bit mask plus
       one (this is the same as in mbind(2), but different from select(2)).

The get_nodes is just a common code which also used in set_mempolicy whoes ERRORS
of EINVAL is not the same as migrate_pages:
    EINVAL
        mode is invalid. Or, mode is MPOL_DEFAULT and nodemask is nonempty, or mode
        is MPOL_BIND or MPOL_INTERLEAVE and nodemask is empty. Or, *maxnode specifies
        more than a page worth of bits*. Or, nodemask specifies one or more node IDs
        that are greater than the maximum supported node ID. Or, none of the node IDs
        specified by nodemask are on-line and allowed by the process's current cpuset
        context, or none of the specified nodes contain memory. Or, the mode argument
        specified both MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES.

So get_nodes just check whether "nodemask specifies one or more node IDs that are
greater than the maximum supported node ID（BITS_TO_LONGS(MAX_NUMNODES)）" as a
common part. If we want check "maxnode exceeds a kernel-imposed limit", maybe we
should add following in migrate_pages:
+       if (BITS_TO_LONGS(MAX_NUMNODES) < BITS_TO_LONGS(maxnode)) {
+               err = -EINVAL;
+               goto out;
+       }
+

And for nodes_empty() check should also be need for this case or real empty nodes set.
Any opinion?

Thanks
Yisheng Xie.

Re: [PATCH] mm/mempolicy: add node_empty check in SYSC_migrate_pages

[Yisheng Xie]

Hi Vlastimil,

Thanks for your comment!
On 2017/10/18 18:46, Vlastimil Babka wrote:
> On 10/18/2017 11:34 AM, Yisheng Xie wrote:
>>>> For MAX_NUMNODES is 4, so 0x10 nodemask will tread as empty set which makes
>>>> 	nodes_subset(*new, node_states[N_MEMORY])
>>>
>>> According to manpage of migrate_pages:
>>>
>>>         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.
>>>
>>> if maxnode parameter is 64, but MAX_NUMNODES ("kernel-imposed limit") is
>>> 4, we should get EINVAL just because of that. I don't see such check in
>>> the migrate_pages implementation though.
>>
>> Yes, that is what manpage said, but I have a question about this: if user
>> set maxnode exceeds a kernel-imposed and try to access node without enough
>> privilege, which errors values we should return ? For I have seen that all
>> of the ltp migrate_pages01 will set maxnode to 64 in my system.
> 
> Hm I don't think it matters much and don't know if there's some commonly
> used priority. Personally I would do the checks resulting in EINVAL
> first, before EPERM, but if the code is structured differently, it may
> stay as it is.

I see，and  I have checked the code of get_nodes, which seems treat
"kernel-imposed limit" as the meaning of
BITS_PER_LONG * BITS_TO_LONGS(MAX_NUMNODES) instead of MAX_NUMNODES,
which I have replied in another mail.

As we use unsigned long to store node bitmap, so the limit should be counted in
multiple of BITS_PER_LONG, fair?

Thanks
Yisheng Xie

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>