introduced net_device_interface level locking.

git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@20611 a95241bf-73f2-0310-859d-f6bbb57e9c96

src/add-ons/kernel/network/stack/datalink.cpp

@@ -45,9 +45,13 @@ device_reader_thread(void *_interface)
 	net_device *device = interface->device;
 	status_t status = B_OK;
 
-	while ((device->flags & IFF_UP) != 0) {
+	BenaphoreLocker rx_lock(interface->rx_lock);
+
+	while (device->flags & IFF_UP) {
 		net_buffer *buffer;
+		rx_lock.Unlock();
 		status = device->module->receive_data(device, &buffer);
+		rx_lock.Lock();
 		if (status == B_OK) {
 			//dprintf("received buffer of %ld bytes length\n", buffer->size);
 
@@ -100,10 +104,6 @@ device_reader_thread(void *_interface)
 		// and the receive_data() above should have been
 		// interrupted. One check should be enough, specially
 		// considering the snooze above.
-		//
-		// TODO: make sure that when receive_data() returns
-		//       after closing the new device->flags are
-		//       already visible in all processors.
 	}
 
 	return status;
@@ -180,6 +180,52 @@ add_default_routes(net_interface_private *interface, int32 option)
 }
 
 
+static status_t
+datalink_control_interface(net_domain_private *domain, int32 option,
+	void *value, size_t *_length, size_t expected, bool getByName)
+{
+	if (*_length < expected)
+		return B_BAD_VALUE;
+
+	ifreq request;
+	memset(&request, 0, sizeof(request));
+
+	if (user_memcpy(&request, value, expected) < B_OK)
+		return B_BAD_ADDRESS;
+
+	BenaphoreLocker _(domain->lock);
+	net_interface *interface = NULL;
+
+	if (getByName)
+		interface = find_interface(domain, request.ifr_name);
+	else
+		interface = find_interface(domain, request.ifr_index);
+
+	status_t status = (interface == NULL) ? ENODEV : B_OK;
+
+	switch (option) {
+	case SIOCGIFINDEX:
+		if (interface)
+			request.ifr_index = interface->index;
+		else
+			request.ifr_index = 0;
+		break;
+
+	case SIOCGIFNAME:
+		if (interface)
+			strlcpy(request.ifr_name, interface->name, IF_NAMESIZE);
+		else
+			status = B_BAD_VALUE; // TODO should be ENXIO?
+		break;
+	}
+
+	if (status < B_OK)
+		return status;
+
+	return user_memcpy(value, &request, sizeof(ifreq));
+}
+
+
 //	#pragma mark - datalink module
 
 
@@ -195,51 +241,20 @@ datalink_control(net_domain *_domain, int32 option, void *value,
 
 	switch (option) {
 		case SIOCGIFINDEX:
-		{
+			return datalink_control_interface(domain, option, value, _length,
-			// get index of interface
+				IF_NAMESIZE, true);
-			struct ifreq request;
-			if (user_memcpy(&request, value, IF_NAMESIZE) < B_OK)
-				return B_BAD_ADDRESS;
-
-			benaphore_lock(&domain->lock);
-
-			net_interface *interface = find_interface(domain,
-				request.ifr_name);
-			if (interface != NULL)
-				request.ifr_index = interface->index;
-			else
-				request.ifr_index = 0;
-
-			benaphore_unlock(&domain->lock);
-			
-			if (request.ifr_index == 0)
-				return ENODEV;
-
-			return user_memcpy(value, &request, sizeof(struct ifreq));
-		}
 		case SIOCGIFNAME:
+			return datalink_control_interface(domain, option, value, _length,
+				sizeof(ifreq), false);
+
+		case SIOCDIFADDR:
+		case SIOCSIFFLAGS:
 		{
-			// get name of interface via index
 			struct ifreq request;
 			if (user_memcpy(&request, value, sizeof(struct ifreq)) < B_OK)
 				return B_BAD_ADDRESS;
 
-			benaphore_lock(&domain->lock);
+			return domain_interface_control(domain, option, &request);
-			status_t status = B_OK;
-
-			net_interface *interface = find_interface(domain,
-				request.ifr_index);
-			if (interface != NULL)
-				strlcpy(request.ifr_name, interface->name, IF_NAMESIZE);
-			else
-				status = B_BAD_VALUE;
-
-			benaphore_unlock(&domain->lock);
-
-			if (status < B_OK)
-				return status;
-
-			return user_memcpy(value, &request, sizeof(struct ifreq));
 		}
 
 		case SIOCAIFADDR:
@@ -251,21 +266,6 @@ datalink_control(net_domain *_domain, int32 option, void *value,
 
 			return add_interface_to_domain(domain, request);
 		}
-		case SIOCDIFADDR:
-		{
-			// remove interface address
-			struct ifreq request;
-			if (user_memcpy(&request, value, sizeof(struct ifreq)) < B_OK)
-				return B_BAD_ADDRESS;
-
-			BenaphoreLocker _(domain->lock);
-
-			net_interface *interface = find_interface(domain,
-				request.ifr_name);
-			if (interface == NULL)
-				return ENODEV;
-			return remove_interface_from_domain(interface);
-		}
 
 		case SIOCGIFCOUNT:
 		{
@@ -314,46 +314,20 @@ datalink_control(net_domain *_domain, int32 option, void *value,
 		default:
 		{
 			// try to pass the request to an existing interface
-
 			struct ifreq request;
 			if (user_memcpy(&request, value, sizeof(struct ifreq)) < B_OK)
 				return B_BAD_ADDRESS;
 
 			BenaphoreLocker _(domain->lock);
-			status_t status = B_OK;
 
 			net_interface *interface = find_interface(domain,
 				request.ifr_name);
-			if (interface != NULL) {
+			if (interface == NULL)
-				// filter out bringing the interface up or down 
+				return B_BAD_VALUE;
-				if (option == SIOCSIFFLAGS) {
-					if (((uint32)request.ifr_flags & IFF_UP)
-							!= (interface->flags & IFF_UP)) {
-						if ((interface->flags & IFF_UP) != 0) {
-							interface_set_down(interface);
-						} else {
-							// bring it up
-							status = interface->first_info->interface_up(
-								interface->first_protocol);
-							if (status == B_OK) {
-								interface->flags |= IFF_UP
-									| (interface->device->media & IFM_ACTIVE
-										? IFF_LINK : 0);
-							}
-						}
-					}
 
-					if (status == B_OK)
+			// pass the request into the datalink protocol stack
-						interface->flags |= request.ifr_flags & ~(IFF_UP | IFF_LINK);
+			return interface->first_info->control(
-				} else {
+				interface->first_protocol, option, value, *_length);
-					// pass the request into the datalink protocol stack
-					status = interface->first_info->control(
-						interface->first_protocol, option, value, *_length);
-				}
-			} else
-				status = B_BAD_VALUE;
-
-			return status;
 		}
 	}
 	return B_BAD_VALUE;

src/add-ons/kernel/network/stack/domains.cpp

@@ -10,6 +10,7 @@
 #include "domains.h"
 #include "interfaces.h"
 #include "utility.h"
+#include "stack_private.h"
 
 #include <net_device.h>
 
@@ -21,6 +22,7 @@
 #include <net/if_media.h>
 #include <new>
 #include <string.h>
+#include <sys/sockio.h>
 
 
 #define TRACE_DOMAINS
@@ -196,6 +198,64 @@ remove_interface_from_domain(net_interface *interface)
 }
 
 
+status_t
+domain_interface_control(net_domain_private *domain, int32 option,
+	ifreq *request)
+{
+	const char *name = request->ifr_name;
+	status_t status = B_OK;
+
+	net_device_interface *device = get_device_interface(name, false);
+	if (device == NULL)
+		return ENODEV;
+	else {
+		// The locking protocol dictates that if both the RX lock
+		// and domain locks are required, we MUST obtain the RX
+		// lock before the domain lock. This order MUST NOT ever
+		// be reversed under the penalty of deadlock.
+		BenaphoreLocker _1(device->rx_lock);
+		BenaphoreLocker _2(domain->lock);
+
+		net_interface *interface = find_interface(domain, name);
+		if (interface != NULL) {
+			switch (option) {
+			case SIOCDIFADDR:
+				remove_interface_from_domain(interface);
+				break;
+
+			case SIOCSIFFLAGS:
+				if (((uint32)request->ifr_flags & IFF_UP)
+					!= (interface->flags & IFF_UP)) {
+					if (interface->flags & IFF_UP) {
+						interface_set_down(interface);
+					} else {
+						status = interface->first_info->interface_up(
+							interface->first_protocol);
+						if (status == B_OK) {
+							interface->flags |= IFF_UP;
+							// TODO this doesn't belong here
+							if (interface->device->media & IFM_ACTIVE)
+								interface->flags |= IFF_LINK;
+						}
+					}
+				}
+
+				if (status == B_OK)
+					interface->flags |= request->ifr_flags & ~(IFF_UP | IFF_LINK);
+				break;
+			}
+		}
+	}
+
+	// If the SIOCDIFADDR call above removed the last interface
+	// associated with the device interface, this put_() will
+	// effectively remove the interface
+	put_device_interface(device);
+
+	return status;
+}
+
+
 void
 domain_interfaces_link_changed(net_device *device)
 {

src/add-ons/kernel/network/stack/domains.h

@@ -39,6 +39,8 @@ status_t remove_interface_from_domain(net_interface *interface);
 void domain_interfaces_link_changed(net_device *device);
 void domain_interface_went_down(net_interface *);
 void domain_removed_device_interface(net_device_interface *);
+status_t domain_interface_control(net_domain_private *domain, int32 option,
+	struct ifreq *request);
 
 net_domain *get_domain(int family);
 status_t register_domain(int family, const char *name,

src/add-ons/kernel/network/stack/interfaces.cpp

@@ -325,6 +325,9 @@ put_device_interface(struct net_device_interface *interface)
 
 	interface->module->uninit_device(interface->device);
 	put_module(interface->module->info.name);
+
+	benaphore_destroy(&interface->rx_lock);
+	delete interface;
 }
 
 
@@ -355,7 +358,7 @@ get_device_interface(uint32 index)
 	If the interface does not yet exist, a new one is created.
 */
 struct net_device_interface *
-get_device_interface(const char *name)
+get_device_interface(const char *name, bool create)
 {
 	BenaphoreLocker locker(sInterfaceLock);
 
@@ -367,6 +370,9 @@ get_device_interface(const char *name)
 		// try to recreate interface - it just got removed
 	}
 
+	if (!create)
+		return NULL;
+
 	void *cookie = open_module_list("network/devices");
 	if (cookie == NULL)
 		return NULL;
@@ -387,23 +393,25 @@ get_device_interface(const char *name)
 				// create new module interface for this
 				interface = new (std::nothrow) net_device_interface;
 				if (interface != NULL) {
-					interface->name = device->name;
+					if (benaphore_init(&interface->rx_lock, "rx lock") >= B_OK) {
-					interface->module = module;
+						interface->name = device->name;
-					interface->device = device;
+						interface->module = module;
-					interface->up_count = 0;
+						interface->device = device;
-					interface->ref_count = 1;
+						interface->up_count = 0;
-					interface->deframe_func = NULL;
+						interface->ref_count = 1;
-					interface->deframe_ref_count = 0;
+						interface->deframe_func = NULL;
+						interface->deframe_ref_count = 0;
 
-					device->index = ++sDeviceIndex;
+						device->index = ++sDeviceIndex;
-					device->module = module;
+						device->module = module;
 
-					sInterfaces.Add(interface);
+						sInterfaces.Add(interface);
-					return interface;
+						return interface;
-				} else
+					}
-					module->uninit_device(device);
+					delete interface;
+				}
+				module->uninit_device(device);
 			}
-
 			put_module(moduleName);
 		}
 	}
@@ -415,6 +423,19 @@ get_device_interface(const char *name)
 void
 down_device_interface(net_device_interface *interface)
 {
+	// RX lock must be held when calling down_device_interface.
+	// Known callers are `interface_protocol_down' which gets
+	// here via one of the following paths:
+	//
+	// - domain_interface_control() [rx lock held, domain lock held]
+	//    interface_set_down()
+	//     interface_protocol_down()
+	//
+	// - domain_interface_control() [rx lock held, domain lock held]
+	//    remove_interface_from_domain()
+	//     delete_interface()
+	//      interface_set_down()
+
 	net_device *device = interface->device;
 
 	dprintf("down_device_interface(%s)\n", interface->name);
@@ -422,13 +443,17 @@ down_device_interface(net_device_interface *interface)
 	device->flags &= ~IFF_UP;
 	interface->module->down(device);
 
-	// TODO: there is a race condition between the previous
+	thread_id reader_thread = interface->reader_thread;
-	//       ->down and device->module->receive_data which
+
-	//       locks us here waiting for the reader_thread
+	// one of the callers must hold a reference to the net_device_interface
+	// usually it is one of the net_interfaces.
+	benaphore_unlock(&interface->rx_lock);
 
 	// make sure the reader thread is gone before shutting down the interface
 	status_t status;
-	wait_for_thread(interface->reader_thread, &status);
+	wait_for_thread(reader_thread, &status);
+
+	benaphore_lock(&interface->rx_lock);
 }
 
 
@@ -449,6 +474,8 @@ unregister_device_deframer(net_device *device)
 	if (interface == NULL)
 		return ENODEV;
 
+	BenaphoreLocker _(interface->rx_lock);
+
 	if (--interface->deframe_ref_count == 0)
 		interface->deframe_func = NULL;
 
@@ -476,6 +503,8 @@ register_device_deframer(net_device *device, net_deframe_func deframeFunc)
 	if (interface == NULL)
 		return ENODEV;
 
+	BenaphoreLocker _(interface->rx_lock);
+
 	if (interface->deframe_func != NULL && interface->deframe_func != deframeFunc)
 		return B_ERROR;
 
@@ -508,6 +537,8 @@ register_device_handler(struct net_device *device, int32 type,
 	if (interface == NULL)
 		return ENODEV;
 
+	BenaphoreLocker _(interface->rx_lock);
+
 	// see if such a handler already for this device
 
 	DeviceHandlerList::Iterator iterator = interface->receive_funcs.GetIterator();
@@ -542,6 +573,8 @@ unregister_device_handler(struct net_device *device, int32 type)
 	if (interface == NULL)
 		return ENODEV;
 
+	BenaphoreLocker _(interface->rx_lock);
+
 	// search for the handler
 
 	DeviceHandlerList::Iterator iterator = interface->receive_funcs.GetIterator();
@@ -571,6 +604,8 @@ register_device_monitor(struct net_device *device,
 	if (interface == NULL)
 		return ENODEV;
 
+	BenaphoreLocker _(interface->rx_lock);
+
 	// Add new monitor
 
 	net_device_monitor *monitor = new (std::nothrow) net_device_monitor;
@@ -595,6 +630,8 @@ unregister_device_monitor(struct net_device *device,
 	if (interface == NULL)
 		return ENODEV;
 
+	BenaphoreLocker _(interface->rx_lock);
+
 	// search for the monitor
 
 	DeviceMonitorList::Iterator iterator = interface->monitor_funcs.GetIterator();
@@ -644,6 +681,8 @@ device_removed(net_device *device)
 	// This is very complex, refer to delete_interface() for
 	// further details.
 
+	BenaphoreLocker _(interface->rx_lock);
+
 	// this will possibly call:
 	//  remove_interface_from_domain() [domain gets locked]
 	//   delete_interface()

src/add-ons/kernel/network/stack/interfaces.h

@@ -44,6 +44,8 @@ struct net_device_interface : DoublyLinkedListLinkImpl<net_device_interface> {
 
 	DeviceMonitorList	monitor_funcs;
 	DeviceHandlerList	receive_funcs;
+
+	benaphore			rx_lock;
 };
 
 typedef DoublyLinkedList<net_device_interface> DeviceInterfaceList;
@@ -78,7 +80,8 @@ uint32 count_device_interfaces();
 status_t list_device_interfaces(void *buffer, size_t *_bufferSize);
 void put_device_interface(struct net_device_interface *interface);
 struct net_device_interface *get_device_interface(uint32 index);
-struct net_device_interface *get_device_interface(const char *name);
+struct net_device_interface *get_device_interface(const char *name,
+	bool create = true);
 void down_device_interface(net_device_interface *interface);
 
 // devices

src/add-ons/kernel/network/stack/routes.cpp

@@ -506,9 +506,19 @@ invalidate_routes(net_domain *_domain, net_interface *interface)
 	while (iterator.HasNext()) {
 		net_route *route = iterator.Next();
 
-		// TODO handle refcounting, if the route needs to linger
+		// TODO If we are removing the interface this will bork.
-		//      for some reason we should set interface or
+		//      Consider the following case:
-		//      something of the sorts that invalidates it's reference
+		//      [thread 1] ipv4_send_data()
+		//      [thread 1]  get_route() [domain locked, unlocked] <- route
+		//      [thread 2] ... [domain locked]
+		//      [thread 2]  invalidate_routes()
+		//      [thread 2]   remove_route() <- route
+		//      [thread 1] ... ipv4_send_data() accesses `route'. Bork bork.
+		//
+		//      We could either add per-route locks (expensive) or
+		//      lock the domain throughout the send_data() routine.
+		//      These are the easy solutions, need to think about this. -hugo
+
 		if (route->interface == interface)
 			remove_route(domain, route);
 	}