From 0f05fa6fd4e49a59cfbf3b2b48305e22faecbe51 Mon Sep 17 00:00:00 2001 From: Stefan Kalkowski Date: Thu, 2 Jul 2015 11:52:57 +0200 Subject: [PATCH] base: resolve dead lock in weak pointer templates This commit eliminates the mutual interlaced taking of destruction lock, list lock and weak pointer locks that could lead to a dead-lock situation when a lock pointer was tried to construct while a weak object is in destruction progress. Now, all weak pointers are invalidated and dequeued at the very beginning of the weak object's destruction. Moreover, before a weak pointer gets invalidated during destruction of a weak object, it gets dequeued, and the list lock is freed again to avoid the former dead-lock. Fix #1607 --- repos/base-codezero/src/core/platform_pd.cc | 3 + repos/base-fiasco/src/core/platform_pd.cc | 3 + repos/base-foc/src/core/platform_pd.cc | 3 + repos/base-nova/src/core/platform_pd.cc | 3 + repos/base-okl4/src/core/platform_pd.cc | 3 + repos/base-pistachio/src/core/platform_pd.cc | 3 + repos/base-sel4/src/core/platform_pd.cc | 3 + repos/base/include/base/weak_ptr.h | 184 +++++++++++------- .../src/core/include/trace/source_registry.h | 5 + repos/os/src/server/nitpicker/view.h | 3 + 10 files changed, 141 insertions(+), 72 deletions(-) diff --git a/repos/base-codezero/src/core/platform_pd.cc b/repos/base-codezero/src/core/platform_pd.cc index 89431ec5b..33e1b1115 100644 --- a/repos/base-codezero/src/core/platform_pd.cc +++ b/repos/base-codezero/src/core/platform_pd.cc @@ -123,5 +123,8 @@ Platform_pd::Platform_pd(Allocator * md_alloc, char const *, Platform_pd::~Platform_pd() { + /* invalidate weak pointers to this object */ + Address_space::lock_for_destruction(); + PWRN("not yet implemented"); } diff --git a/repos/base-fiasco/src/core/platform_pd.cc b/repos/base-fiasco/src/core/platform_pd.cc index 4ce85e21c..11cf50ffe 100644 --- a/repos/base-fiasco/src/core/platform_pd.cc +++ b/repos/base-fiasco/src/core/platform_pd.cc @@ -258,6 +258,9 @@ Platform_pd::Platform_pd(Allocator * md_alloc, char const *, Platform_pd::~Platform_pd() { + /* invalidate weak pointers to this object */ + Address_space::lock_for_destruction(); + /* unbind all threads */ while (Platform_thread *t = _next_thread()) unbind_thread(t); diff --git a/repos/base-foc/src/core/platform_pd.cc b/repos/base-foc/src/core/platform_pd.cc index 52fdba102..23c433b5f 100644 --- a/repos/base-foc/src/core/platform_pd.cc +++ b/repos/base-foc/src/core/platform_pd.cc @@ -128,6 +128,9 @@ Platform_pd::Platform_pd() Platform_pd::~Platform_pd() { + /* invalidate weak pointers to this object */ + Address_space::lock_for_destruction(); + for (unsigned i = 0; i < THREAD_MAX; i++) { if (_threads[i]) _threads[i]->unbind(); diff --git a/repos/base-nova/src/core/platform_pd.cc b/repos/base-nova/src/core/platform_pd.cc index c5d1763a2..c9b31ce92 100644 --- a/repos/base-nova/src/core/platform_pd.cc +++ b/repos/base-nova/src/core/platform_pd.cc @@ -53,6 +53,9 @@ Platform_pd::Platform_pd(Allocator * md_alloc, char const *, Platform_pd::~Platform_pd() { + /* invalidate weak pointers to this object */ + Address_space::lock_for_destruction(); + if (_pd_sel == ~0UL) return; /* Revoke and free cap, pd is gone */ diff --git a/repos/base-okl4/src/core/platform_pd.cc b/repos/base-okl4/src/core/platform_pd.cc index 43ac47fef..067dd3ab8 100644 --- a/repos/base-okl4/src/core/platform_pd.cc +++ b/repos/base-okl4/src/core/platform_pd.cc @@ -354,6 +354,9 @@ Platform_pd::Platform_pd(signed pd_id, bool create) Platform_pd::~Platform_pd() { + /* invalidate weak pointers to this object */ + Address_space::lock_for_destruction(); + /* unbind all threads */ while (Platform_thread *t = _next_thread()) unbind_thread(t); diff --git a/repos/base-pistachio/src/core/platform_pd.cc b/repos/base-pistachio/src/core/platform_pd.cc index 84c6a65cf..048c00b9d 100644 --- a/repos/base-pistachio/src/core/platform_pd.cc +++ b/repos/base-pistachio/src/core/platform_pd.cc @@ -360,6 +360,9 @@ Platform_pd::Platform_pd(Allocator * md_alloc, char const *, Platform_pd::~Platform_pd() { + /* invalidate weak pointers to this object */ + Address_space::lock_for_destruction(); + PT_DBG("Destroying all threads of pd %p", this); /* unbind all threads */ diff --git a/repos/base-sel4/src/core/platform_pd.cc b/repos/base-sel4/src/core/platform_pd.cc index 801b58a15..1ec4f5573 100644 --- a/repos/base-sel4/src/core/platform_pd.cc +++ b/repos/base-sel4/src/core/platform_pd.cc @@ -170,6 +170,9 @@ Platform_pd::Platform_pd(Allocator * md_alloc, char const *, Platform_pd::~Platform_pd() { + /* invalidate weak pointers to this object */ + Address_space::lock_for_destruction(); + platform_specific()->free_core_sel(_vm_cnode_sel); platform_specific()->free_core_sel(_vm_pad_cnode_sel); platform_specific()->free_core_sel(_cspace_cnode_sel); diff --git a/repos/base/include/base/weak_ptr.h b/repos/base/include/base/weak_ptr.h index 17289171d..79d60c2d2 100644 --- a/repos/base/include/base/weak_ptr.h +++ b/repos/base/include/base/weak_ptr.h @@ -15,6 +15,7 @@ #define _INCLUDE__BASE__WEAK_PTR_H_ #include +#include #include namespace Genode { @@ -43,21 +44,18 @@ class Genode::Weak_ptr_base : public Genode::List::Element Lock mutable _lock; Weak_object_base *_obj; - bool _valid; /* true if '_obj' points to an - existing object */ + + /* + * This lock is used to synchronize destruction of a weak pointer + * and its corresponding weak object that happen simultanously + */ + Lock mutable _destruct_lock { Lock::LOCKED }; inline void _adopt(Weak_object_base *obj); inline void _disassociate(); protected: - /** - * Return pointer to object if it exists, or 0 if object vanished - * - * \noapi - */ - Weak_object_base *obj() const { return _valid ? _obj: 0; } - explicit inline Weak_ptr_base(Weak_object_base *obj); public: @@ -79,6 +77,13 @@ class Genode::Weak_ptr_base : public Genode::List::Element */ inline bool operator == (Weak_ptr_base const &other) const; + /** + * Return pointer to object if it exists, or 0 if object vanished + * + * \noapi + */ + Weak_object_base *obj() const { return _obj; } + /** * Inspection hook for unit test * @@ -105,19 +110,16 @@ class Genode::Weak_object_base List _list; /** - * Lock used to defer the destruction of an object derived from - * 'Weak_object_base' + * Buffers dequeued weak pointer that get invalidated currently */ - Lock _destruct_lock; - - protected: + Weak_ptr_base *_ptr_in_destruction = nullptr; /** - * Destructor - * - * \noapi + * Lock to synchronize access to object */ - inline ~Weak_object_base(); + Lock _lock; + + protected: /** * To be called from 'Weak_object' only @@ -129,13 +131,84 @@ class Genode::Weak_object_base public: + /** + * This exception signals a weak pointer that the object + * is in destruction progress + */ + class In_destruction : Exception {}; + + ~Weak_object_base() + { + if (_list.first()) + PERR("Weak object %p not destructed properly " + "there are still dangling pointers to it", this); + } + + void disassociate(Weak_ptr_base *ptr) + { + if (!ptr) return; + + { + Lock::Guard guard(_list_lock); + + /* + * If the weak pointer that tries to disassociate is currently + * removed to invalidate it by the weak object's destructor, + * signal that fact to the pointer, so it can free it's lock, + * and block until invalidation is finished. + */ + if (_ptr_in_destruction == ptr) + throw In_destruction(); + + _list.remove(ptr); + } + } + /** * Mark object as safe to be destructed * * This method must be called by the destructor of a weak object to * defer the destruction until no 'Locked_ptr' is held to the object. */ - void lock_for_destruction() { _destruct_lock.lock(); } + void lock_for_destruction() + { + /* + * Loop through the list of weak pointers and invalidate them + */ + while (true) { + + /* + * To prevent dead-locks we always have to hold + * the order of lock access, therefore we first + * dequeue one weak pointer and free the list lock again + */ + { + Lock::Guard guard(_list_lock); + _ptr_in_destruction = _list.first(); + + /* if the list is empty we're done */ + if (!_ptr_in_destruction) break; + _list.remove(_ptr_in_destruction); + } + + { + Lock::Guard guard(_ptr_in_destruction->_lock); + _ptr_in_destruction->_obj = nullptr; + + /* + * unblock a weak pointer that tried to disassociate + * in the meantime + */ + _ptr_in_destruction->_destruct_lock.unlock(); + } + } + + /* + * synchronize with locked pointers that already aquired + * the lock before the corresponding weak pointer got invalidated + */ + _lock.lock(); + } /** * Inspection hook for unit test @@ -249,7 +322,7 @@ struct Genode::Locked_ptr : Genode::Locked_ptr_base * Only if valid, the locked pointer can be de-referenced. Otherwise, * the attempt will result in a null-pointer access. */ - bool is_valid() const { return curr != 0; } + bool is_valid() const { return curr != nullptr; } }; @@ -263,41 +336,24 @@ void Genode::Weak_ptr_base::_adopt(Genode::Weak_object_base *obj) return; _obj = obj; - _valid = true; - Lock::Guard guard(_obj->_list_lock); - _obj->_list.insert(this); + { + Lock::Guard guard(_obj->_list_lock); + _obj->_list.insert(this); + } } void Genode::Weak_ptr_base::_disassociate() { /* defer destruction of object */ - { + try { Lock::Guard guard(_lock); - if (!_valid) - return; - - _obj->_destruct_lock.lock(); + if (_obj) _obj->disassociate(this); + } catch(Weak_object_base::In_destruction&) { + _destruct_lock.lock(); } - - /* - * Disassociate reference from object - * - * Because we hold the '_destruct_lock', we are safe to do - * the list operation. However, after we have released the - * 'Weak_ptr_base::_lock', the object may have invalidated - * the reference. So we must check for validity again. - */ - { - Lock::Guard guard(_obj->_list_lock); - if (_valid) - _obj->_list.remove(this); - } - - /* release object */ - _obj->_destruct_lock.unlock(); } @@ -307,7 +363,7 @@ Genode::Weak_ptr_base::Weak_ptr_base(Genode::Weak_object_base *obj) } -Genode::Weak_ptr_base::Weak_ptr_base() : _obj(0), _valid(false) { } +Genode::Weak_ptr_base::Weak_ptr_base() : _obj(nullptr) { } void Genode::Weak_ptr_base::operator = (Weak_ptr_base const &other) @@ -316,9 +372,11 @@ void Genode::Weak_ptr_base::operator = (Weak_ptr_base const &other) if (&other == this) return; - Weak_object_base *obj = other.obj(); _disassociate(); - _adopt(obj); + { + Lock::Guard guard(other._lock); + _adopt(other._obj); + } } @@ -329,8 +387,7 @@ bool Genode::Weak_ptr_base::operator == (Weak_ptr_base const &other) const Lock::Guard guard_this(_lock), guard_other(other._lock); - return (!_valid && !other._valid) - || (_valid && other._valid && _obj == other._obj); + return (_obj == other._obj); } @@ -348,39 +405,22 @@ Genode::Weak_ptr Genode::Weak_object_base::_weak_ptr() } -Genode::Weak_object_base::~Weak_object_base() -{ - { - Lock::Guard guard(_list_lock); - - Weak_ptr_base *curr = 0; - while ((curr = _list.first())) { - - Lock::Guard guard(curr->_lock); - curr->_valid = false; - _list.remove(curr); - } - } -} - - Genode::Locked_ptr_base::Locked_ptr_base(Weak_ptr_base &weak_ptr) -: curr(0) +: curr(nullptr) { Lock::Guard guard(weak_ptr._lock); - if (!weak_ptr._valid) - return; + if (!weak_ptr.obj()) return; - curr = weak_ptr._obj; - curr->_destruct_lock.lock(); + curr = weak_ptr.obj(); + curr->_lock.lock(); } Genode::Locked_ptr_base::~Locked_ptr_base() { if (curr) - curr->_destruct_lock.unlock(); + curr->_lock.unlock(); } #endif /* _INCLUDE__BASE__WEAK_PTR_H_ */ diff --git a/repos/base/src/core/include/trace/source_registry.h b/repos/base/src/core/include/trace/source_registry.h index d87354604..d503969fc 100644 --- a/repos/base/src/core/include/trace/source_registry.h +++ b/repos/base/src/core/include/trace/source_registry.h @@ -84,6 +84,11 @@ class Genode::Trace::Source _unique_id(_alloc_unique_id()), _info(info), _control(control) { } + ~Source() + { + /* invalidate weak pointers to this object */ + lock_for_destruction(); + } /************************************* ** Interface used by TRACE service ** diff --git a/repos/os/src/server/nitpicker/view.h b/repos/os/src/server/nitpicker/view.h index 4e3ee3037..9a0108c5a 100644 --- a/repos/os/src/server/nitpicker/view.h +++ b/repos/os/src/server/nitpicker/view.h @@ -126,6 +126,9 @@ class View : public Same_buffer_list_elem, virtual ~View() { + /* invalidate weak pointers to this object */ + lock_for_destruction(); + /* break link to our parent */ if (_parent) _parent->remove_child(*this);