[kernel][object] Fix port/eport lock order violation
Link/unlink eports from their ports under the eport lock and have the
port call a new locked variant of PortMatches so the eport lock is
always acquired first.
Test: CQ
ZX-2517 #done
Change-Id: I96c24ac61052794113513a0b0aec266c5c78a704
diff --git a/kernel/object/excp_port.cpp b/kernel/object/excp_port.cpp
index 914f5cd..06dcdbe 100644
--- a/kernel/object/excp_port.cpp
+++ b/kernel/object/excp_port.cpp
@@ -60,7 +60,9 @@
: type_(type), port_key_(port_key), port_(port) {
LTRACE_ENTRY_OBJ;
DEBUG_ASSERT(port_ != nullptr);
- port_->LinkExceptionPort(this);
+
+ Guard<fbl::Mutex> guard{&lock_};
+ port_->LinkExceptionPortEportLocked(this);
}
ExceptionPort::~ExceptionPort() {
@@ -204,20 +206,22 @@
// We may not have a target if the binding (Set*Target) never happened,
// so don't require that we're bound.
target_.reset();
- }
- // It should actually be safe to hold our lock while calling into
- // the PortDispatcher, but there's no reason to.
- // Unlink ourselves from the PortDispatcher's list.
- // No-op if this method was ultimately called from
- // PortDispatcher:on_zero_handles (via ::OnPortZeroHandles).
- port->UnlinkExceptionPort(this);
+ // Unlink ourselves from the PortDispatcher's list.
+ // No-op if this method was ultimately called from
+ // PortDispatcher:on_zero_handles (via ::OnPortZeroHandles).
+ port->UnlinkExceptionPortEportLocked(this);
+ }
LTRACE_EXIT_OBJ;
}
bool ExceptionPort::PortMatches(const PortDispatcher *port, bool allow_null) {
Guard<fbl::Mutex> guard{&lock_};
+ return PortMatchesLocked(port, allow_null);
+}
+
+bool ExceptionPort::PortMatchesLocked(const PortDispatcher *port, bool allow_null) {
return (allow_null && port_ == nullptr) || port_.get() == port;
}
diff --git a/kernel/object/include/object/excp_port.h b/kernel/object/include/object/excp_port.h
index 97a6a26..b84e445 100644
--- a/kernel/object/include/object/excp_port.h
+++ b/kernel/object/include/object/excp_port.h
@@ -59,6 +59,7 @@
// Validates that this eport is associated with the given instance.
bool PortMatches(const PortDispatcher* port, bool allow_null);
+ bool PortMatchesLocked(const PortDispatcher* port, bool allow_null) TA_REQ(lock_);
static void BuildArchReport(zx_exception_report_t* report, uint32_t type,
const arch_exception_context_t* arch_context);
diff --git a/kernel/object/include/object/port_dispatcher.h b/kernel/object/include/object/port_dispatcher.h
index e7e25ed..a7a8bd5 100644
--- a/kernel/object/include/object/port_dispatcher.h
+++ b/kernel/object/include/object/port_dispatcher.h
@@ -7,6 +7,7 @@
#pragma once
#include <object/dispatcher.h>
+#include <object/excp_port.h>
#include <object/semaphore.h>
#include <object/state_observer.h>
@@ -77,7 +78,6 @@
// when cancellation happens and the port still owns the packet.
//
-class ExceptionPort;
class PortDispatcher;
class PortObserver;
struct PortPacket;
@@ -199,18 +199,18 @@
bool CancelQueued(PortPacket* port_packet);
private:
- friend class ExceptionPort;
+ friend ExceptionPort;
explicit PortDispatcher(uint32_t options);
// Adopts a RefPtr to |eport|, and adds it to |eports_|.
- // Called by ExceptionPort.
- void LinkExceptionPort(ExceptionPort* eport);
+ // Called by ExceptionPort under |eport|'s lock.
+ void LinkExceptionPortEportLocked(ExceptionPort* eport) TA_REQ(eport->lock_);
// Removes |eport| from |eports_|, dropping its RefPtr.
// Does nothing if |eport| is not on the list.
- // Called by ExceptionPort.
- void UnlinkExceptionPort(ExceptionPort* eport);
+ // Called by ExceptionPort under |eport|'s lock.
+ void UnlinkExceptionPortEportLocked(ExceptionPort* eport) TA_REQ(eport->lock_);
fbl::Canary<fbl::magic("PORT")> canary_;
const uint32_t options_;
diff --git a/kernel/object/port_dispatcher.cpp b/kernel/object/port_dispatcher.cpp
index 9fc3cc7..7aefae8 100644
--- a/kernel/object/port_dispatcher.cpp
+++ b/kernel/object/port_dispatcher.cpp
@@ -451,20 +451,20 @@
return false;
}
-void PortDispatcher::LinkExceptionPort(ExceptionPort* eport) {
+void PortDispatcher::LinkExceptionPortEportLocked(ExceptionPort* eport) {
canary_.Assert();
Guard<fbl::Mutex> guard{get_lock()};
- DEBUG_ASSERT_COND(eport->PortMatches(this, /* allow_null */ false));
+ DEBUG_ASSERT_COND(eport->PortMatchesLocked(this, /* allow_null */ false));
DEBUG_ASSERT(!eport->InContainer());
eports_.push_back(ktl::move(AdoptRef(eport)));
}
-void PortDispatcher::UnlinkExceptionPort(ExceptionPort* eport) {
+void PortDispatcher::UnlinkExceptionPortEportLocked(ExceptionPort* eport) {
canary_.Assert();
Guard<fbl::Mutex> guard{get_lock()};
- DEBUG_ASSERT_COND(eport->PortMatches(this, /* allow_null */ true));
+ DEBUG_ASSERT_COND(eport->PortMatchesLocked(this, /* allow_null */ true));
if (eport->InContainer()) {
eports_.erase(*eport);
}
