Date:   Thu, 12 Oct 2017 11:29:08 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Luis Felipe Sandoval Castro 
        <luis.felipe.sandoval.castro@...el.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
        mingo@...nel.org, rientjes@...gle.com, n-horiguchi@...jp.nec.com,
        salls@...ucsb.edu, Cristopher Lameter <cl@...ux.com>,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH v1] mm/mempolicy.c: Fix get_nodes() off-by-one error.

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

Powered by blists - more mailing lists