Re: [PATCH v1] mm/mempolicy.c: Fix get_nodes() off-by-one error.

[Vlastimil Babka]

On 10/12/2017 10:46 AM, Michal Hocko wrote:
> [CC Christoph who seems to be the author of the code]
> 
> I would also note that a single patch rarely requires a separate cover
> letter. If there is an information which is not suitable for the
> changelog then you can place it in the diffstate area.
> 
> On Fri 06-10-17 08:36:34, Luis Felipe Sandoval Castro wrote:
>> set_mempolicy() and mbind() take as argument a pointer to a bit mask
>> (nodemask) and the number of bits in the mask the kernel will use
>> (maxnode), among others.  For instace on a system with 2 NUMA nodes valid
>> masks are: 0b00, 0b01, 0b10 and 0b11 it's clear maxnode=2, however an
>> off-by-one error in get_nodes() the function that copies the node mask from
>> user space requires users to pass maxnode = 3 in this example and maxnode =
>> actual_maxnode + 1 in the general case. This patch fixes such error.
> 
> man page of mbind says this
> : nodemask points to a bit mask of nodes containing up to maxnode bits.
> : The bit mask size is rounded to the next multiple of sizeof(unsigned
> : long), but the kernel will use bits only up to maxnode.
> 
> The definition is rather unfortunate. My understanding is that maxnode==1
> will result in copying only bit 0, maxnode==2 will result bits 0 and 1
> being copied. This would be consistent with

But that's not what really happens, it seems? The commit log says you
need to pass maxnode == 3 to get bits 0 and 1. Actually, the unfortunate
"bits only up to maxnode" description would suggest an off-by-one error
in the opposite direction, i.e. maxnode == 3 copying bits 0 up to 3,
thus 4 bits.

> : A NULL value of nodemask or a maxnode value of zero specifies the
> : empty set of nodes.  If the value of maxnode is zero, the nodemask
> : argument is ignored.
> 
> where maxnode==0 means an empty mask. While maxnode==0 will return
> EINVAL AFAICS so it clearly breaks the above wording.
> 
> mbind(0x7ff990b83000, 4096, MPOL_BIND, {}, 0, MPOL_MF_MOVE) = -1 EINVAL (Invalid argument)
> 
> This has been broken for ages and I suspect that tools have found their
> way around that. E.g.
> $ strace -e set_mempolicy numactl --membind=0,1 sleep 1s
> set_mempolicy(MPOL_BIND, 0x21753b0, 1025) = 0

And this is I think the reason why we can't change this now. I assume
numactl allocates 1024 bits (0 to 1023) and passes 1025 to make sure all
1024 bits are processed. If we change it now, kernel will process 1025
bits (0 to 1024) and overflow the allocated bitmask. If it happens to be
at the border of mmaped vma, it's a segfault...

> I assume that the existing userspace simply does the same thing. Pre
> zeros the whole mask with the maxnode being set to the maximum possible
> NUMA nodes.
> 
> Your patch seems broken in the similar way AFAICS. maxnode==1 shouldn't
> be any special.
> 
> Andi has voiced a concern about backward compatibility but I am not sure
> the risk is very high. The current behavior is simply broken unless you
> use a large maxnode anyway. What kind of breakage would you envision
> Andi?
> 
>> Signed-off-by: Luis Felipe Sandoval Castro <luis.felipe.sandoval.castro@xxxxxxxxx>
>> ---
>>  mm/mempolicy.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index 006ba62..0c2e3cd 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -1265,11 +1265,10 @@ static int get_nodes(nodemask_t *nodes, const unsigned long __user *nmask,
>>  	unsigned long nlongs;
>>  	unsigned long endmask;
>>  
>> -	--maxnode;
>>  	nodes_clear(*nodes);
>> -	if (maxnode == 0 || !nmask)
>> +	if (maxnode == 1 || !nmask)
>>  		return 0;
>> -	if (maxnode > PAGE_SIZE*BITS_PER_BYTE)
>> +	if (maxnode - 1 > PAGE_SIZE * BITS_PER_BYTE)
>>  		return -EINVAL;
>>  
>>  	nlongs = BITS_TO_LONGS(maxnode);
>> -- 
>> 1.8.3.1
>>
>