From 26153d0f67e36b509dade3b8aed43ba7b753578b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Tue, 3 Mar 2009 14:04:57 +0000 Subject: [PATCH] * The net_domain's lock is now a recursive lock. * Fixed all route locking problems, of which there were numerous ({add|remove}_route(), and list_routes() did not lock at all). Added lock assertions in functions that don't do the locking themselves. * A route will now be removed from the list in remove_route(), not in put_route_internal(). Before, a route could easily be removed twice, causing remove_route() to release references it did not own. This fixes bug #2706. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@29386 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- src/add-ons/kernel/network/stack/datalink.cpp | 8 +-- src/add-ons/kernel/network/stack/domains.cpp | 24 ++++----- src/add-ons/kernel/network/stack/domains.h | 29 +++++----- .../kernel/network/stack/interfaces.cpp | 7 +-- src/add-ons/kernel/network/stack/routes.cpp | 53 +++++++++---------- src/add-ons/kernel/network/stack/routes.h | 3 +- 6 files changed, 59 insertions(+), 65 deletions(-) diff --git a/src/add-ons/kernel/network/stack/datalink.cpp b/src/add-ons/kernel/network/stack/datalink.cpp index 7cabe8d9b5..fc9e20d2e8 100644 --- a/src/add-ons/kernel/network/stack/datalink.cpp +++ b/src/add-ons/kernel/network/stack/datalink.cpp @@ -195,7 +195,7 @@ datalink_control_interface(net_domain_private* domain, int32 option, if (user_memcpy(&request, value, expected) < B_OK) return B_BAD_ADDRESS; - MutexLocker _(domain->lock); + RecursiveLocker _(domain->lock); net_interface* interface = NULL; if (getByName) @@ -320,7 +320,7 @@ datalink_control(net_domain* _domain, int32 option, void* value, if (user_memcpy(&request, value, sizeof(struct ifreq)) < B_OK) return B_BAD_ADDRESS; - MutexLocker _(domain->lock); + RecursiveLocker _(domain->lock); net_interface* interface = find_interface(domain, request.ifr_name); @@ -413,7 +413,7 @@ datalink_is_local_address(net_domain* _domain, const struct sockaddr* address, if (domain == NULL || address == NULL) return false; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); net_interface* interface = NULL; net_interface* fallback = NULL; @@ -466,7 +466,7 @@ datalink_get_interface_with_address(net_domain* _domain, if (domain == NULL) return NULL; - MutexLocker _(domain->lock); + RecursiveLocker _(domain->lock); net_interface* interface = NULL; diff --git a/src/add-ons/kernel/network/stack/domains.cpp b/src/add-ons/kernel/network/stack/domains.cpp index 63b0eb1b28..f73f0aea35 100644 --- a/src/add-ons/kernel/network/stack/domains.cpp +++ b/src/add-ons/kernel/network/stack/domains.cpp @@ -36,11 +36,10 @@ static mutex sDomainLock; static list sDomains; -/*! - Scans the domain list for the specified family. +/*! Scans the domain list for the specified family. You need to hold the sDomainLock when calling this function. */ -static net_domain_private* +static net_domain_private* lookup_domain(int family) { net_domain_private* domain = NULL; @@ -60,10 +59,9 @@ lookup_domain(int family) // #pragma mark - -/*! - Gets the domain of the specified family. +/*! Gets the domain of the specified family. */ -net_domain* +net_domain* get_domain(int family) { MutexLocker locker(sDomainLock); @@ -117,7 +115,7 @@ list_domain_interfaces(void* _buffer, size_t* bufferSize) if (domain == NULL) break; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); net_interface* interface = NULL; while (true) { @@ -163,7 +161,7 @@ add_interface_to_domain(net_domain* _domain, if (deviceInterface == NULL) return ENODEV; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); net_interface_private* interface = NULL; status_t status; @@ -226,7 +224,7 @@ domain_interface_control(net_domain_private* domain, int32 option, // and domain locks are required, we MUST obtain the receive // lock before the domain lock. RecursiveLocker _1(device->receive_lock); - MutexLocker _2(domain->lock); + RecursiveLocker _2(domain->lock); net_interface* interface = find_interface(domain, name); if (interface != NULL) { @@ -273,7 +271,7 @@ domain_interface_control(net_domain_private* domain, int32 option, void domain_interface_went_down(net_interface* interface) { - ASSERT_LOCKED_MUTEX(&((net_domain_private*)interface->domain)->lock); + ASSERT_LOCKED_RECURSIVE(&((net_domain_private*)interface->domain)->lock); TRACE(("domain_interface_went_down(%i, %s)\n", interface->domain->family, interface->name)); @@ -293,7 +291,7 @@ domain_removed_device_interface(net_device_interface* deviceInterface) if (domain == NULL) break; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); net_interface_private* interface = find_interface(domain, deviceInterface->device->name); @@ -322,7 +320,7 @@ register_domain(int family, const char* name, if (domain == NULL) return B_NO_MEMORY; - mutex_init(&domain->lock, name); + recursive_lock_init(&domain->lock, name); domain->family = family; domain->name = name; @@ -359,7 +357,7 @@ unregister_domain(net_domain* _domain) delete_interface(interface); } - mutex_destroy(&domain->lock); + recursive_lock_destroy(&domain->lock); delete domain; return B_OK; } diff --git a/src/add-ons/kernel/network/stack/domains.h b/src/add-ons/kernel/network/stack/domains.h index 7a5b2ceb5f..84778ec04a 100644 --- a/src/add-ons/kernel/network/stack/domains.h +++ b/src/add-ons/kernel/network/stack/domains.h @@ -1,5 +1,5 @@ /* - * Copyright 2006-2007, Haiku, Inc. All Rights Reserved. + * Copyright 2006-2009, Haiku, Inc. All Rights Reserved. * Distributed under the terms of the MIT License. * * Authors: @@ -22,7 +22,7 @@ struct net_device_interface; struct net_domain_private : net_domain { struct list_link link; - mutex lock; + recursive_lock lock; RouteList routes; RouteInfoList route_infos; @@ -33,19 +33,18 @@ status_t init_domains(); status_t uninit_domains(); uint32 count_domain_interfaces(); -status_t list_domain_interfaces(void *buffer, size_t *_bufferSize); -status_t add_interface_to_domain(net_domain *domain, struct ifreq& request); -status_t remove_interface_from_domain(net_interface *interface); -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); +status_t list_domain_interfaces(void* buffer, size_t* _bufferSize); +status_t add_interface_to_domain(net_domain* domain, struct ifreq& request); +status_t remove_interface_from_domain(net_interface* interface); +void domain_interface_went_down(net_interface* interface); +void domain_removed_device_interface(net_device_interface* 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, - struct net_protocol_module_info *module, - struct net_address_module_info *addressModule, - net_domain **_domain); -status_t unregister_domain(net_domain *domain); +net_domain* get_domain(int family); +status_t register_domain(int family, const char* name, + struct net_protocol_module_info* module, + struct net_address_module_info* addressModule, net_domain* *_domain); +status_t unregister_domain(net_domain* domain); #endif // DOMAINS_H diff --git a/src/add-ons/kernel/network/stack/interfaces.cpp b/src/add-ons/kernel/network/stack/interfaces.cpp index 89b76d9c3f..36cab2ece9 100644 --- a/src/add-ons/kernel/network/stack/interfaces.cpp +++ b/src/add-ons/kernel/network/stack/interfaces.cpp @@ -327,7 +327,7 @@ put_interface(struct net_interface_private* interface) { // TODO: reference counting // TODO: better locking scheme - mutex_unlock(&((net_domain_private*)interface->domain)->lock); + recursive_lock_unlock(&((net_domain_private*)interface->domain)->lock); } @@ -335,7 +335,7 @@ struct net_interface_private* get_interface(net_domain* _domain, const char* name) { net_domain_private* domain = (net_domain_private*)_domain; - mutex_lock(&domain->lock); + recursive_lock_lock(&domain->lock); net_interface_private* interface = NULL; while (true) { @@ -344,11 +344,12 @@ get_interface(net_domain* _domain, const char* name) if (interface == NULL) break; + // TODO: We keep the domain locked for now if (!strcmp(interface->name, name)) return interface; } - mutex_unlock(&domain->lock); + recursive_lock_unlock(&domain->lock); return NULL; } diff --git a/src/add-ons/kernel/network/stack/routes.cpp b/src/add-ons/kernel/network/stack/routes.cpp index 34bc959050..ee89608faa 100644 --- a/src/add-ons/kernel/network/stack/routes.cpp +++ b/src/add-ons/kernel/network/stack/routes.cpp @@ -206,13 +206,17 @@ find_route(net_domain* _domain, const sockaddr* address) static void put_route_internal(struct net_domain_private* domain, net_route* _route) { + ASSERT_LOCKED_RECURSIVE(&domain->lock); + net_route_private* route = (net_route_private*)_route; if (route == NULL || atomic_add(&route->ref_count, -1) != 1) return; - // remove route + // delete route - it must already have been removed at this point + + ASSERT(route->GetDoublyLinkedListLink()->next == NULL + && route->GetDoublyLinkedListLink()->previous == NULL); - domain->routes.Remove(route); delete route; } @@ -221,6 +225,7 @@ static struct net_route* get_route_internal(struct net_domain_private* domain, const struct sockaddr* address) { + ASSERT_LOCKED_RECURSIVE(&domain->lock); net_route_private* route = NULL; if (address->sa_family == AF_LINK) { @@ -256,6 +261,7 @@ get_route_internal(struct net_domain_private* domain, static void update_route_infos(struct net_domain_private* domain) { + ASSERT_LOCKED_RECURSIVE(&domain->lock); RouteInfoList::Iterator iterator = domain->route_infos.GetIterator(); while (iterator.HasNext()) { @@ -304,7 +310,7 @@ fill_route_entry(route_entry* target, void* _buffer, size_t bufferSize, uint32 route_table_size(net_domain_private* domain) { - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); uint32 size = 0; RouteList::Iterator iterator = domain->routes.GetIterator(); @@ -331,6 +337,8 @@ route_table_size(net_domain_private* domain) status_t list_routes(net_domain_private* domain, void* buffer, size_t size) { + RecursiveLocker _(domain->lock); + RouteList::Iterator iterator = domain->routes.GetIterator(); size_t spaceLeft = size; @@ -456,6 +464,8 @@ add_route(struct net_domain* _domain, const struct net_route* newRoute) || !domain->address_module->check_mask(newRoute->mask)) return B_BAD_VALUE; + RecursiveLocker _(domain->lock); + net_route_private* route = find_route(domain, newRoute); if (route != NULL) return B_FILE_EXISTS; @@ -480,9 +490,6 @@ add_route(struct net_domain* _domain, const struct net_route* newRoute) route->mtu = 0; route->ref_count = 1; - // TODO: for now... - //MutexLocker locker(domain->lock); - // Insert the route sorted by completeness of its mask RouteList::Iterator iterator = domain->routes.GetIterator(); @@ -524,13 +531,14 @@ remove_route(struct net_domain* _domain, const struct net_route* removeRoute) AddressString(domain, removeRoute->gateway ? removeRoute->gateway : NULL).Data(), removeRoute->flags)); - // TODO: for now... - //MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); net_route_private* route = find_route(domain, removeRoute); if (route == NULL) return B_ENTRY_NOT_FOUND; + domain->routes.Remove(route); + put_route_internal(domain, route); update_route_infos(domain); @@ -555,7 +563,7 @@ get_route_information(struct net_domain* _domain, void* value, size_t length) if (status != B_OK) return status; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); net_route_private* route = find_route(domain, (sockaddr*)&destination); if (route == NULL) @@ -582,19 +590,6 @@ invalidate_routes(net_domain* _domain, net_interface* interface) while (iterator.HasNext()) { net_route* route = iterator.Next(); - // 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->index == interface->index) remove_route(domain, route); } @@ -605,7 +600,7 @@ struct net_route* get_route(struct net_domain* _domain, const struct sockaddr* address) { struct net_domain_private* domain = (net_domain_private*)_domain; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); return get_route_internal(domain, address); } @@ -616,7 +611,7 @@ get_device_route(struct net_domain* _domain, uint32 index, net_route** _route) { net_domain_private* domain = (net_domain_private*)_domain; - MutexLocker _(domain->lock); + RecursiveLocker _(domain->lock); net_interface_private* interface = NULL; @@ -642,7 +637,7 @@ get_buffer_route(net_domain* _domain, net_buffer* buffer, net_route** _route) { net_domain_private* domain = (net_domain_private*)_domain; - MutexLocker _(domain->lock); + RecursiveLocker _(domain->lock); net_route* route = get_route_internal(domain, buffer->destination); if (route == NULL) @@ -677,7 +672,7 @@ void put_route(struct net_domain* _domain, net_route* route) { struct net_domain_private* domain = (net_domain_private*)_domain; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); put_route_internal(domain, (net_route*)route); } @@ -687,7 +682,7 @@ status_t register_route_info(struct net_domain* _domain, struct net_route_info* info) { struct net_domain_private* domain = (net_domain_private*)_domain; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); domain->route_infos.Add(info); info->route = get_route_internal(domain, &info->address); @@ -700,7 +695,7 @@ status_t unregister_route_info(struct net_domain* _domain, struct net_route_info* info) { struct net_domain_private* domain = (net_domain_private*)_domain; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); domain->route_infos.Remove(info); if (info->route != NULL) @@ -714,7 +709,7 @@ status_t update_route_info(struct net_domain* _domain, struct net_route_info* info) { struct net_domain_private* domain = (net_domain_private*)_domain; - MutexLocker locker(domain->lock); + RecursiveLocker locker(domain->lock); put_route_internal(domain, info->route); info->route = get_route_internal(domain, &info->address); diff --git a/src/add-ons/kernel/network/stack/routes.h b/src/add-ons/kernel/network/stack/routes.h index c723af9932..835a809e6f 100644 --- a/src/add-ons/kernel/network/stack/routes.h +++ b/src/add-ons/kernel/network/stack/routes.h @@ -15,7 +15,8 @@ #include -struct net_route_private : net_route, public DoublyLinkedListLinkImpl { +struct net_route_private + : net_route, DoublyLinkedListLinkImpl { int32 ref_count; net_route_private();