base-linux: fix race in region_map_mmap

This patch fixes a race condition triggered by the thread test running
on Linux inside VirtualBox. The 'test_stack_alloc' sporadically produced
one of two errors: A segfault in the 'Thread::deinit_platform_thread' on
the attempt to access the 'native_thread' of the to-be-destructed thread
(this data structure is located on the thread's stack). Or, an error
message about a region conflict within the stack area.

The problem was that two instances of 'Region_map_mmap' issued a
sequence of munmap and mmap each. Even though each instance locked the
attach/detach operations, the lock was held per instance. In a situation
where two instances performed attach/detach operations in parallel, the
syscall sequences could interfere with each other.

In the test scenario, the two region-map instances are the test's
address space and the stack area. When creating a thread, the thread's
trace-control dataspace is attached at an arbitrary place (picked by
the Linux kernel) within the address space whereas the stack is attached
at the stack area. The problem is the following sequence:

Thread A wants to destruct a thread:
1. Remove stack from stack area
   (issue unmap syscall)
2. Preserve virtual address range that was occupied from the stack
   so that Linux won't use it
   (issue mmap syscall)

Thread B wants to construct a thread:
1. Request trace-control dataspace from CPU session
2. Attach trace-control dataspace to address space at a location
   picked by the Linux kernel
   (issue mmap syscall)

The problem occurs when thread B's second step is executed in between
the steps 1 and 2 of thread A and the Linux kernel picks the
just-unmapped address as the location for the new trace-control mapping.
Now, the trace control dataspace is mapped at the virtual address that
was designated for the stack of the to-be-created thread, and the
attempt to map the real stack fails.

The patch fixes the problem by replacing the former region-map-local
locks by a component-global lock.

Furthermore, it cleans up core's implementation of the support function
for the region-map-mmap implementation, eliminating the temporary
unlocking of the region-map lock during RPC.
This commit is contained in:
Norman Feske 2016-10-25 16:55:12 +02:00 committed by Christian Helmuth
parent 466bec038f
commit b9280678fb
4 changed files with 12 additions and 37 deletions

View File

@ -187,15 +187,6 @@ size_t Region_map_mmap::_dataspace_size(Capability<Dataspace> ds_cap)
if (!ds_cap.valid())
return Local_capability<Dataspace>::deref(ds_cap)->size();
/* use RPC if called from a different thread */
if (!core_env()->entrypoint()->is_myself()) {
/* release Region_map_mmap::_lock during RPC */
_lock.unlock();
Genode::size_t size = Dataspace_client(ds_cap).size();
_lock.lock();
return size;
}
/* use local function call if called from the entrypoint */
return core_env()->entrypoint()->apply(ds_cap, [] (Dataspace *ds) {
return ds ? ds->size() : 0; });
@ -204,15 +195,6 @@ size_t Region_map_mmap::_dataspace_size(Capability<Dataspace> ds_cap)
int Region_map_mmap::_dataspace_fd(Capability<Dataspace> ds_cap)
{
if (!core_env()->entrypoint()->is_myself()) {
/* release Region_map_mmap::_lock during RPC */
_lock.unlock();
Untyped_capability fd_cap = Linux_dataspace_client(ds_cap).fd();
int socket = Capability_space::ipc_cap_data(fd_cap).dst.socket;
_lock.lock();
return socket;
}
Capability<Linux_dataspace> lx_ds_cap = static_cap_cast<Linux_dataspace>(ds_cap);
/*
@ -231,14 +213,6 @@ int Region_map_mmap::_dataspace_fd(Capability<Dataspace> ds_cap)
bool Region_map_mmap::_dataspace_writable(Dataspace_capability ds_cap)
{
if (!core_env()->entrypoint()->is_myself()) {
/* release Region_map_mmap::_lock during RPC */
_lock.unlock();
bool writable = Dataspace_client(ds_cap).writable();
_lock.lock();
return writable;
}
return core_env()->entrypoint()->apply(ds_cap, [] (Dataspace *ds) {
return ds ? ds->writable() : false; });
}

View File

@ -35,7 +35,6 @@ class Genode::Region_map_mmap : public Region_map, public Dataspace
{
private:
Lock _lock; /* protect '_rmap' */
Region_registry _rmap;
bool const _sub_rm; /* false if region map is root */
size_t const _size;

View File

@ -60,6 +60,16 @@ static bool is_sub_rm_session(Dataspace_capability ds)
}
/**
* Lock for protecting mmap/unmap sequences and region-map meta data
*/
static Lock &lock()
{
static Lock lock;
return lock;
}
addr_t Region_map_mmap::_reserve_local(bool use_local_addr,
addr_t local_addr,
Genode::size_t size)
@ -164,7 +174,7 @@ Region_map::Local_addr Region_map_mmap::attach(Dataspace_capability ds,
Region_map::Local_addr local_addr,
bool executable)
{
Lock::Guard lock_guard(_lock);
Lock::Guard lock_guard(lock());
/* only support attach_at for sub RM sessions */
if (_sub_rm && !use_local_addr) {
@ -306,7 +316,7 @@ Region_map::Local_addr Region_map_mmap::attach(Dataspace_capability ds,
void Region_map_mmap::detach(Region_map::Local_addr local_addr)
{
Lock::Guard lock_guard(_lock);
Lock::Guard lock_guard(lock());
/*
* Cases

View File

@ -77,14 +77,6 @@ void Stack::size(size_t const size)
Stack *
Thread::_alloc_stack(size_t stack_size, char const *name, bool main_thread)
{
/*
* Synchronize stack list when creating new threads from multiple threads
*
* XXX: remove interim fix
*/
static Lock alloc_lock;
Lock::Guard _lock_guard(alloc_lock);
/* allocate stack */
Stack *stack = Stack_allocator::stack_allocator().alloc(this, main_thread);
if (!stack)