From b9280678fb5b670d9be55c11eb28a290b07e439c Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Tue, 25 Oct 2016 16:55:12 +0200 Subject: [PATCH] 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. --- repos/base-linux/src/core/platform.cc | 26 ------------------- .../include/base/internal/region_map_mmap.h | 1 - .../src/lib/base/region_map_mmap.cc | 14 ++++++++-- repos/base/src/lib/base/thread.cc | 8 ------ 4 files changed, 12 insertions(+), 37 deletions(-) diff --git a/repos/base-linux/src/core/platform.cc b/repos/base-linux/src/core/platform.cc index d32695bfc..cd488eb78 100644 --- a/repos/base-linux/src/core/platform.cc +++ b/repos/base-linux/src/core/platform.cc @@ -187,15 +187,6 @@ size_t Region_map_mmap::_dataspace_size(Capability ds_cap) if (!ds_cap.valid()) return Local_capability::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 ds_cap) int Region_map_mmap::_dataspace_fd(Capability 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 lx_ds_cap = static_cap_cast(ds_cap); /* @@ -231,14 +213,6 @@ int Region_map_mmap::_dataspace_fd(Capability 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; }); } diff --git a/repos/base-linux/src/include/base/internal/region_map_mmap.h b/repos/base-linux/src/include/base/internal/region_map_mmap.h index 29da3c4b3..37c0f79f8 100644 --- a/repos/base-linux/src/include/base/internal/region_map_mmap.h +++ b/repos/base-linux/src/include/base/internal/region_map_mmap.h @@ -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; diff --git a/repos/base-linux/src/lib/base/region_map_mmap.cc b/repos/base-linux/src/lib/base/region_map_mmap.cc index 6af7d6520..915dde73d 100644 --- a/repos/base-linux/src/lib/base/region_map_mmap.cc +++ b/repos/base-linux/src/lib/base/region_map_mmap.cc @@ -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 diff --git a/repos/base/src/lib/base/thread.cc b/repos/base/src/lib/base/thread.cc index cfa662761..189f3ef1e 100644 --- a/repos/base/src/lib/base/thread.cc +++ b/repos/base/src/lib/base/thread.cc @@ -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)