[kernel][magenta] Fix port bug

When trying to remove the port leaker we end up creating
a kernel crash, which takes some time to repro

The issue is that CanRep() was racing with Dequeue()
when the packet is removed from the queue and the lock
is released. The effect is basically a double free
on the delete observer.

Best repro on the existing code so far is
1- apply PS1 or PS2
2- $ while USER_AUTORUN=autorun ./scripts/run-magenta-x86-64 -b -k; do :;
done
3- wait for ~5 minutes (you need kvm)

with autorun being:
msleep 50
/boot/test/sys/exception-test
msleep 50
dm poweroff

While here we
- remove duplicated fields key_ annd handle_
- add a couple more debug asserts

MG-1093 #done
MG-1094 #done

Change-Id: I9d44f070c5b38bc1a7edbce8c344a0b2edacf6b3

kernel/object/include/object/port_dispatcher.h

@@ -98,9 +98,8 @@ struct PortPacket final : public mxtl::DoublyLinkedListable<PortPacket*> {
     PortPacket(const PortPacket&) = delete;
     void operator=(PortPacket) = delete;
 
-    uint32_t type() const { return packet.type; }
     uint64_t key() const { return packet.key; }
-
+    bool is_ephemeral() const { return allocator != nullptr; }
     void Free() { allocator->Free(this); }
 };
 
@@ -113,11 +112,6 @@ public:
                  uint64_t key, mx_signals_t signals);
     ~PortObserver() = default;
 
-    // Returns void pointer because this method can only be used for comparing
-    // values. Calling a method on the handle will very likely cause a deadlock.
-    const void* handle() const { return handle_; }
-    uint64_t key() const { return key_; }
-
 private:
     PortObserver(const PortObserver&) = delete;
     PortObserver& operator=(const PortObserver&) = delete;
@@ -134,12 +128,10 @@ private:
     Flags MaybeQueue(mx_signals_t new_state, uint64_t count);
 
     const uint32_t type_;
-    const uint64_t key_;
     const mx_signals_t trigger_;
-    const Handle* const handle_;
-    mxtl::RefPtr<PortDispatcher> const port_;
-
     PortPacket packet_;
+
+    mxtl::RefPtr<PortDispatcher> const port_;
 };
 
 class PortDispatcher final : public Dispatcher {
@@ -173,7 +165,6 @@ private:
     friend class ExceptionPort;
 
     explicit PortDispatcher(uint32_t options);
-    PortObserver* CopyLocked(PortPacket* port_packet, mx_port_packet_t* packet) TA_REQ(lock_);
 
     // Adopts a RefPtr to |eport|, and adds it to |eports_|.
     // Called by ExceptionPort.
@@ -184,7 +175,7 @@ private:
     // Called by ExceptionPort.
     void UnlinkExceptionPort(ExceptionPort* eport);
 
-    mxtl::Canary<mxtl::magic("POR2")> canary_;
+    mxtl::Canary<mxtl::magic("PORT")> canary_;
     mxtl::Mutex lock_;
     Semaphore sema_;
     bool zero_handles_ TA_GUARDED(lock_);

kernel/object/port_dispatcher.cpp

@@ -68,22 +68,25 @@ void ArenaPortAllocator::Free(PortPacket* port_packet) {
 PortPacket::PortPacket(const void* handle, PortAllocator* allocator)
     : packet{}, handle(handle), observer(nullptr), allocator(allocator) {
     // Note that packet is initialized to zeros.
+    if (handle) {
+        // Currently |handle| is only valid if the packets are not ephemeral
+        // which means that PortObserver always uses the kernel heap.
+        DEBUG_ASSERT(allocator == nullptr);
+    }
 }
 
 PortObserver::PortObserver(uint32_t type, const Handle* handle, mxtl::RefPtr<PortDispatcher> port,
                            uint64_t key, mx_signals_t signals)
     : type_(type),
-      key_(key),
       trigger_(signals),
-      handle_(handle),
-      port_(mxtl::move(port)),
-      packet_(handle, nullptr) {
+      packet_(handle, nullptr),
+      port_(mxtl::move(port)) {
 
     DEBUG_ASSERT(handle != nullptr);
 
     auto& packet = packet_.packet;
     packet.status = MX_OK;
-    packet.key = key_;
+    packet.key = key;
     packet.type = type_;
     packet.signal.trigger = trigger_;
 }
@@ -108,7 +111,7 @@ StateObserver::Flags PortObserver::OnStateChange(mx_signals_t new_state) {
 }
 
 StateObserver::Flags PortObserver::OnCancel(Handle* handle) {
-    if (handle_ == handle) {
+    if (packet_.handle == handle) {
         return kHandled | kNeedRemoval;
     } else {
         return 0;
@@ -116,7 +119,7 @@ StateObserver::Flags PortObserver::OnCancel(Handle* handle) {
 }
 
 StateObserver::Flags PortObserver::OnCancelByKey(Handle* handle, const void* port, uint64_t key) {
-    if ((key_ != key) || (handle_ != handle) || (port_.get() != port))
+    if ((packet_.handle != handle) || (packet_.key() != key) || (port_.get() != port))
         return 0;
     return kHandled | kNeedRemoval;
 }
@@ -232,26 +235,32 @@ mx_status_t PortDispatcher::Queue(PortPacket* port_packet, mx_signals_t observed
     return MX_OK;
 }
 
-mx_status_t PortDispatcher::Dequeue(mx_time_t deadline, mx_port_packet_t* packet) {
+mx_status_t PortDispatcher::Dequeue(mx_time_t deadline, mx_port_packet_t* out_packet) {
     canary_.Assert();
 
-    PortPacket* port_packet = nullptr;
-    PortObserver* observer = nullptr;
-
     while (true) {
         {
             AutoLock al(&lock_);
-            if (packets_.is_empty())
+
+            PortPacket* port_packet = packets_.pop_front();
+            if (port_packet == nullptr)
                 goto wait;
 
-            port_packet = packets_.pop_front();
-            observer = CopyLocked(port_packet, packet);
-        }
+            if (out_packet != nullptr)
+                *out_packet = port_packet->packet;
 
-        if (observer)
-            delete observer;
-        else if (packet && (packet->type & PKT_FLAG_EPHEMERAL))
-            port_packet->Free();
+            PortObserver* observer = port_packet->observer;
+
+            if (observer) {
+                // Deleting the observer under the lock is fine because
+                // the reference that holds to this PortDispatcher is by
+                // construction not the last one. We need to do this under
+                // the lock because another thread can call CanReap().
+                delete observer;
+            } else if (port_packet->is_ephemeral()) {
+                port_packet->Free();
+            }
+        }
 
         return MX_OK;
 
@@ -262,13 +271,6 @@ wait:
     }
 }
 
-PortObserver* PortDispatcher::CopyLocked(PortPacket* port_packet, mx_port_packet_t* packet) {
-    if (packet)
-        *packet = port_packet->packet;
-
-    return (port_packet->type() & PKT_FLAG_EPHEMERAL) ? nullptr : port_packet->observer;
-}
-
 bool PortDispatcher::CanReap(PortObserver* observer, PortPacket* port_packet) {
     canary_.Assert();