From dd416b6a1ad44f68d0b52acb01c78f92feccdaea Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Thu, 25 Feb 2021 13:27:29 +0100 Subject: [PATCH 01/17] trigger pagemap bug --- kernel/app/init-example/app/init.cc | 51 +++++++++++++++---- .../memory-amd64/objects/PageMapAmd64.cc | 1 + 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/kernel/app/init-example/app/init.cc b/kernel/app/init-example/app/init.cc index e27c8497..1bb8259a 100644 --- a/kernel/app/init-example/app/init.cc +++ b/kernel/app/init-example/app/init.cc @@ -517,26 +517,57 @@ void test_process(){ MLOG_INFO(mlog::app, "Test process finished"); } +void pageMapBug(){ + MLOG_INFO(mlog::app, "Trigger PageMap deadlock"); + + mythos::PortalLock pl(portal); + + mythos::PageMap pm4(capAlloc()); + mythos::PageMap pm3(capAlloc()); + mythos::PageMap pm2(capAlloc()); + + auto res = pm4.create(pl, kmem, 4).wait(); + ASSERT(res); + res = pm3.create(pl, kmem, 3).wait(); + ASSERT(res); + res = pm2.create(pl, kmem, 2).wait(); + ASSERT(res); + + uintptr_t vaddr = 0x4000000; + + pm4.installMap(pl, pm3, ((vaddr >> 39) & 0x1FF) << 39, 4, + mythos::protocol::PageMap::MapFlags().writable(true).configurable(true)).wait(); + pm3.installMap(pl, pm2, ((vaddr >> 30) & 0x1FF) << 30, 3, + mythos::protocol::PageMap::MapFlags().writable(true).configurable(true)).wait(); + + MLOG_INFO(mlog::app, "Try to delete pm3 -> deadlock"); + + capAlloc.free(pm3.cap(), pl); + + MLOG_INFO(mlog::app, "If you can read this, you might have fixed the deadlock?!"); +} + int main() { char const str[] = "Hello world!"; mythos::syscall_debug(str, sizeof(str)-1); MLOG_ERROR(mlog::app, "application is starting :)", DVARhex(info_ptr), DVARhex(initstack_top)); - test_float(); - test_Example(); - test_Portal(); + //test_float(); + //test_Example(); + //test_Portal(); test_heap(); // heap must be initialized for tls test - test_tls(); - test_exceptions(); + //test_tls(); + //test_exceptions(); //test_InterruptControl(); //test_HostChannel(portal, 24*1024*1024, 2*1024*1024); - test_ExecutionContext(); - test_pthreads(); - test_Rapl(); - test_processor_allocator(); - test_process(); + //test_ExecutionContext(); + //test_pthreads(); + //test_Rapl(); + //test_processor_allocator(); + //test_process(); //test_CgaScreen(); + pageMapBug(); char const end[] = "bye, cruel world!"; mythos::syscall_debug(end, sizeof(end)-1); diff --git a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc index a3d88dbb..989a1909 100644 --- a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc +++ b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc @@ -111,6 +111,7 @@ namespace mythos { if (self.isOriginal()) { MLOG_DETAIL(mlog::cap, "delete PageMap", self); for (size_t i = 0; i < num_caps(); ++i) { + if(_cap_table(i).is_locked()) MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, "table is locked and cannot be deleted! -> deadlock!", self); auto res = del.deleteEntry(_cap_table(i)); ASSERT_MSG(res, "Mapped entries must be deletable."); if (!res) RETHROW(res); From 13611a59603543ca43735e61d295dda0e23e97a0 Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Thu, 25 Feb 2021 21:56:14 +0100 Subject: [PATCH 02/17] debug output --- .../capability-spinning/objects/CapEntry.cc | 1 + .../capability-spinning/objects/CapEntry.hh | 20 ++++++++++++++++--- .../objects/RevokeOperation.cc | 4 +++- .../memory-amd64/objects/PageMapAmd64.cc | 7 ++++--- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index 52c134ed..7e8f1914 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -139,6 +139,7 @@ namespace mythos { Error CapEntry::try_lock_prev() { auto prev = Link(_prev).ptr(); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), DVAR(prev)); if (!prev) { return Error::GENERIC_ERROR; } diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index e837ed87..1bf4e9b0 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -84,9 +84,23 @@ namespace mythos { bool kill(); optional unlink(); - bool try_lock() { return !(_next.fetch_or(LOCKED_FLAG) & LOCKED_FLAG); } - void lock() { while (!try_lock()) { hwthread_pause(); } } - void unlock() { auto res = _next.fetch_and(~LOCKED_FLAG); ASSERT(res & LOCKED_FLAG); } + bool try_lock() { + bool ret = !(_next.fetch_or(LOCKED_FLAG) & LOCKED_FLAG); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), ret? " locked" : "locking failed!"); + return ret; } + void lock() { + int loop = 0; + while (!try_lock()) { + hwthread_pause(); + loop++; + if(loop > 2){ + MLOG_ERROR(mlog::cap, " lockeing failed too many times -> fail"); + while(1); + } + } } + void unlock() { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); + auto res = _next.fetch_and(~LOCKED_FLAG); ASSERT(res & LOCKED_FLAG); } /// only for assertions and debugging /// only trust the result if it is false and it should be true diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.cc b/kernel/objects/capability-spinning/objects/RevokeOperation.cc index 18e373ca..bfbaff3d 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.cc +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.cc @@ -110,7 +110,7 @@ namespace mythos { do { if (_startTraversal(root, rootCap)) { leaf = _findLockedLeaf(root); - MLOG_DETAIL(mlog::cap, "_findLockedLeaf returned", DVAR(*leaf), DVAR(rootCap)); + MLOG_ERROR(mlog::cap, "_findLockedLeaf returned", DVAR(*leaf), DVAR(rootCap)); if (leaf == root && !rootCap.isZombie()) { // this is a revoke, do not delete the root. no more children -> we are done root->finishRevoke(); @@ -145,6 +145,7 @@ namespace mythos { bool RevokeOperation::_startTraversal(CapEntry* root, Cap rootCap) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__ ); if (!root->lock_prev()) { // start is currently unlinked // must be the work of another deleter ... success! @@ -171,6 +172,7 @@ namespace mythos { CapEntry* RevokeOperation::_findLockedLeaf(CapEntry* root) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__ ); auto curEntry = root; while (true) { auto curCap = curEntry->cap(); diff --git a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc index 989a1909..0b19a808 100644 --- a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc +++ b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc @@ -101,17 +101,18 @@ namespace mythos { optional PageMap::MappedPageMap::deleteCap(CapEntry& entry, Cap /*self*/, IDeleter&) { auto idx = &entry - &map->_cap_table(0); - MLOG_DETAIL(mlog::cap, "delete mapped PageMap", DVAR(idx)); + MLOG_ERROR(mlog::cap, "delete mapped PageMap", DVAR(idx)); map->_pm_table(idx).reset(); RETURN(Error::SUCCESS); } optional PageMap::deleteCap(CapEntry&, Cap self, IDeleter& del) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, self); if (self.isOriginal()) { - MLOG_DETAIL(mlog::cap, "delete PageMap", self); + MLOG_ERROR(mlog::cap, "delete PageMap", self); for (size_t i = 0; i < num_caps(); ++i) { - if(_cap_table(i).is_locked()) MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, "table is locked and cannot be deleted! -> deadlock!", self); + if(_cap_table(i).is_locked()) MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, "table is locked and cannot be deleted! -> deadlock detected!"); auto res = del.deleteEntry(_cap_table(i)); ASSERT_MSG(res, "Mapped entries must be deletable."); if (!res) RETHROW(res); From 9d43fb7b3b9562bc510efdd48fbee8599eab08c1 Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Fri, 26 Feb 2021 11:20:19 +0100 Subject: [PATCH 03/17] debug output --- kernel/objects/capability-spinning/objects/CapEntry.cc | 2 ++ .../capability-spinning/objects/RevokeOperation.cc | 5 +++-- kernel/objects/memory-amd64/objects/PageMapAmd64.cc | 8 +++++--- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index 7e8f1914..ab564c22 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -80,6 +80,7 @@ namespace mythos { optional CapEntry::moveTo(CapEntry& other) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), DVAR(other)); ASSERT(other.cap().isAllocated()); ASSERT(!other.isLinked()); if (!lock_prev()) { @@ -129,6 +130,7 @@ namespace mythos { { auto next = Link(_next).withoutFlags(); auto prev = Link(_prev).withoutFlags(); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); next->_prev.store(prev.value()); prev->_next.store(next.value()); _prev.store(Link().value()); diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.cc b/kernel/objects/capability-spinning/objects/RevokeOperation.cc index bfbaff3d..3bad991e 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.cc +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.cc @@ -106,6 +106,7 @@ namespace mythos { optional RevokeOperation::_delete(CapEntry* root, Cap rootCap) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(root), DVAR(rootCap)); CapEntry* leaf; do { if (_startTraversal(root, rootCap)) { @@ -145,7 +146,7 @@ namespace mythos { bool RevokeOperation::_startTraversal(CapEntry* root, Cap rootCap) { - MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__ ); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(root), DVAR(rootCap) ); if (!root->lock_prev()) { // start is currently unlinked // must be the work of another deleter ... success! @@ -172,7 +173,7 @@ namespace mythos { CapEntry* RevokeOperation::_findLockedLeaf(CapEntry* root) { - MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__ ); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(root) ); auto curEntry = root; while (true) { auto curCap = curEntry->cap(); diff --git a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc index 0b19a808..3f54a4a5 100644 --- a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc +++ b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc @@ -90,16 +90,18 @@ namespace mythos { for (size_t i = num_caps(); i < TABLE_SIZE; ++i) MLOG_INFO(mlog::cap, i, _pm_table(i)); } - optional PageMap::MappedFrame::deleteCap(CapEntry& entry, Cap /*self*/, IDeleter&) + optional PageMap::MappedFrame::deleteCap(CapEntry& entry, Cap self, IDeleter&) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(entry), DVAR(self), DVARhex(this)); auto idx = &entry - &map->_cap_table(0); MLOG_DETAIL(mlog::cap, "delete mapped Frame", DVAR(idx)); map->_pm_table(idx).reset(); RETURN(Error::SUCCESS); } - optional PageMap::MappedPageMap::deleteCap(CapEntry& entry, Cap /*self*/, IDeleter&) + optional PageMap::MappedPageMap::deleteCap(CapEntry& entry, Cap self, IDeleter&) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(entry), DVAR(self), DVARhex(this)); auto idx = &entry - &map->_cap_table(0); MLOG_ERROR(mlog::cap, "delete mapped PageMap", DVAR(idx)); map->_pm_table(idx).reset(); @@ -108,7 +110,7 @@ namespace mythos { optional PageMap::deleteCap(CapEntry&, Cap self, IDeleter& del) { - MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, self); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, self, DVARhex(this)); if (self.isOriginal()) { MLOG_ERROR(mlog::cap, "delete PageMap", self); for (size_t i = 0; i < num_caps(); ++i) { From 418ba4502ec064cdb4a1683497bf14b64cc9c828 Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Fri, 26 Feb 2021 11:38:13 +0100 Subject: [PATCH 04/17] debug output --- kernel/objects/capability-spinning/objects/CapEntry.cc | 3 +++ kernel/objects/capability-spinning/objects/CapEntry.hh | 1 + 2 files changed, 4 insertions(+) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index ab564c22..db56b64d 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -32,6 +32,7 @@ namespace mythos { void CapEntry::initRoot(Cap c) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); ASSERT(isKernelAddress(this)); ASSERT(c.isUsable()); ASSERT(cap().isEmpty()); @@ -62,6 +63,7 @@ namespace mythos { void CapEntry::reset() { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); ASSERT(isUnlinked() || cap().isAllocated()); _prev.store(Link().value()); _next.store(Link().value()); @@ -128,6 +130,7 @@ namespace mythos { optional CapEntry::unlink() { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); auto next = Link(_next).withoutFlags(); auto prev = Link(_prev).withoutFlags(); MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index 1bf4e9b0..c620a2b4 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -199,6 +199,7 @@ namespace mythos { CapEntry& targetEntry, Cap targetCap, COMMITFUN const& commit) { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), DVAR(parentCap), DVAR(targetEntry), DVAR(targetCap)); ASSERT(isKernelAddress(this)); ASSERT(targetEntry.cap().isAllocated()); lock(); // lock the parent entry, the child is already acquired From 9d9c9d73686208f09ecab89b9e5a4ce14c67949e Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Fri, 26 Feb 2021 11:46:57 +0100 Subject: [PATCH 05/17] rename CapEntry::unlink --- kernel/objects/capability-spinning/objects/CapEntry.cc | 3 +-- kernel/objects/capability-spinning/objects/CapEntry.hh | 2 +- kernel/objects/capability-spinning/objects/RevokeOperation.cc | 2 +- kernel/objects/capability-utils/objects/ops.hh | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index db56b64d..f9b889c7 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -128,12 +128,11 @@ namespace mythos { return true; } - optional CapEntry::unlink() + optional CapEntry::unlinkAndUnlockPrev() { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); auto next = Link(_next).withoutFlags(); auto prev = Link(_prev).withoutFlags(); - MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); next->_prev.store(prev.value()); prev->_next.store(next.value()); _prev.store(Link().value()); diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index c620a2b4..3229d7bd 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -83,7 +83,7 @@ namespace mythos { * allocated. Returns true if zombified. */ bool kill(); - optional unlink(); + optional unlinkAndUnlockPrev(); bool try_lock() { bool ret = !(_next.fetch_or(LOCKED_FLAG) & LOCKED_FLAG); MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), ret? " locked" : "locking failed!"); diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.cc b/kernel/objects/capability-spinning/objects/RevokeOperation.cc index 3bad991e..25199d01 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.cc +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.cc @@ -129,7 +129,7 @@ namespace mythos { } auto delRes = leafCap.getPtr()->deleteCap(*leaf, leafCap, *this); if (delRes) { - leaf->unlink(); + leaf->unlinkAndUnlockPrev(); leaf->reset(); } else { // Either tried to delete a portal that is currently deleting diff --git a/kernel/objects/capability-utils/objects/ops.hh b/kernel/objects/capability-utils/objects/ops.hh index a0cc8a9f..11007e92 100644 --- a/kernel/objects/capability-utils/objects/ops.hh +++ b/kernel/objects/capability-utils/objects/ops.hh @@ -75,7 +75,7 @@ namespace mythos { if (!dst.lock_prev()) return true; // was already unlinked, should be empty eventually dst.lock(); dst.kill(); // kill again because someone might have inserted something usable meanwhile - dst.unlink(); + dst.unlinkAndUnlockPrev(); fun(); // perform some caller-defined action while still in an exclusive state dst.reset(); // this markes the entry as writeable again, leaves exclusive state return true; From 9be691677164c4f6af9bd6926f4fd7561bc6f696 Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Fri, 26 Feb 2021 11:59:43 +0100 Subject: [PATCH 06/17] capentry unlock debug output --- kernel/objects/capability-spinning/objects/CapEntry.cc | 8 ++++++++ kernel/objects/capability-spinning/objects/CapEntry.hh | 7 ++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index f9b889c7..d902af8a 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -37,7 +37,9 @@ namespace mythos { ASSERT(c.isUsable()); ASSERT(cap().isEmpty()); Link loopLink(this); + MLOG_ERROR(mlog::cap, "this unlocks _next"); _next.store(loopLink.value()); + MLOG_ERROR(mlog::cap, "this unlocks _prev"); _prev.store(loopLink.value()); _cap.store(c.value()); } @@ -65,6 +67,7 @@ namespace mythos { { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); ASSERT(isUnlinked() || cap().isAllocated()); + MLOG_ERROR(mlog::cap, "this unlocks _prev"); _prev.store(Link().value()); _next.store(Link().value()); // mark as empty @@ -108,7 +111,9 @@ namespace mythos { other._prev.store(prev.value()); prev->_next.store(Link(&other).value()); other.commit(thisCap); + MLOG_ERROR(mlog::cap, "this unlocks _prev"); _prev.store(Link().value()); + MLOG_ERROR(mlog::cap, "this unlocks _next"); _next.store(Link().value()); _cap.store(Cap().value()); RETURN(Error::SUCCESS); @@ -135,7 +140,9 @@ namespace mythos { auto prev = Link(_prev).withoutFlags(); next->_prev.store(prev.value()); prev->_next.store(next.value()); + MLOG_ERROR(mlog::cap, "this unlocks _prev"); _prev.store(Link().value()); + MLOG_ERROR(mlog::cap, "this unlocks _next"); _next.store(Link().value()); RETURN(Error::SUCCESS); } @@ -151,6 +158,7 @@ namespace mythos { if (Link(_prev.load()).ptr() == prev) { return Error::SUCCESS; } else { // my _prev has changed in the mean time + MLOG_ERROR(mlog::cap, "this unlocks prev"); prev->unlock(); return Error::RETRY; } diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index 3229d7bd..c69b36d3 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -108,7 +108,9 @@ namespace mythos { Error try_lock_prev(); bool lock_prev(); - void unlock_prev() { Link(_prev)->unlock(); } + void unlock_prev() { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); + Link(_prev)->unlock(); } CapEntry* next() { @@ -216,10 +218,13 @@ namespace mythos { auto next = Link(_next.load()).withoutFlags(); next->setPrevPreserveFlags(&targetEntry); + MLOG_ERROR(mlog::cap, "this unlocks _next remotely ", DVAR(targetEntry)); targetEntry._next.store(next.value()); // deleted or revoking can not be set in target._prev // as we allocated target for being inserted + MLOG_ERROR(mlog::cap, "this unlocks _prev remotely ", DVAR(targetEntry)); targetEntry._prev.store(Link(this).value()); + MLOG_ERROR(mlog::cap, "this unlocks _next "); this->_next.store(Link(&targetEntry).value()); // unlocks the parent entry targetEntry.commit(targetCap); // release the target entry as usable RETURN(Error::SUCCESS); From 7da68f2e10e58335bb3b2308ac2a3cc821cde7f0 Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Fri, 26 Feb 2021 16:19:45 +0100 Subject: [PATCH 07/17] renames lock to lock_next. --- .../capability-spinning/objects/CapEntry.cc | 8 +-- .../capability-spinning/objects/CapEntry.hh | 51 ++++++++++++------- .../objects/RevokeOperation.cc | 18 +++---- .../objects/capability-utils/objects/ops.hh | 2 +- .../memory-amd64/objects/PageMapAmd64.cc | 2 +- 5 files changed, 47 insertions(+), 34 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index d902af8a..eeb9d5b6 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -92,11 +92,11 @@ namespace mythos { other.reset(); THROW(Error::GENERIC_ERROR); } - lock(); + lock_next(); auto thisCap = cap(); if (isRevoking() || !thisCap.isUsable()) { other.reset(); - unlock(); + unlock_next(); unlock_prev(); THROW(Error::INVALID_CAPABILITY); } @@ -154,12 +154,12 @@ namespace mythos { if (!prev) { return Error::GENERIC_ERROR; } - if (prev->try_lock()) { + if (prev->try_lock_next()) { if (Link(_prev.load()).ptr() == prev) { return Error::SUCCESS; } else { // my _prev has changed in the mean time MLOG_ERROR(mlog::cap, "this unlocks prev"); - prev->unlock(); + prev->unlock_next(); return Error::RETRY; } } else return Error::RETRY; diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index c69b36d3..b4528eab 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -84,37 +84,50 @@ namespace mythos { bool kill(); optional unlinkAndUnlockPrev(); - bool try_lock() { + + /* lock next functions protect the link to the next CapEntry */ + + bool try_lock_next() + { bool ret = !(_next.fetch_or(LOCKED_FLAG) & LOCKED_FLAG); MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), ret? " locked" : "locking failed!"); - return ret; } - void lock() { + return ret; + } + + void lock_next() + { int loop = 0; - while (!try_lock()) { - hwthread_pause(); - loop++; - if(loop > 2){ - MLOG_ERROR(mlog::cap, " lockeing failed too many times -> fail"); - while(1); + while (!try_lock_next()) { + hwthread_pause(); +#warning remove counting for production + loop++; + PANIC_MSG(loop < 2," locking failed too many times"); } - } } - void unlock() { + } + void unlock_next() + { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); - auto res = _next.fetch_and(~LOCKED_FLAG); ASSERT(res & LOCKED_FLAG); } + auto res = _next.fetch_and(~LOCKED_FLAG); + ASSERT(res & LOCKED_FLAG); + } /// only for assertions and debugging /// only trust the result if it is false and it should be true - bool is_locked() const { return _next.load() & CapEntry::LOCKED_FLAG; } + bool next_is_locked() const { return _next.load() & CapEntry::LOCKED_FLAG; } + + /* lock prev functions protect the link to the next CapEntry */ Error try_lock_prev(); bool lock_prev(); - void unlock_prev() { + void unlock_prev() + { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); - Link(_prev)->unlock(); } + Link(_prev)->unlock_next(); + } CapEntry* next() { - ASSERT(is_locked()); + ASSERT(next_is_locked()); return Link(_next).ptr(); } @@ -204,11 +217,11 @@ namespace mythos { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), DVAR(parentCap), DVAR(targetEntry), DVAR(targetCap)); ASSERT(isKernelAddress(this)); ASSERT(targetEntry.cap().isAllocated()); - lock(); // lock the parent entry, the child is already acquired + lock_next(); // lock the parent entry, the child is already acquired auto curCap = cap(); // lazy-locking: check that we still operate on the same parent capability if (!curCap.isUsable() || curCap != parentCap) { - unlock(); // unlock the parent entry + unlock_next(); // unlock the parent entry targetEntry.reset(); // release exclusive usage and revert to an empty entry THROW(Error::LOST_RACE); } @@ -239,7 +252,7 @@ namespace mythos { if (entry.isLinked()) out << ":linked"; if (entry.isDeleted()) out << ":deleted"; if (entry.isUnlinked()) out << ":unlinked"; - if (entry.is_locked()) out << ":locked"; + if (entry.next_is_locked()) out << ":next_locked"; if (entry.isRevoking()) out << ":revoking"; return out; } diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.cc b/kernel/objects/capability-spinning/objects/RevokeOperation.cc index 25199d01..8327725e 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.cc +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.cc @@ -85,11 +85,11 @@ namespace mythos { monitor.requestDone(); return; } - entry.lock(); + entry.lock_next(); auto rootCap = entry.cap(); if (!rootCap.isUsable()) { // this is not the cap you are locking for ... - entry.unlock(); + entry.unlock_next(); res->response(t, Error::LOST_RACE); release(); monitor.requestDone(); @@ -99,7 +99,7 @@ namespace mythos { // if some other revoke or delete clears the flag or changes the cap values // all children have been deleted in the mean time and we are done entry.setRevoking(); - entry.unlock(); + entry.unlock_next(); _result = _delete(&entry, rootCap).state(); _startAsyncDelete(t); } @@ -115,14 +115,14 @@ namespace mythos { if (leaf == root && !rootCap.isZombie()) { // this is a revoke, do not delete the root. no more children -> we are done root->finishRevoke(); - root->unlock(); + root->unlock_next(); root->unlock_prev(); RETURN(Error::SUCCESS); } auto leafCap = leaf->cap(); ASSERT(leafCap.isZombie()); if (leafCap.getPtr() == _guarded) { - leaf->unlock(); + leaf->unlock_next(); leaf->unlock_prev(); // attempted to delete guarded object THROW(Error::CYCLIC_DEPENDENCY); @@ -134,7 +134,7 @@ namespace mythos { } else { // Either tried to delete a portal that is currently deleting // or tried to to delete _guarded via a recursive call. - leaf->unlock(); + leaf->unlock_next(); leaf->unlock_prev(); RETHROW(delRes); } @@ -159,12 +159,12 @@ namespace mythos { root->unlock_prev(); return false; } - root->lock(); + root->lock_next(); // compare only value, not the zombie state if (root->cap().asZombie() != rootCap.asZombie()) { // start has a new value // must be the work of another deleter ... success! - root->unlock(); + root->unlock_next(); root->unlock_prev(); return false; } @@ -190,7 +190,7 @@ namespace mythos { // go to next child curEntry->unlock_prev(); nextEntry->kill(); - nextEntry->lock(); + nextEntry->lock_next(); curEntry = nextEntry; continue; } else return curEntry; diff --git a/kernel/objects/capability-utils/objects/ops.hh b/kernel/objects/capability-utils/objects/ops.hh index 11007e92..1f846a7c 100644 --- a/kernel/objects/capability-utils/objects/ops.hh +++ b/kernel/objects/capability-utils/objects/ops.hh @@ -73,7 +73,7 @@ namespace mythos { { if (!dst.kill()) return false; // not killable (allocated but not usable) if (!dst.lock_prev()) return true; // was already unlinked, should be empty eventually - dst.lock(); + dst.lock_next(); dst.kill(); // kill again because someone might have inserted something usable meanwhile dst.unlinkAndUnlockPrev(); fun(); // perform some caller-defined action while still in an exclusive state diff --git a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc index 3f54a4a5..2d14cb11 100644 --- a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc +++ b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc @@ -114,7 +114,7 @@ namespace mythos { if (self.isOriginal()) { MLOG_ERROR(mlog::cap, "delete PageMap", self); for (size_t i = 0; i < num_caps(); ++i) { - if(_cap_table(i).is_locked()) MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, "table is locked and cannot be deleted! -> deadlock detected!"); + if(_cap_table(i).next_is_locked()) MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, "table is locked and cannot be deleted! -> deadlock detected!"); auto res = del.deleteEntry(_cap_table(i)); ASSERT_MSG(res, "Mapped entries must be deletable."); if (!res) RETHROW(res); From 74f3bcc59f76c5ae4102e406f7e75586aed45df0 Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Sat, 27 Feb 2021 07:33:13 +0100 Subject: [PATCH 08/17] PML4 broadcast page fault test --- kernel/app/init-example/app/init.cc | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/kernel/app/init-example/app/init.cc b/kernel/app/init-example/app/init.cc index 1bb8259a..fa5ad6f2 100644 --- a/kernel/app/init-example/app/init.cc +++ b/kernel/app/init-example/app/init.cc @@ -517,7 +517,24 @@ void test_process(){ MLOG_INFO(mlog::app, "Test process finished"); } -void pageMapBug(){ +void pageMapPageFault(){ + MLOG_INFO(mlog::app, "Trigger PageMap page fault"); + + mythos::PortalLock pl(portal); + + mythos::PageMap pm4(capAlloc()); + + auto res = pm4.create(pl, kmem, 4).wait(); + ASSERT(res); + + MLOG_INFO(mlog::app, "Try to delete pm4 -> page fault"); + + capAlloc.free(pm4.cap(), pl); + + MLOG_INFO(mlog::app, "If you can read this, you might have fixed the page fault?!"); +} + +void pageMapDeadlock(){ MLOG_INFO(mlog::app, "Trigger PageMap deadlock"); mythos::PortalLock pl(portal); @@ -567,7 +584,8 @@ int main() //test_processor_allocator(); //test_process(); //test_CgaScreen(); - pageMapBug(); + pageMapPageFault(); + pageMapDeadlock(); char const end[] = "bye, cruel world!"; mythos::syscall_debug(end, sizeof(end)-1); From 7e8d78a707e52b273559445b8b6983c74f86293c Mon Sep 17 00:00:00 2001 From: Philipp Gypser Date: Sat, 27 Feb 2021 07:33:39 +0100 Subject: [PATCH 09/17] fixed PML4 broadcast page fault --- kernel/boot/apboot-common/boot/DeployHWThread.hh | 2 ++ .../memory-amd64/objects/PML4InvalidationBroadcastAmd64.cc | 1 + 2 files changed, 3 insertions(+) diff --git a/kernel/boot/apboot-common/boot/DeployHWThread.hh b/kernel/boot/apboot-common/boot/DeployHWThread.hh index 946fc25c..e5be3b3f 100644 --- a/kernel/boot/apboot-common/boot/DeployHWThread.hh +++ b/kernel/boot/apboot-common/boot/DeployHWThread.hh @@ -38,6 +38,7 @@ #include "cpu/idle.hh" #include "async/Place.hh" #include "objects/DeleteBroadcast.hh" +#include "objects/PML4InvalidationBroadcastAmd64.hh" #include "objects/SchedulingContext.hh" #include "objects/InterruptControl.hh" #include "boot/memory-layout.h" @@ -78,6 +79,7 @@ struct DeployHWThread { idt.init(); DeleteBroadcast::init(); // depends on hwthread enumeration + PML4InvalidationBroadcast::init(); // depends on hwthread enumeration } void prepare(cpu::ThreadID threadID, cpu::ApicID apicID) diff --git a/kernel/objects/memory-amd64/objects/PML4InvalidationBroadcastAmd64.cc b/kernel/objects/memory-amd64/objects/PML4InvalidationBroadcastAmd64.cc index 9129a0b6..1a0cfcac 100644 --- a/kernel/objects/memory-amd64/objects/PML4InvalidationBroadcastAmd64.cc +++ b/kernel/objects/memory-amd64/objects/PML4InvalidationBroadcastAmd64.cc @@ -58,6 +58,7 @@ namespace mythos { } // propagate PML4InvalidationBroadcast* pnext = this->next; + ASSERT(pnext); while (pnext != start && pnext->home->getCR3() != pml4) pnext = pnext->next; if (pnext != start) { MLOG_DETAIL(mlog::cap, "relay pml4 invalidation"); From efc4adea174f1b40c0bae94fc9f3c4bac129d205 Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Mon, 1 Mar 2021 11:09:46 +0100 Subject: [PATCH 10/17] adds lock_cap. --- .../capability-spinning/objects/CapEntry.cc | 4 +- .../capability-spinning/objects/CapEntry.hh | 38 ++++++++++++++++++- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index eeb9d5b6..70be5751 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -67,7 +67,6 @@ namespace mythos { { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); ASSERT(isUnlinked() || cap().isAllocated()); - MLOG_ERROR(mlog::cap, "this unlocks _prev"); _prev.store(Link().value()); _next.store(Link().value()); // mark as empty @@ -106,12 +105,11 @@ namespace mythos { next->setPrevPreserveFlags(&other); other._next.store(next.value()); - // deleted or revoking can not be set in other._prev + // deletion, deleted or revoking can not be set in other._prev // as we allocated other for moving other._prev.store(prev.value()); prev->_next.store(Link(&other).value()); other.commit(thisCap); - MLOG_ERROR(mlog::cap, "this unlocks _prev"); _prev.store(Link().value()); MLOG_ERROR(mlog::cap, "this unlocks _next"); _next.store(Link().value()); diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index b4528eab..85e5b384 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -104,6 +104,7 @@ namespace mythos { PANIC_MSG(loop < 2," locking failed too many times"); } } + void unlock_next() { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); @@ -111,6 +112,33 @@ namespace mythos { ASSERT(res & LOCKED_FLAG); } + /* deletion lock functions protect the deletion of a object */ + + bool try_lock_cap() + { + bool ret = !(_prev.fetch_or(LOCKED_FLAG) & LOCKED_FLAG); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), ret? " locked" : "locking failed!"); + return ret; + } + + void lock_cap() + { + int loop = 0; + while (!try_lock_cap()) { + hwthread_pause(); +#warning remove counting for production + loop++; + PANIC_MSG(loop < 2," locking failed too many times"); + } + } + + void unlock_cap() + { + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); + auto res = _prev.fetch_and(~LOCKED_FLAG); + ASSERT(res & LOCKED_FLAG); + } + /// only for assertions and debugging /// only trust the result if it is false and it should be true bool next_is_locked() const { return _next.load() & CapEntry::LOCKED_FLAG; } @@ -144,9 +172,15 @@ namespace mythos { // called by move and insertAfter void setPrevPreserveFlags(CapEntry* ptr); + // lock flag in _next and _prev + // _next protects the link to the next entry (lock_next) + // _prev protects the capability in the entry from being changed (lock_cap) static constexpr uintlink_t LOCKED_FLAG = 1; - static constexpr uintlink_t REVOKING_FLAG = 1 << 1; - static constexpr uintlink_t DELETED_FLAG = 1 << 2; + + // flags describing the entry in _prev + static constexpr uintlink_t REVOKING_FLAG = 1 << 1; // prevents from moving + static constexpr uintlink_t DELETED_FLAG = 1 << 2; // prevents from inserting in soon-to-be-deleted object + static constexpr uintlink_t FLAG_MASK = 7; static_assert((DELETED_FLAG | REVOKING_FLAG | FLAG_MASK) == FLAG_MASK, "prev flags do not fit"); From 9b24ea6fbcb964d763395c04c6058b5b68887be9 Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Mon, 1 Mar 2021 18:47:04 +0100 Subject: [PATCH 11/17] searches for leaf locklessy. --- .../capability-spinning/objects/CapEntry.cc | 30 +++- .../capability-spinning/objects/CapEntry.hh | 26 +-- .../objects/RevokeOperation.cc | 149 ++++++++---------- .../objects/RevokeOperation.hh | 3 +- .../objects/capability-utils/objects/ops.hh | 3 +- .../memory-amd64/objects/PageMapAmd64.cc | 2 +- 6 files changed, 113 insertions(+), 100 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index 70be5751..863d9a6a 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -91,15 +91,18 @@ namespace mythos { other.reset(); THROW(Error::GENERIC_ERROR); } + lock_cap(); lock_next(); auto thisCap = cap(); if (isRevoking() || !thisCap.isUsable()) { other.reset(); unlock_next(); + unlock_cap(); unlock_prev(); THROW(Error::INVALID_CAPABILITY); } + // using these values removes lock auto next= Link(_next).withoutFlags(); auto prev= Link(_prev).withoutFlags(); @@ -108,10 +111,12 @@ namespace mythos { // deletion, deleted or revoking can not be set in other._prev // as we allocated other for moving other._prev.store(prev.value()); + MLOG_ERROR(mlog::cap, "this unlocks prev"); prev->_next.store(Link(&other).value()); other.commit(thisCap); + MLOG_ERROR(mlog::cap, "this unlocks cap"); _prev.store(Link().value()); - MLOG_ERROR(mlog::cap, "this unlocks _next"); + MLOG_ERROR(mlog::cap, "this unlocks next"); _next.store(Link().value()); _cap.store(Cap().value()); RETURN(Error::SUCCESS); @@ -131,15 +136,24 @@ namespace mythos { return true; } - optional CapEntry::unlinkAndUnlockPrev() + bool CapEntry::kill(Cap expected) + { + CapValue expectedValue = expected.value(); + MLOG_DETAIL(mlog::cap, this, ".kill", DVAR(expected)); + return _cap.compare_exchange_strong(expectedValue, expected.asZombie().value()); + } + + + optional CapEntry::unlinkAndUnlockLinks() { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); - auto next = Link(_next).withoutFlags(); - auto prev = Link(_prev).withoutFlags(); - next->_prev.store(prev.value()); - prev->_next.store(next.value()); - MLOG_ERROR(mlog::cap, "this unlocks _prev"); - _prev.store(Link().value()); + auto next = Link(_next); + auto prev = Link(_prev); +#warning this does not preserve the flags of next :( + next->_prev.store(prev.withoutFlags().value()); + MLOG_ERROR(mlog::cap, "this unlocks _next of predecessor"); + prev->_next.store(next.withoutFlags().value()); + _prev.store(Link().withoutPtr().value()); MLOG_ERROR(mlog::cap, "this unlocks _next"); _next.store(Link().value()); RETURN(Error::SUCCESS); diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index 85e5b384..69bacc23 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -83,7 +83,9 @@ namespace mythos { * allocated. Returns true if zombified. */ bool kill(); - optional unlinkAndUnlockPrev(); + bool kill(Cap expected); + + optional unlinkAndUnlockLinks(); /* lock next functions protect the link to the next CapEntry */ @@ -101,7 +103,7 @@ namespace mythos { hwthread_pause(); #warning remove counting for production loop++; - PANIC_MSG(loop < 2," locking failed too many times"); + PANIC_MSG(loop < 3, "locking next failed too many times"); } } @@ -128,7 +130,7 @@ namespace mythos { hwthread_pause(); #warning remove counting for production loop++; - PANIC_MSG(loop < 2," locking failed too many times"); + PANIC_MSG(loop < 3," locking failed too many times"); } } @@ -147,6 +149,7 @@ namespace mythos { Error try_lock_prev(); bool lock_prev(); + void unlock_prev() { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); @@ -155,7 +158,6 @@ namespace mythos { CapEntry* next() { - ASSERT(next_is_locked()); return Link(_next).ptr(); } @@ -197,9 +199,10 @@ namespace mythos { CapEntry* operator->() { ASSERT(ptr()); return ptr(); } Link withFlags(uintlink_t flags) const { return Link(_offset(), flags); } - Link withoutFlags() const { return Link(_offset(), 0); } + Link withoutFlags() const { return withFlags(0); } Link withPtr(CapEntry* ptr) const { return Link(ptr, flags()); } + Link withoutPtr() const { return withPtr(nullptr); } CapEntry* ptr() const { @@ -251,11 +254,15 @@ namespace mythos { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), DVAR(parentCap), DVAR(targetEntry), DVAR(targetCap)); ASSERT(isKernelAddress(this)); ASSERT(targetEntry.cap().isAllocated()); - lock_next(); // lock the parent entry, the child is already acquired + // lock the parent entry, the child is already acquired + lock_cap(); + lock_next(); auto curCap = cap(); // lazy-locking: check that we still operate on the same parent capability if (!curCap.isUsable() || curCap != parentCap) { - unlock_next(); // unlock the parent entry + // unlock the parent entry + unlock_next(); + unlock_cap(); targetEntry.reset(); // release exclusive usage and revert to an empty entry THROW(Error::LOST_RACE); } @@ -265,14 +272,15 @@ namespace mythos { auto next = Link(_next.load()).withoutFlags(); next->setPrevPreserveFlags(&targetEntry); - MLOG_ERROR(mlog::cap, "this unlocks _next remotely ", DVAR(targetEntry)); + // dest was never locked MLOG_ERROR(mlog::cap, "this unlocks next in child", DVAR(targetEntry)); targetEntry._next.store(next.value()); // deleted or revoking can not be set in target._prev // as we allocated target for being inserted - MLOG_ERROR(mlog::cap, "this unlocks _prev remotely ", DVAR(targetEntry)); + // dest was never locked MLOG_ERROR(mlog::cap, "this unlocks cap in child", DVAR(targetEntry)); targetEntry._prev.store(Link(this).value()); MLOG_ERROR(mlog::cap, "this unlocks _next "); this->_next.store(Link(&targetEntry).value()); // unlocks the parent entry + unlock_cap(); targetEntry.commit(targetCap); // release the target entry as usable RETURN(Error::SUCCESS); } diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.cc b/kernel/objects/capability-spinning/objects/RevokeOperation.cc index 8327725e..a2ec3bfe 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.cc +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.cc @@ -85,11 +85,11 @@ namespace mythos { monitor.requestDone(); return; } - entry.lock_next(); + entry.lock_cap(); auto rootCap = entry.cap(); if (!rootCap.isUsable()) { // this is not the cap you are locking for ... - entry.unlock_next(); + entry.unlock_cap(); res->response(t, Error::LOST_RACE); release(); monitor.requestDone(); @@ -99,7 +99,7 @@ namespace mythos { // if some other revoke or delete clears the flag or changes the cap values // all children have been deleted in the mean time and we are done entry.setRevoking(); - entry.unlock_next(); + entry.unlock_cap(); _result = _delete(&entry, rootCap).state(); _startAsyncDelete(t); } @@ -108,92 +108,83 @@ namespace mythos { { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(root), DVAR(rootCap)); CapEntry* leaf; + Cap leafCap; do { - if (_startTraversal(root, rootCap)) { - leaf = _findLockedLeaf(root); - MLOG_ERROR(mlog::cap, "_findLockedLeaf returned", DVAR(*leaf), DVAR(rootCap)); - if (leaf == root && !rootCap.isZombie()) { - // this is a revoke, do not delete the root. no more children -> we are done - root->finishRevoke(); - root->unlock_next(); - root->unlock_prev(); - RETURN(Error::SUCCESS); - } - auto leafCap = leaf->cap(); - ASSERT(leafCap.isZombie()); - if (leafCap.getPtr() == _guarded) { - leaf->unlock_next(); - leaf->unlock_prev(); - // attempted to delete guarded object - THROW(Error::CYCLIC_DEPENDENCY); - } - auto delRes = leafCap.getPtr()->deleteCap(*leaf, leafCap, *this); - if (delRes) { - leaf->unlinkAndUnlockPrev(); - leaf->reset(); - } else { - // Either tried to delete a portal that is currently deleting - // or tried to to delete _guarded via a recursive call. - leaf->unlock_next(); - leaf->unlock_prev(); - RETHROW(delRes); - } - } else RETURN(Error::SUCCESS); // could not restart, must be done + // compare only value, not the zombie state + if (root->cap().asZombie() != rootCap.asZombie()) { + // start has a new value + // must be the work of another deleter ... success! + RETURN(Error::SUCCESS); // could not restart, must be done + } + if (!_findLeaf(root, rootCap, leaf, leafCap)) continue; + MLOG_ERROR(mlog::cap, "_findLockedLeaf returned", DVAR(*leaf), DVAR(rootCap)); + if (leaf == root && !rootCap.isZombie()) { + // this is a revoke, do not delete the root. no more children -> we are done + root->finishRevoke(); + RETURN(Error::SUCCESS); + } + ASSERT(leafCap.isZombie()); + if (leafCap.getPtr() == _guarded) { + leaf->unlock_cap(); + // attempted to delete guarded object + THROW(Error::CYCLIC_DEPENDENCY); + } + leaf->lock_cap(); + if (leaf->cap() != leafCap) { + MLOG_DETAIL(mlog::cap, "leaf cap changed concurrently"); + leaf->unlock_cap(); + continue; + } + auto delRes = leafCap.getPtr()->deleteCap(*leaf, leafCap, *this); + auto prevres = leaf->lock_prev(); + ASSERT_MSG(prevres, "somebody unlinked CapEntry currently in unlinking process"); + leaf->lock_next(); + if (delRes) { + leaf->unlinkAndUnlockLinks(); + leaf->reset(); + } else { + // deletion failed + // Either tried to delete a portal that is currently deleting + // or tried to to delete _guarded via a recursive call. + leaf->unlock_cap(); + RETHROW(delRes); + } } while (leaf != root); // deleted root RETURN(Error::SUCCESS); } - bool RevokeOperation::_startTraversal(CapEntry* root, Cap rootCap) - { - MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(root), DVAR(rootCap) ); - if (!root->lock_prev()) { - // start is currently unlinked - // must be the work of another deleter ... success! - return false; - } - if (root->prev() == root) { - // this is the actual root of the tree - // and has no children - // avoid deadlocks and finish the operation - root->unlock_prev(); - return false; - } - root->lock_next(); - // compare only value, not the zombie state - if (root->cap().asZombie() != rootCap.asZombie()) { - // start has a new value - // must be the work of another deleter ... success! - root->unlock_next(); - root->unlock_prev(); - return false; - } - return true; - } - - CapEntry* RevokeOperation::_findLockedLeaf(CapEntry* root) + // not sure if we even need that locking + bool RevokeOperation::_findLeaf(CapEntry* const root, Cap const rootCap, CapEntry*& leafEntry, Cap& leafCap) { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(root) ); - auto curEntry = root; + leafEntry = root; + leafCap = rootCap; while (true) { - auto curCap = curEntry->cap(); - auto nextEntry = curEntry->next(); - // wait for potencially allocated cap to become usable/zombie - Cap nextCap; - for (nextCap = nextEntry->cap(); - nextCap.isAllocated(); - nextCap = nextEntry->cap()) { - ASSERT(!nextEntry->isDeleted()); - hwthread_pause(); + auto nextEntry = leafEntry->next(); + if (nextEntry) { + Cap nextCap; + // wait for potencially allocated cap to become usable/zombie + for (nextCap = nextEntry->cap(); + nextCap.isAllocated(); + nextCap = nextEntry->cap()) { + ASSERT(!nextEntry->isDeleted()); + hwthread_pause(); + } + if (cap::isParentOf(*leafEntry, leafCap, *nextEntry, nextCap)) { + if (!nextEntry->kill(nextCap)) { + MLOG_DETAIL(mlog::cap, "cap to be killed changed concurrently"); + return false; + } + // go to next child + leafEntry = nextEntry; + leafCap = nextCap.asZombie(); + continue; + } else return true; + } else { + MLOG_DETAIL(mlog::cap, "found dead end scanning for leaf"); + return false; // restart at root } - if (cap::isParentOf(*curEntry, curCap, *nextEntry, nextCap)) { - // go to next child - curEntry->unlock_prev(); - nextEntry->kill(); - nextEntry->lock_next(); - curEntry = nextEntry; - continue; - } else return curEntry; } } diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.hh b/kernel/objects/capability-spinning/objects/RevokeOperation.hh index 6ad3a33c..593f156c 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.hh +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.hh @@ -107,8 +107,7 @@ private: void _deleteObject(Tasklet* t); optional _delete(CapEntry* root, Cap rootCap); - bool _startTraversal(CapEntry* root, Cap rootCap); - CapEntry* _findLockedLeaf(CapEntry* root); + bool _findLeaf(CapEntry* const root, Cap const rootCap, CapEntry*& leaf, Cap& leafCap); void _startAsyncDelete(Tasklet* t); diff --git a/kernel/objects/capability-utils/objects/ops.hh b/kernel/objects/capability-utils/objects/ops.hh index 1f846a7c..83cf5525 100644 --- a/kernel/objects/capability-utils/objects/ops.hh +++ b/kernel/objects/capability-utils/objects/ops.hh @@ -73,9 +73,10 @@ namespace mythos { { if (!dst.kill()) return false; // not killable (allocated but not usable) if (!dst.lock_prev()) return true; // was already unlinked, should be empty eventually + dst.lock_cap(); dst.lock_next(); dst.kill(); // kill again because someone might have inserted something usable meanwhile - dst.unlinkAndUnlockPrev(); + dst.unlinkAndUnlockLinks(); fun(); // perform some caller-defined action while still in an exclusive state dst.reset(); // this markes the entry as writeable again, leaves exclusive state return true; diff --git a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc index 2d14cb11..ad337076 100644 --- a/kernel/objects/memory-amd64/objects/PageMapAmd64.cc +++ b/kernel/objects/memory-amd64/objects/PageMapAmd64.cc @@ -387,7 +387,7 @@ namespace mythos { auto res = visitTables(&_pm_table(0), level(), op); *failaddr = op.failaddr; *faillevel = op.current_level; - RETHROW(res.state()); + RETURN(res.state()); } Error PageMap::invokeInstallMap(Tasklet*, Cap self, IInvocation* msg) From ae1bfc3160af3381671d73a36fb2ac9a844d8977 Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Mon, 1 Mar 2021 19:01:16 +0100 Subject: [PATCH 12/17] fixed overwritten flags. --- kernel/objects/capability-spinning/objects/CapEntry.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index 863d9a6a..93ef532b 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -149,8 +149,7 @@ namespace mythos { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this)); auto next = Link(_next); auto prev = Link(_prev); -#warning this does not preserve the flags of next :( - next->_prev.store(prev.withoutFlags().value()); + next->setPrevPreserveFlags(prev.ptr()); MLOG_ERROR(mlog::cap, "this unlocks _next of predecessor"); prev->_next.store(next.withoutFlags().value()); _prev.store(Link().withoutPtr().value()); From 9469d8baf56c5fd3dd132cc4546789939c806117 Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Mon, 1 Mar 2021 19:30:16 +0100 Subject: [PATCH 13/17] fixes lock order. --- kernel/objects/capability-spinning/objects/CapEntry.cc | 5 +++-- kernel/objects/capability-utils/objects/ops.hh | 5 ++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index 93ef532b..363ff9a7 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -87,18 +87,19 @@ namespace mythos { MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), DVAR(other)); ASSERT(other.cap().isAllocated()); ASSERT(!other.isLinked()); + lock_cap(); if (!lock_prev()) { + unlock_cap(); other.reset(); THROW(Error::GENERIC_ERROR); } - lock_cap(); lock_next(); auto thisCap = cap(); if (isRevoking() || !thisCap.isUsable()) { other.reset(); unlock_next(); - unlock_cap(); unlock_prev(); + unlock_cap(); THROW(Error::INVALID_CAPABILITY); } diff --git a/kernel/objects/capability-utils/objects/ops.hh b/kernel/objects/capability-utils/objects/ops.hh index 83cf5525..068c3074 100644 --- a/kernel/objects/capability-utils/objects/ops.hh +++ b/kernel/objects/capability-utils/objects/ops.hh @@ -72,8 +72,11 @@ namespace mythos { bool resetReference(CapEntry& dst, const COMMITFUN& fun) { if (!dst.kill()) return false; // not killable (allocated but not usable) - if (!dst.lock_prev()) return true; // was already unlinked, should be empty eventually dst.lock_cap(); + if (!dst.lock_prev()) { + dst.unlock_cap(); + return true; // was already unlinked, should be empty eventually + } dst.lock_next(); dst.kill(); // kill again because someone might have inserted something usable meanwhile dst.unlinkAndUnlockLinks(); From 0cb2143fdd6429eb1afe573bebee6182411186fb Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Mon, 1 Mar 2021 19:54:15 +0100 Subject: [PATCH 14/17] adds a bit more documentation. --- .../capability-spinning/objects/CapEntry.hh | 10 +++++++- .../objects/RevokeOperation.hh | 23 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index 69bacc23..c0eba6b6 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -44,7 +44,15 @@ namespace mythos { * thus prev and next of root are locked independently * * operations that write to the capability - * or flags (zombie) must lock both next and prev + * or call deleteCap on an obejct must lock_cap + * + * lock order: lock_cap, lock_prev, lock_next + * + * acquired status means someone exclusively acquired an unlinked CapEntry + * therefore no races with others trying to insert it. + * + * acquired status must be only hold shortly + * */ class CapEntry { diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.hh b/kernel/objects/capability-spinning/objects/RevokeOperation.hh index 593f156c..0ec3c42b 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.hh +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.hh @@ -39,6 +39,29 @@ namespace mythos { +/** + * The revoke operation consists of 2 phases seperated + * by an invalidation broadcast. + * + * 1. synchronous phase removes entries from the capability tree + * 2. delete broadcast ensures there are no more references in stack + * 3. asynchronous phase interacts with a KM to recycle objects + * + * 1. synchronous phase + * + * - Traverses the tree starting from an node (root of that subtree). + * - this traverses the list without locking + * - It finds a leaf, killing all the capabilities inbetween. + * - the leaf is deleted by killing deleteCap of the object (protected by lock_cap). + * if that succeedes, the CapEntry MUST be removed from the tree, as we can't do that twice + * and can't hold lock_cap forever. + * - If there are problems because of concurrent access, the operation + * restarts at the root of the subtree. + * - If we can't fix problems synchronously, we switch to asynch. phase + * without finishing, reporting "Error::RETRY" to the user. + * - a guarded object is the object containing the RevokeOperation, it can't be deleted + * + */ class RevokeOperation : public IResult , protected IDeleter From 21e6d93ab7317acb8b67054cd39397ec9f53125b Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Tue, 2 Mar 2021 13:10:10 +0100 Subject: [PATCH 15/17] updates try_lock code. --- .../capability-spinning/objects/CapEntry.cc | 22 +++++++++---------- .../capability-spinning/objects/CapEntry.hh | 21 +++++++++++++----- .../objects/RevokeOperation.cc | 2 +- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/CapEntry.cc b/kernel/objects/capability-spinning/objects/CapEntry.cc index 363ff9a7..feb723db 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.cc +++ b/kernel/objects/capability-spinning/objects/CapEntry.cc @@ -137,11 +137,15 @@ namespace mythos { return true; } - bool CapEntry::kill(Cap expected) + // fails if cap was changed concurrently + bool CapEntry::try_kill(Cap expected) { CapValue expectedValue = expected.value(); - MLOG_DETAIL(mlog::cap, this, ".kill", DVAR(expected)); - return _cap.compare_exchange_strong(expectedValue, expected.asZombie().value()); + MLOG_DETAIL(mlog::cap, this, ".try_kill", DVAR(expected)); + if (!_cap.compare_exchange_strong(expectedValue, expected.asZombie().value())) { + // if the cap was just zombified by sb. else, thats okay + return (Cap(expectedValue).asZombie() == expected.asZombie()); + } else return true; } @@ -166,15 +170,9 @@ namespace mythos { if (!prev) { return Error::GENERIC_ERROR; } - if (prev->try_lock_next()) { - if (Link(_prev.load()).ptr() == prev) { - return Error::SUCCESS; - } else { // my _prev has changed in the mean time - MLOG_ERROR(mlog::cap, "this unlocks prev"); - prev->unlock_next(); - return Error::RETRY; - } - } else return Error::RETRY; + auto success = prev->try_lock_next(this); + ASSERT(Link(_prev.load()).ptr() == prev); + return success ? Error::SUCCESS : Error::RETRY; } bool CapEntry::lock_prev() diff --git a/kernel/objects/capability-spinning/objects/CapEntry.hh b/kernel/objects/capability-spinning/objects/CapEntry.hh index c0eba6b6..f8539e9f 100644 --- a/kernel/objects/capability-spinning/objects/CapEntry.hh +++ b/kernel/objects/capability-spinning/objects/CapEntry.hh @@ -91,19 +91,22 @@ namespace mythos { * allocated. Returns true if zombified. */ bool kill(); - bool kill(Cap expected); + bool try_kill(Cap expected); optional unlinkAndUnlockLinks(); /* lock next functions protect the link to the next CapEntry */ - bool try_lock_next() + bool try_lock_next(CapEntry* next) { - bool ret = !(_next.fetch_or(LOCKED_FLAG) & LOCKED_FLAG); - MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), ret? " locked" : "locking failed!"); - return ret; + Link expected(next); + uintlink_t expectedValue = expected.value(); + auto ret = _next.compare_exchange_strong(expectedValue, expected.withFlags(LOCKED_FLAG).value()); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), ret? " locked" : "locking failed!"); + return ret; } + void lock_next() { int loop = 0; @@ -182,6 +185,14 @@ namespace mythos { // called by move and insertAfter void setPrevPreserveFlags(CapEntry* ptr); + // called by lock_next + bool try_lock_next() + { + bool ret = !(_next.fetch_or(LOCKED_FLAG) & LOCKED_FLAG); + MLOG_ERROR(mlog::cap, __PRETTY_FUNCTION__, DVAR(this), ret? " locked" : "locking failed!"); + return ret; + } + // lock flag in _next and _prev // _next protects the link to the next entry (lock_next) // _prev protects the capability in the entry from being changed (lock_cap) diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.cc b/kernel/objects/capability-spinning/objects/RevokeOperation.cc index a2ec3bfe..de63682d 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.cc +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.cc @@ -172,7 +172,7 @@ namespace mythos { hwthread_pause(); } if (cap::isParentOf(*leafEntry, leafCap, *nextEntry, nextCap)) { - if (!nextEntry->kill(nextCap)) { + if (!nextEntry->try_kill(nextCap)) { MLOG_DETAIL(mlog::cap, "cap to be killed changed concurrently"); return false; } From c7c08d788f16b11965f212dc17db2e51000694f5 Mon Sep 17 00:00:00 2001 From: Robert Kuban Date: Tue, 2 Mar 2021 13:15:30 +0100 Subject: [PATCH 16/17] updates comment. --- .../objects/capability-spinning/objects/RevokeOperation.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/kernel/objects/capability-spinning/objects/RevokeOperation.cc b/kernel/objects/capability-spinning/objects/RevokeOperation.cc index de63682d..b8580dd9 100644 --- a/kernel/objects/capability-spinning/objects/RevokeOperation.cc +++ b/kernel/objects/capability-spinning/objects/RevokeOperation.cc @@ -143,9 +143,8 @@ namespace mythos { leaf->unlinkAndUnlockLinks(); leaf->reset(); } else { - // deletion failed - // Either tried to delete a portal that is currently deleting - // or tried to to delete _guarded via a recursive call. + // deletion failed in the object specific handler + // this can be also from trying to delete rhw guarded object (currently deleting portal) leaf->unlock_cap(); RETHROW(delRes); } From abed1d90a1f564610bc9872770c26ea15cca9e2a Mon Sep 17 00:00:00 2001 From: Randolf Rotta Date: Thu, 22 Jul 2021 13:57:33 +0200 Subject: [PATCH 17/17] reduced failing test set --- kernel/app/init-example/app/init.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/app/init-example/app/init.cc b/kernel/app/init-example/app/init.cc index cb294621..7cd1345c 100644 --- a/kernel/app/init-example/app/init.cc +++ b/kernel/app/init-example/app/init.cc @@ -592,14 +592,14 @@ int main() //test_InterruptControl(); //test_HostChannel(portal, 24*1024*1024, 2*1024*1024); test_ExecutionContext(); - test_pthreads(); - test_Rapl(); - test_processor_allocator(); + //test_pthreads(); + //test_Rapl(); + //test_processor_allocator(); //test_process(); //test_CgaScreen(); + testCapMapDeletion(); pageMapPageFault(); pageMapDeadlock(); - testCapMapDeletion(); char const end[] = "bye, cruel world!"; mythos::syscall_debug(end, sizeof(end)-1);