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

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

[Luis Felipe Sandoval Castro]

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.

Signed-off-by: Luis Felipe Sandoval Castro <luis.felipe.sandoval.castro@intel.com>
---
 mm/mempolicy.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index d911fa5..5274e9d2 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1208,11 +1208,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

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

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

[Andi Kleen]

On Thu, Oct 19, 2017 at 03:48:09AM +0000, Sandoval Castro, Luis Felipe wrote:
> On Tue 18-10-17 10:42:34, Luis Felipe Sandoval Castro wrote:
> 
> Sorry for the delayed replay, from your feedback I don't think my
> patch has any chances of being merged... I'm wondering though,
> if a note in the man pages "range non inclusive" or something
> like that would help to avoid confusions? Thanks

Yes fixing the man pages is a good idea.

-Andi

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

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

[Sandoval Castro, Luis Felipe]

On Tue 18-10-17 10:42:34, Luis Felipe Sandoval Castro wrote:

Sorry for the delayed replay, from your feedback I don't think my
patch has any chances of being merged... I'm wondering though,
if a note in the man pages "range non inclusive" or something
like that would help to avoid confusions? Thanks

> On Thu 12-10-17 08:28:25, Andi Kleen wrote:
> > On Thu, Oct 12, 2017 at 10:46:33AM +0200, Michal Hocko wrote:
> > > [CC Christoph who seems to be the author of the code]
> >
> > Actually you can blame me. I did the mistake originally.
> > It was found many years ago, but then it was already too late
> > to change.
> >
> > > 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?
> >
> > libnuma uses the available number of nodes as max.
> >
> > So it would always lose the last one with your chance.
> 
> I must be missing something because libnuma does
> if (set_mempolicy(policy, bmp->maskp, bmp->size + 1) < 0)
> 
> so it sets max as size + 1 which is exactly what the man page describes.
> 
> > Your change would be catastrophic.
> 
> I am not sure which change do you mean here. I wasn't proposing any
> patch (yet). All I was saying is that the docuementation diagrees with
> the in kernel implementation. The only applications that would break
> would be those which do not comply to the documentation AFAICS, no?
> --
> Michal Hocko
> SUSE Labs

Best Regards,
Luis

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

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

[Michal Hocko]

On Thu 12-10-17 08:28:25, Andi Kleen wrote:
> On Thu, Oct 12, 2017 at 10:46:33AM +0200, Michal Hocko wrote:
> > [CC Christoph who seems to be the author of the code]
> 
> Actually you can blame me. I did the mistake originally.
> It was found many years ago, but then it was already too late
> to change.
> 
> > 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?
> 
> libnuma uses the available number of nodes as max. 
> 
> So it would always lose the last one with your chance.

I must be missing something because libnuma does
if (set_mempolicy(policy, bmp->maskp, bmp->size + 1) < 0)

so it sets max as size + 1 which is exactly what the man page describes.

> Your change would be catastrophic.

I am not sure which change do you mean here. I wasn't proposing any
patch (yet). All I was saying is that the docuementation diagrees with
the in kernel implementation. The only applications that would break
would be those which do not comply to the documentation AFAICS, no?
-- 
Michal Hocko
SUSE Labs

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

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

[Andi Kleen]

On Thu, Oct 12, 2017 at 10:46:33AM +0200, Michal Hocko wrote:
> [CC Christoph who seems to be the author of the code]

Actually you can blame me. I did the mistake originally.
It was found many years ago, but then it was already too late
to change.

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

libnuma uses the available number of nodes as max. 

So it would always lose the last one with your chance.

Your change would be catastrophic.

The only way to fix it really would be to define
a new syscall. But I don't think it is needed, 
the existing maxnode+1 interface works
(just should be properly documented)

-Andi

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

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

[Michal Hocko]

On Thu 12-10-17 11:14:02, Vlastimil Babka wrote:
> 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.

Well, that really depends on whether _up to_ is inclusive here. My
understanding is that it should be but the way how this is used just
disagrees. E.g. libnuma does the following

static void setpol(int policy, struct bitmask *bmp)
{ 
	if (set_mempolicy(policy, bmp->maskp, bmp->size + 1) < 0)
		numa_error("set_mempolicy");
} 

which suggests that up-to is not inclusive.
 
> > : 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...

That is not what I was suggesting at all. I just think that we should
adhere to the documentation and copy one less than given which would be
in sync with the doc and how it is used with libnuma.
-- 
Michal Hocko
SUSE Labs

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

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@intel.com>
>> ---
>>  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
>>
> 

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

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

[Michal Hocko]

[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

: 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

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@intel.com>
> ---
>  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
> 

-- 
Michal Hocko
SUSE Labs

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

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

[Luis Felipe Sandoval Castro]

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.

Signed-off-by: Luis Felipe Sandoval Castro <luis.felipe.sandoval.castro@intel.com>
---
 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

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