diff --git a/src/add-ons/kernel/network/stack/datalink.cpp b/src/add-ons/kernel/network/stack/datalink.cpp index b9e3c364c3..851967b35e 100644 --- a/src/add-ons/kernel/network/stack/datalink.cpp +++ b/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: - { - // get index of interface - 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)); - } + return datalink_control_interface(domain, option, value, _length, + IF_NAMESIZE, true); 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); - 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)); + return domain_interface_control(domain, option, &request); } 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) { - // filter out bringing the interface up or down - 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 (interface == NULL) + return B_BAD_VALUE; - if (status == B_OK) - interface->flags |= request.ifr_flags & ~(IFF_UP | IFF_LINK); - } else { - // 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; + // pass the request into the datalink protocol stack + return interface->first_info->control( + interface->first_protocol, option, value, *_length); } } return B_BAD_VALUE; diff --git a/src/add-ons/kernel/network/stack/domains.cpp b/src/add-ons/kernel/network/stack/domains.cpp index 4c16f96a7d..946e4772d7 100644 --- a/src/add-ons/kernel/network/stack/domains.cpp +++ b/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 @@ -21,6 +22,7 @@ #include #include #include +#include #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) { diff --git a/src/add-ons/kernel/network/stack/domains.h b/src/add-ons/kernel/network/stack/domains.h index 6cb837bda8..83252f5d0f 100644 --- a/src/add-ons/kernel/network/stack/domains.h +++ b/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, diff --git a/src/add-ons/kernel/network/stack/interfaces.cpp b/src/add-ons/kernel/network/stack/interfaces.cpp index da2770402b..37e3f62459 100644 --- a/src/add-ons/kernel/network/stack/interfaces.cpp +++ b/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; - interface->module = module; - interface->device = device; - interface->up_count = 0; - interface->ref_count = 1; - interface->deframe_func = NULL; - interface->deframe_ref_count = 0; + if (benaphore_init(&interface->rx_lock, "rx lock") >= B_OK) { + interface->name = device->name; + interface->module = module; + interface->device = device; + interface->up_count = 0; + interface->ref_count = 1; + interface->deframe_func = NULL; + interface->deframe_ref_count = 0; - device->index = ++sDeviceIndex; - device->module = module; + device->index = ++sDeviceIndex; + device->module = module; - sInterfaces.Add(interface); - return interface; - } else - module->uninit_device(device); + sInterfaces.Add(interface); + return interface; + } + 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 - // ->down and device->module->receive_data which - // locks us here waiting for the reader_thread + thread_id reader_thread = interface->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() diff --git a/src/add-ons/kernel/network/stack/interfaces.h b/src/add-ons/kernel/network/stack/interfaces.h index 19d73c7be9..9c819372dc 100644 --- a/src/add-ons/kernel/network/stack/interfaces.h +++ b/src/add-ons/kernel/network/stack/interfaces.h @@ -44,6 +44,8 @@ struct net_device_interface : DoublyLinkedListLinkImpl { DeviceMonitorList monitor_funcs; DeviceHandlerList receive_funcs; + + benaphore rx_lock; }; typedef DoublyLinkedList 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 diff --git a/src/add-ons/kernel/network/stack/routes.cpp b/src/add-ons/kernel/network/stack/routes.cpp index e45e226ce8..79ae9b66c2 100644 --- a/src/add-ons/kernel/network/stack/routes.cpp +++ b/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 - // for some reason we should set interface or - // something of the sorts that invalidates it's reference + // TODO If we are removing the interface this will bork. + // Consider the following case: + // [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); }