2011-06-17 15:22:02

by Vasileios Karakasis

[permalink] [raw]
Subject: [BUG] Invalid return address of mmap() followed by mbind() in multithreaded context

Hi,

I am implementing a multithreaded numa aware code where each thread
mmap()'s an anonymous private region and then mbind()'s it to its local
node. The threads are performing a series of such mmap() + mbind()
operations. My program crashed with SIGSEGV and I noticed that mmap()
returned an invalid address.

I am sending you a simple program that reproduces the error. The program
creates two threads and each thread starts allocating pages and then
binds them to the local node 0. After a number of iterations the program
crashes as it tries to dereference the address returned by mmap(). The
bug doesn't come up when using a single thread, neither when using only
mmap().

I am running a 2.6.39.1 kernel on a 64-bit dual-core machine, but I
tracked this bug back down to the 2.6.34.9 version.

This bug also affects libnuma.

Regards,
--
V.K.


#include <assert.h>
#include <sys/mman.h>
#include <pthread.h>
#include <numaif.h>

#define NR_ITER 10240
#define PAGE_SIZE 4096

void *thread_func(void *args)
{
unsigned char *addr;
int err, i;
unsigned long node = 0x1;

for (i = 0; i < NR_ITER; i++) {
addr = mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
if (addr == (void *) -1) {
assert(0 && "mmap failed");
}
*addr = 0;

err = mbind(addr, PAGE_SIZE, MPOL_BIND, &node, sizeof(node), 0);
if (err < 0) {
assert(0 && "mbind failed");
}
}
return (void *) 0;
}

int main(void)
{
pthread_t thread;
pthread_create(&thread, NULL, thread_func, NULL);
thread_func(NULL);
pthread_join(thread, NULL);
return 0;
}


Attachments:
0x17A67A9C.asc (2.75 kB)
signature.asc (198.00 B)
OpenPGP digital signature
Download all attachments

2011-06-18 18:12:36

by Andi Kleen

[permalink] [raw]
Subject: Re: [BUG] Invalid return address of mmap() followed by mbind() in multithreaded context

> for (i = 0; i < NR_ITER; i++) {
> addr = mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE,
> MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
> if (addr == (void *) -1) {
> assert(0 && "mmap failed");
> }
> *addr = 0;
>
> err = mbind(addr, PAGE_SIZE, MPOL_BIND, &node, sizeof(node), 0);

mbind() can be only done before the first touch. you're not actually testing
numa policy.

-andi

2011-06-18 18:41:29

by Vasileios Karakasis

[permalink] [raw]
Subject: Re: [BUG] Invalid return address of mmap() followed by mbind() in multithreaded context

That's right, but what I want to demonstrate is that the address
returned by mmap() is invalid and the dereference crashes the program,
while it shouldn't. I could equally omit this statement, in which case
mbind() would fail with EFAULT.

On 06/18/2011 09:12 PM, Andi Kleen wrote:
>> for (i = 0; i < NR_ITER; i++) {
>> addr = mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE,
>> MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
>> if (addr == (void *) -1) {
>> assert(0 && "mmap failed");
>> }
>> *addr = 0;
>>
>> err = mbind(addr, PAGE_SIZE, MPOL_BIND, &node, sizeof(node), 0);
>
> mbind() can be only done before the first touch. you're not actually testing
> numa policy.
>
> -andi

--
V.K.


Attachments:
signature.asc (198.00 B)
OpenPGP digital signature

2011-06-19 23:42:47

by Vasileios Karakasis

[permalink] [raw]
Subject: Re: [BUG] Invalid return address of mmap() followed by mbind() in multithreaded context

I'm sending you a slightly modified version that actually makes clear
how libnuma is affected. If you compile with -DUSE_LIBNUMA, you will get
an EFAULT from mbind() and then crash.

This is the gdb output where the address passed to mbind() is invalid.

(gdb) r
Starting program: a.out
[Thread debugging using libthread_db enabled]
[New Thread 0x7ffff7633700 (LWP 17977)]
a.out: mmap-bug.c:29: thread_func: Assertion `0 && "mbind() failed"' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff7667a75 in *__GI_raise (sig=<value optimized out>)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
64 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
in ../nptl/sysdeps/unix/sysv/linux/raise.c
(gdb) f 3
#3 0x00000000004007b8 in thread_func (args=0x0) at mmap-bug.c:29
29 assert(0 && "mbind() failed");
(gdb) p addr
$1 = (unsigned char *) 0x7ffff5c27000 <Address 0x7ffff5c27000 out of bounds>


#include <assert.h>
#include <sys/mman.h>
#include <pthread.h>
#include <numa.h>
#include <numaif.h>

#define NR_ITER 10240
#define PAGE_SIZE 4096

void *thread_func(void *args)
{
unsigned char *addr;
int err, i;
unsigned long node = 0x1;

for (i = 0; i < NR_ITER; i++) {
#ifdef USE_LIBNUMA
addr = numa_alloc_onnode(PAGE_SIZE, 0);
#else
addr = mmap(0, PAGE_SIZE, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
if (addr == (void *) -1)
assert(0 && "mmap() failed");

err = mbind(addr, PAGE_SIZE, MPOL_BIND, &node, sizeof(node), 0);
if (err < 0)
assert(0 && "mbind() failed");
#endif
*addr = 0;
}

return (void *) 0;
}

int main(void)
{
pthread_t thread;
pthread_create(&thread, NULL, thread_func, NULL);
thread_func(NULL);
pthread_join(thread, NULL);
return 0;
}



On 06/18/2011 09:41 PM, Vasileios Karakasis wrote:
> That's right, but what I want to demonstrate is that the address
> returned by mmap() is invalid and the dereference crashes the program,
> while it shouldn't. I could equally omit this statement, in which case
> mbind() would fail with EFAULT.
>
> On 06/18/2011 09:12 PM, Andi Kleen wrote:
>>
>> mbind() can be only done before the first touch. you're not actually testing
>> numa policy.
>>
>> -andi
>

--
V.K.


Attachments:
signature.asc (198.00 B)
OpenPGP digital signature

2011-06-27 17:15:34

by Kornilios Kourtis

[permalink] [raw]
Subject: Re: [BUG] Invalid return address of mmap() followed by mbind() in multithreaded context


Hi,

On Fri, Jun 17, 2011 at 06:21:49PM +0300, Vasileios Karakasis wrote:
> Hi,
>
> I am implementing a multithreaded numa aware code where each thread
> mmap()'s an anonymous private region and then mbind()'s it to its local
> node. The threads are performing a series of such mmap() + mbind()
> operations. My program crashed with SIGSEGV and I noticed that mmap()
> returned an invalid address.

I've taken a closer look at this issue.

As Vasileios said, it can be reproduced by having two threads doing the
following loop:
| for {
| addr = mmap(4096, MAP_ANONUMOUS)
| if (addr == (void *)-1)
| continue
| mbind(addr, 4096, 0x1) // do mbind on first NUMA node
| }
After a couple of iterations, mbind() will return EFAULT, although the addr is
valid.

Doing a bisect, pins it down to the following commit (Author added to To:):
9d8cebd4bcd7c3878462fdfda34bbcdeb4df7ef4
mm: fix mbind vma merge problem
Which adds merging of vmas in the mbind() path.
Reverting this commit, seems to fix the issue.

I 've added some printks to track down the issue, and EFAULT is returned on:
mm/mempolicy.c: mbind_range()
| vma = find_vma_prev(mm. start, &prev);
| if (!vma |vma->vm_start > start)
| return EFAULT;
Where: vma->start > start

I am not sure what exactly happens, but concurrent merges and splits
of (already mapped) VMAs do not seem to work well together.

cheers,
-Kornilios

--
Kornilios Kourtis

2011-06-28 02:07:26

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [BUG] Invalid return address of mmap() followed by mbind() in multithreaded context

(2011/06/28 2:18), Kornilios Kourtis wrote:
>
> Hi,
>
> On Fri, Jun 17, 2011 at 06:21:49PM +0300, Vasileios Karakasis wrote:
>> Hi,
>>
>> I am implementing a multithreaded numa aware code where each thread
>> mmap()'s an anonymous private region and then mbind()'s it to its local
>> node. The threads are performing a series of such mmap() + mbind()
>> operations. My program crashed with SIGSEGV and I noticed that mmap()
>> returned an invalid address.
>
> I've taken a closer look at this issue.
>
> As Vasileios said, it can be reproduced by having two threads doing the
> following loop:
> | for {
> | addr = mmap(4096, MAP_ANONUMOUS)
> | if (addr == (void *)-1)
> | continue
> | mbind(addr, 4096, 0x1) // do mbind on first NUMA node
> | }
> After a couple of iterations, mbind() will return EFAULT, although the addr is
> valid.
>
> Doing a bisect, pins it down to the following commit (Author added to To:):
> 9d8cebd4bcd7c3878462fdfda34bbcdeb4df7ef4
> mm: fix mbind vma merge problem
> Which adds merging of vmas in the mbind() path.
> Reverting this commit, seems to fix the issue.
>
> I 've added some printks to track down the issue, and EFAULT is returned on:
> mm/mempolicy.c: mbind_range()
> | vma = find_vma_prev(mm. start, &prev);
> | if (!vma |vma->vm_start > start)
> | return EFAULT;
> Where: vma->start > start
>
> I am not sure what exactly happens, but concurrent merges and splits
> of (already mapped) VMAs do not seem to work well together.

Hi

Thank you for digging this! I look it at soon as far as possible.

- kosaki