From 488f8888a2b9fde9127daa836a42ce977d9c16b0 Mon Sep 17 00:00:00 2001
From: Augustin Cavalier <waddlesplash@gmail.com>
Date: Thu, 10 Mar 2022 14:27:40 -0500
Subject: [PATCH] network stack: Fix deadlock in get_interface functions.

If these methods are called while the interface in question
is receiving data via a receive thread, we can hit a deadlock
where a receive thread is holding the receive lock and then
tries to call get_interface_address_for_link (due, e.g., to ipv4
checking is_local_link_address), which tries to acquire the interfaces
lock, while at the same time we are trying to acquire the receive lock
due to CreateDomainDatalinkIfNeeded invoking a module's datalink_init
which calls register_device_handler, so we deadlock. (Whew!)

As far as I can tell, we do not need to set Busy() here despite unlocking
the interfaces lock, as the Interface acquires its own lock in
CreateDomainDatalinkIfNeeded.

Observed in VMware when the DHCP client spins for a long time,
and the deadlock occurs upon opening Network preferences.
---
 .../kernel/network/stack/interfaces.cpp       | 32 ++++++++++++++-----
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/add-ons/kernel/network/stack/interfaces.cpp b/src/add-ons/kernel/network/stack/interfaces.cpp
index 60e4a0175c..8921092aa3 100644
--- a/src/add-ons/kernel/network/stack/interfaces.cpp
+++ b/src/add-ons/kernel/network/stack/interfaces.cpp
@@ -1426,11 +1426,16 @@ get_interface(net_domain* domain, uint32 index)
 	if (interface == NULL || interface->IsBusy())
 		return NULL;
 
+	// We must unlock before invoking CreateDomainDatalinkIfNeeded, because
+	// otherwise we can hit lock ordering inversions with receive threads,
+	// usually in register_device_handler.
+	BReference<Interface> interfaceRef(interface);
+	locker.Unlock();
+
 	if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
 		return NULL;
 
-	interface->AcquireReference();
-	return interface;
+	return interfaceRef.Detach();
 }
 
 
@@ -1443,11 +1448,14 @@ get_interface(net_domain* domain, const char* name)
 	if (interface == NULL || interface->IsBusy())
 		return NULL;
 
+	// See comment in get_interface.
+	BReference<Interface> interfaceRef(interface);
+	locker.Unlock();
+
 	if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
 		return NULL;
 
-	interface->AcquireReference();
-	return interface;
+	return interfaceRef.Detach();
 }
 
 
@@ -1461,11 +1469,15 @@ get_interface_for_device(net_domain* domain, uint32 index)
 		if (interface->device->index == index) {
 			if (interface->IsBusy())
 				return NULL;
+
+			// See comment in get_interface.
+			BReference<Interface> interfaceRef(interface);
+			locker.Unlock();
+
 			if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
 				return NULL;
 
-			interface->AcquireReference();
-			return interface;
+			return interfaceRef.Detach();
 		}
 	}
 
@@ -1499,11 +1511,15 @@ get_interface_for_link(net_domain* domain, const sockaddr* _linkAddress)
 				&& linkAddress.sdl_index == interface->index)) {
 			if (interface->IsBusy())
 				return NULL;
+
+			// See comment in get_interface.
+			BReference<Interface> interfaceRef(interface);
+			locker.Unlock();
+
 			if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
 				return NULL;
 
-			interface->AcquireReference();
-			return interface;
+			return interfaceRef.Detach();
 		}
 	}