From f50d8165555ca173620a912f9bfa34cdfe86644e Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Fri, 14 Dec 2012 09:29:49 +0100 Subject: [PATCH] base: fix dangling session pointers in rm_session If page faults are handled concurrently (as for base-nova) the traverse lookup call in rm_session_component must be thread safe, which it isn't. If the faulting area is backed by nested dataspaces which are managed by various rm_sessions then a race happens under following circumstances (triggered occasionally by the bomb test). The traverse lookup may return a pointer to a rm_session of a nested dataspace. If the rm_session is in parallel subject to destruction it happened that faults got enqueued to the faulters list of the deleted rm_session and internally to a list of the current rm_session of the Rm_client. During destruction of the faulting Rm_client the associated rm_session will be dissolved from the Rm_client, which leads to dereferencing the dangling pointer of the already destructed rm_session. On base-nova the memory of the rm_session object get unmapped eventually, so that the de-referencing of the dangling pointer caused page faults in core. The memory on other kernels inside core never get unmapped so that the bug doesn't trigger visible faults. The patch replace the keeping of a rm_session pointer by keeping a capability instead. The rm_session object must be looked up now explicitly in the Object_pool implementation, which implements proper reference counting on the rm_session object. Issue #549 --- .../src/core/include/rm_session_component.h | 12 +++- base/src/core/include/dataspace_component.h | 5 +- base/src/core/include/rm_root.h | 27 +++++++- base/src/core/include/rm_session_component.h | 22 ++++--- base/src/core/rm_session_component.cc | 63 +++++++++++++++---- 5 files changed, 100 insertions(+), 29 deletions(-) diff --git a/base-linux/src/core/include/rm_session_component.h b/base-linux/src/core/include/rm_session_component.h index c63561bf0..5f79c13b9 100644 --- a/base-linux/src/core/include/rm_session_component.h +++ b/base-linux/src/core/include/rm_session_component.h @@ -28,10 +28,20 @@ namespace Genode { class Rm_session_component : public Rpc_object { + private: + + class Rm_dataspace_component { + + public: + + void sub_rm_session(Native_capability _cap) { } + }; + public: Rm_session_component(Rpc_entrypoint *ds_ep, Rpc_entrypoint *thread_ep, + Rpc_entrypoint *session_ep, Allocator *md_alloc, size_t ram_quota, Pager_entrypoint *pager_ep, @@ -56,7 +66,7 @@ namespace Genode { Dataspace_capability dataspace() { return Dataspace_capability(); } - void dissolve(Rm_client *cl) { } + Rm_dataspace_component *dataspace_component() { return 0; } }; struct Rm_member { Rm_session_component *member_rm_session() { return 0; } }; diff --git a/base/src/core/include/dataspace_component.h b/base/src/core/include/dataspace_component.h index 309f4b793..fcc67cd81 100644 --- a/base/src/core/include/dataspace_component.h +++ b/base/src/core/include/dataspace_component.h @@ -27,7 +27,6 @@ namespace Genode { class Rm_region; - class Rm_session_component; /** * Deriving classes can own a dataspace to implement conditional behavior @@ -117,9 +116,9 @@ namespace Genode { /** * Return region-manager session corresponding to nested dataspace * - * \retval 0 dataspace is not a nested dataspace + * \retval invalid capability if dataspace is not a nested one */ - virtual Rm_session_component *sub_rm_session() { return 0; } + virtual Native_capability sub_rm_session() { return Dataspace_capability(); } addr_t core_local_addr() const { return _core_local_addr; } bool is_io_mem() const { return _is_io_mem; } diff --git a/base/src/core/include/rm_root.h b/base/src/core/include/rm_root.h index 69e27c15b..fa854ae69 100644 --- a/base/src/core/include/rm_root.h +++ b/base/src/core/include/rm_root.h @@ -49,10 +49,31 @@ namespace Genode { return new (md_alloc()) Rm_session_component(_ds_ep, _thread_ep, + this->ep(), _md_alloc, ram_quota, - &_pager_ep, - start == ~0UL ? _vm_start : start, - size == 0 ? _vm_size : size); + &_pager_ep, + start == ~0UL ? _vm_start : start, + size == 0 ? _vm_size : size); + } + + Session_capability session(Root::Session_args const &args) + { + Session_capability cap = Root_component::session(args); + + /* lookup rm_session_component object */ + Object_pool::Guard rm_session(ep()->lookup_and_lock(cap)); + if (!rm_session) + /* should never happen */ + return cap; + + /** + * Assign rm_session capability to dataspace component. It can + * not be done beforehand because the dataspace_component is + * constructed before the rm_session + */ + if (rm_session->dataspace_component()) + rm_session->dataspace_component()->sub_rm_session(rm_session->cap()); + return cap; } void _upgrade_session(Rm_session_component *rm, const char *args) diff --git a/base/src/core/include/rm_session_component.h b/base/src/core/include/rm_session_component.h index 941c69dde..9cacd833b 100644 --- a/base/src/core/include/rm_session_component.h +++ b/base/src/core/include/rm_session_component.h @@ -212,6 +212,7 @@ namespace Genode { Rpc_entrypoint *_ds_ep; Rpc_entrypoint *_thread_ep; + Rpc_entrypoint *_session_ep; Allocator_guard _md_alloc; Signal_transmitter _fault_notifier; /* notification mechanism for @@ -239,24 +240,25 @@ namespace Genode { { private: - Rm_session_component *_rm_session_component; + Native_capability _rm_session_cap; public: /** * Constructor */ - Rm_dataspace_component(Rm_session_component *rsc, size_t size) + Rm_dataspace_component(size_t size) : - Dataspace_component(size, 0, false, false, 0), - _rm_session_component(rsc) { _managed = true; } + Dataspace_component(size, 0, false, false, 0) + { _managed = true; } /*********************************** ** Dataspace component interface ** ***********************************/ - Rm_session_component *sub_rm_session() { return _rm_session_component; } + Native_capability sub_rm_session() { return _rm_session_cap; } + void sub_rm_session(Native_capability _cap) { _rm_session_cap = _cap; } }; @@ -286,6 +288,7 @@ namespace Genode { */ Rm_session_component(Rpc_entrypoint *ds_ep, Rpc_entrypoint *thread_ep, + Rpc_entrypoint *session_ep, Allocator *md_alloc, size_t ram_quota, Pager_entrypoint *pager_ep, @@ -301,10 +304,11 @@ namespace Genode { * * \return true lookup succeeded */ - bool reverse_lookup(addr_t dst_base, - Fault_area *dst_fault_region, - Dataspace_component **src_dataspace, - Fault_area *src_fault_region); + bool reverse_lookup(addr_t dst_base, + Fault_area *dst_fault_region, + Dataspace_component **src_dataspace, + Fault_area *src_fault_region, + Rm_session_component **sub_rm_session); /** * Register fault diff --git a/base/src/core/rm_session_component.cc b/base/src/core/rm_session_component.cc index 35f0e8c65..63f0bed07 100644 --- a/base/src/core/rm_session_component.cc +++ b/base/src/core/rm_session_component.cc @@ -2,6 +2,7 @@ * \brief Implementation of the RM session interface * \author Christian Helmuth * \author Norman Feske + * \author Alexander Boettcher * \date 2006-07-17 * * FIXME arg_string and quota missing @@ -173,34 +174,59 @@ int Rm_client::pager(Ipc_pager &pager) print_page_fault("page fault", pf_addr, pf_ip, pf_type, badge()); Rm_session_component *curr_rm_session = member_rm_session(); - addr_t curr_rm_base = 0; - Dataspace_component *src_dataspace = 0; + Rm_session_component *sub_rm_session = 0; + addr_t curr_rm_base = 0; + Dataspace_component *src_dataspace = 0; Rm_session_component::Fault_area src_fault_area; Rm_session_component::Fault_area dst_fault_area(pf_addr); bool lookup; - /* traverse potentially nested dataspaces until we hit a leaf dataspace */ unsigned level; enum { MAX_NESTING_LEVELS = 5 }; - for (level = 0; level < MAX_NESTING_LEVELS; level++) { + /* helper guard to release the rm_session lock on return */ + class Guard { + private: + + Rm_session_component ** _release_session; + unsigned * _level; + + public: + + explicit Guard(Rm_session_component ** rs, unsigned * level) + : _release_session(rs), _level(level) {} + + ~Guard() { + if ((*_level > 0) && (*_release_session)) + (*_release_session)->release(); + } + } release_guard(&curr_rm_session, &level); + + /* traverse potentially nested dataspaces until we hit a leaf dataspace */ + for (level = 0; level < MAX_NESTING_LEVELS; level++) { lookup = curr_rm_session->reverse_lookup(curr_rm_base, &dst_fault_area, &src_dataspace, - &src_fault_area); - if (!lookup) - break; - + &src_fault_area, + &sub_rm_session); /* check if we need to traverse into a nested dataspace */ - Rm_session_component *sub_rm_session = src_dataspace->sub_rm_session(); if (!sub_rm_session) break; + if (!lookup) { + sub_rm_session->release(); + break; + } + /* set up next iteration */ curr_rm_base = dst_fault_area.fault_addr() - src_fault_area.fault_addr() + src_dataspace->map_src_addr(); + + if (level > 0) + curr_rm_session->release(); curr_rm_session = sub_rm_session; + sub_rm_session = 0; } if (level == MAX_NESTING_LEVELS) { @@ -624,7 +650,8 @@ void Rm_session_component::remove_client(Pager_capability pager_cap) bool Rm_session_component::reverse_lookup(addr_t dst_base, Fault_area *dst_fault_area, Dataspace_component **src_dataspace, - Fault_area *src_fault_area) + Fault_area *src_fault_area, + Rm_session_component **sub_rm_session) { /* serialize access */ Lock::Guard lock_guard(_lock); @@ -672,7 +699,16 @@ bool Rm_session_component::reverse_lookup(addr_t dst_base, /* constrain source fault area by the source dataspace dimensions */ src_fault_area->constrain(src_base, (*src_dataspace)->size()); - return src_fault_area->valid() && dst_fault_area->valid(); + bool lookup = src_fault_area->valid() && dst_fault_area->valid(); + + if (!lookup) + return lookup; + + /* lookup and lock nested dataspace if required */ + Native_capability session_cap = (*src_dataspace)->sub_rm_session(); + *sub_rm_session = dynamic_cast(_session_ep->lookup_and_lock(session_cap)); + + return lookup; } @@ -732,17 +768,18 @@ static Dataspace_capability _type_deduction_helper(Dataspace_capability cap) { Rm_session_component::Rm_session_component(Rpc_entrypoint *ds_ep, Rpc_entrypoint *thread_ep, + Rpc_entrypoint *session_ep, Allocator *md_alloc, size_t ram_quota, Pager_entrypoint *pager_ep, addr_t vm_start, size_t vm_size) : - _ds_ep(ds_ep), _thread_ep(thread_ep), + _ds_ep(ds_ep), _thread_ep(thread_ep), _session_ep(session_ep), _md_alloc(md_alloc, ram_quota), _client_slab(&_md_alloc), _ref_slab(&_md_alloc), _map(&_md_alloc), _pager_ep(pager_ep), - _ds(this, vm_size), _ds_cap(_type_deduction_helper(ds_ep->manage(&_ds))) + _ds(vm_size), _ds_cap(_type_deduction_helper(ds_ep->manage(&_ds))) { /* configure managed VM area */ _map.add_range(vm_start, vm_size);