* Added a dedicated lock for the device monitors. This fixes a locking issue in

interface_protocol_send_data() which accessed the monitors unlocked.
* Changed SIOCCPACKETCAP to check if the device name matches the one used with
  SIOCSPACKETCAP.


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

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

@@ -642,18 +642,8 @@ interface_protocol_send_data(net_datalink_protocol* _protocol,
 	interface_protocol* protocol = (interface_protocol*)_protocol;
 	Interface* interface = (Interface*)protocol->interface;
 
-	// TODO: Need to think about this locking. We can't obtain the
+	if (atomic_get(&interface->DeviceInterface()->monitor_count) > 0)
-	//       RX Lock here (nor would it make sense) as the ARP
+		device_interface_monitor_receive(interface->DeviceInterface(), buffer);
-	//       module calls send_data() with it's lock held (similiar
-	//       to the domain lock, which would violate the locking
-	//       protocol).
-
-	DeviceMonitorList::Iterator iterator =
-		interface->DeviceInterface()->monitor_funcs.GetIterator();
-	while (iterator.HasNext()) {
-		net_device_monitor* monitor = iterator.Next();
-		monitor->receive(monitor, buffer);
-	}
 
 	return protocol->device_module->send_data(protocol->device, buffer);
 }

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

@@ -63,11 +63,8 @@ device_reader_thread(void* _interface)
 
 		if (status == B_OK) {
 			// feed device monitors
-			DeviceMonitorList::Iterator iterator =
+			if (atomic_get(&interface->monitor_count) > 0)
-				interface->monitor_funcs.GetIterator();
+				device_interface_monitor_receive(interface, buffer);
-			while (net_device_monitor* monitor = iterator.Next()) {
-				monitor->receive(monitor, buffer);
-			}
 
 			buffer->interface_address = NULL;
 			buffer->type = interface->deframe_func(interface->device, buffer);
@@ -165,7 +162,8 @@ allocate_device_interface(net_device* device, net_device_module_info* module)
 	if (interface == NULL)
 		return NULL;
 
-	recursive_lock_init(&interface->receive_lock, "interface receive lock");
+	recursive_lock_init(&interface->receive_lock, "device interface receive");
+	mutex_init(&interface->monitor_lock, "device interface monitors");
 
 	char name[128];
 	snprintf(name, sizeof(name), "%s receive queue", device->name);
@@ -199,6 +197,7 @@ error2:
 	uninit_fifo(&interface->receive_queue);
 error1:
 	recursive_lock_destroy(&interface->receive_lock);
+	mutex_destroy(&interface->monitor_lock);
 	delete interface;
 
 	return NULL;
@@ -208,7 +207,7 @@ error1:
 static void
 notify_device_monitors(net_device_interface* interface, int32 event)
 {
-	RecursiveLocker _(interface->receive_lock);
+	MutexLocker locker(interface->monitor_lock);
 
 	DeviceMonitorList::Iterator iterator
 		= interface->monitor_funcs.GetIterator();
@@ -239,17 +238,19 @@ dump_device_interface(int argc, char** argv)
 	kprintf("ref_count:         %" B_PRId32 "\n", interface->ref_count);
 	kprintf("deframe_func:      %p\n", interface->deframe_func);
 	kprintf("deframe_ref_count: %" B_PRId32 "\n", interface->ref_count);
-	kprintf("monitor_funcs:\n");
-	kprintf("receive_funcs:\n");
 	kprintf("consumer_thread:   %ld\n", interface->consumer_thread);
-	kprintf("receive_lock:      %p\n", &interface->receive_lock);
-	kprintf("receive_queue:     %p\n", &interface->receive_queue);
 
+	kprintf("monitor_count:     %" B_PRId32 "\n", interface->monitor_count);
+	kprintf("monitor_lock:      %p\n", &interface->monitor_lock);
+	kprintf("monitor_funcs:\n");
 	DeviceMonitorList::Iterator monitorIterator
 		= interface->monitor_funcs.GetIterator();
 	while (monitorIterator.HasNext())
 		kprintf("  %p\n", monitorIterator.Next());
 
+	kprintf("receive_lock:      %p\n", &interface->receive_lock);
+	kprintf("receive_queue:     %p\n", &interface->receive_queue);
+	kprintf("receive_funcs:\n");
 	DeviceHandlerList::Iterator handlerIterator
 		= interface->receive_funcs.GetIterator();
 	while (handlerIterator.HasNext())
@@ -374,6 +375,7 @@ put_device_interface(struct net_device_interface* interface)
 	device->module->uninit_device(device);
 	put_module(moduleName);
 
+	mutex_destroy(&interface->monitor_lock);
 	recursive_lock_destroy(&interface->receive_lock);
 	delete interface;
 }
@@ -449,6 +451,25 @@ get_device_interface(const char* name, bool create)
 }
 
 
+/*!	Feeds the device monitors of the \a interface with the specified \a buffer.
+	You might want to check interface::monitor_count before calling this
+	function for optimization.
+*/
+void
+device_interface_monitor_receive(net_device_interface* interface,
+	net_buffer* buffer)
+{
+	MutexLocker locker(interface->monitor_lock);
+
+	DeviceMonitorList::Iterator iterator
+		= interface->monitor_funcs.GetIterator();
+	while (iterator.HasNext()) {
+		net_device_monitor* monitor = iterator.Next();
+		monitor->receive(monitor, buffer);
+	}
+}
+
+
 status_t
 up_device_interface(net_device_interface* interface)
 {
@@ -666,8 +687,10 @@ register_device_monitor(net_device* device, net_device_monitor* monitor)
 	if (interface == NULL)
 		return B_DEVICE_NOT_FOUND;
 
-	RecursiveLocker _(interface->receive_lock);
+	MutexLocker monitorLocker(interface->monitor_lock);
 	interface->monitor_funcs.Add(monitor);
+	atomic_add(&interface->monitor_count, 1);
+
 	return B_OK;
 }
 
@@ -683,14 +706,16 @@ unregister_device_monitor(net_device* device, net_device_monitor* monitor)
 	if (interface == NULL)
 		return B_DEVICE_NOT_FOUND;
 
-	RecursiveLocker _(interface->receive_lock);
+	MutexLocker monitorLocker(interface->monitor_lock);
 
 	// search for the monitor
 
-	DeviceMonitorList::Iterator iterator = interface->monitor_funcs.GetIterator();
+	DeviceMonitorList::Iterator iterator
+		= interface->monitor_funcs.GetIterator();
 	while (iterator.HasNext()) {
 		if (iterator.Next() == monitor) {
 			iterator.Remove();
+			atomic_add(&interface->monitor_count, -1);
 			return B_OK;
 		}
 	}
@@ -736,6 +761,7 @@ device_removed(net_device* device)
 
 	// By now all of the monitors must have removed themselves. If they
 	// didn't, they'll probably wait forever to be callback'ed again.
+	mutex_lock(&interface->monitor_lock);
 	interface->monitor_funcs.RemoveAll();
 
 	// All of the readers should be gone as well since we are out of

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

@@ -36,9 +36,11 @@ struct net_device_interface : DoublyLinkedListLinkImpl<net_device_interface> {
 	net_deframe_func	deframe_func;
 	int32				deframe_ref_count;
 
+	int32				monitor_count;
+	mutex				monitor_lock;
 	DeviceMonitorList	monitor_funcs;
-	DeviceHandlerList	receive_funcs;
 
+	DeviceHandlerList	receive_funcs;
 	recursive_lock		receive_lock;
 
 	thread_id			consumer_thread;
@@ -58,12 +60,15 @@ 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,
 	bool create = true);
+void device_interface_monitor_receive(net_device_interface* interface,
+	net_buffer* buffer);
 status_t up_device_interface(net_device_interface* interface);
 void down_device_interface(net_device_interface* interface);
 
 // devices
 status_t unregister_device_deframer(net_device* device);
-status_t register_device_deframer(net_device* device, net_deframe_func deframeFunc);
+status_t register_device_deframer(net_device* device,
+	net_deframe_func deframeFunc);
 status_t register_domain_device_handler(struct net_device* device, int32 type,
 	struct net_domain* domain);
 status_t register_device_handler(struct net_device* device, int32 type,

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

@@ -46,7 +46,7 @@ public:
 	virtual						~LinkProtocol();
 
 			status_t			StartMonitoring(const char* deviceName);
-			status_t			StopMonitoring();
+			status_t			StopMonitoring(const char* deviceName);
 
 protected:
 			status_t			SocketStatus(bool peek) const;
@@ -92,7 +92,7 @@ LinkProtocol::StartMonitoring(const char* deviceName)
 {
 	MutexLocker _(fLock);
 
-	if (fMonitoredDevice)
+	if (fMonitoredDevice != NULL)
 		return B_BUSY;
 
 	net_device_interface* interface = get_device_interface(deviceName);
@@ -111,11 +111,14 @@ LinkProtocol::StartMonitoring(const char* deviceName)
 
 
 status_t
-LinkProtocol::StopMonitoring()
+LinkProtocol::StopMonitoring(const char* deviceName)
 {
 	MutexLocker _(fLock);
 
-	// TODO compare our device with the supplied device name?
+	if (fMonitoredDevice == NULL
+		|| strcmp(fMonitoredDevice->device->name, deviceName) != 0)
+		return B_BAD_VALUE;
+
 	return _Unregister();
 }
 
@@ -351,14 +354,20 @@ link_control(net_protocol* _protocol, int level, int option, void* value,
 				return B_NOT_ALLOWED;
 
 			struct ifreq request;
-			if (user_memcpy(&request, value, IF_NAMESIZE) < B_OK)
+			if (user_memcpy(&request, value, IF_NAMESIZE) != B_OK)
 				return B_BAD_ADDRESS;
 
 			return protocol->StartMonitoring(request.ifr_name);
 		}
 
 		case SIOCCPACKETCAP:
-			return protocol->StopMonitoring();
+		{
+			struct ifreq request;
+			if (user_memcpy(&request, value, IF_NAMESIZE) != B_OK)
+				return B_BAD_ADDRESS;
+
+			return protocol->StopMonitoring(request.ifr_name);
+		}
 	}
 
 	return gNetDatalinkModule.control(sDomain, option, value, _length);