i2p/haiku-beta6
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

* The net_domain's lock is now a recursive lock.

Browse Source 
* 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
This commit is contained in:
Axel Dörfler
2009-03-03 14:04:57 +00:00
parent f11e13ffa2
commit 26153d0f67
6 changed files with 59 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/add-ons/kernel/network/stack/datalink.cpp
+4 -4
View File
@@ -195,7 +195,7 @@ datalink_control_interface(net_domain_private* domain, int32 option,
if (user_memcpy(&request, value, expected) < B_OK) if (user_memcpy(&request, value, expected) < B_OK)
return B_BAD_ADDRESS; return B_BAD_ADDRESS;
MutexLocker _(domain->lock); RecursiveLocker _(domain->lock);
net_interface* interface = NULL; net_interface* interface = NULL;
if (getByName) 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) if (user_memcpy(&request, value, sizeof(struct ifreq)) < B_OK)
return B_BAD_ADDRESS; return B_BAD_ADDRESS;
MutexLocker _(domain->lock); RecursiveLocker _(domain->lock);
net_interface* interface = find_interface(domain, net_interface* interface = find_interface(domain,
request.ifr_name); request.ifr_name);
@@ -413,7 +413,7 @@ datalink_is_local_address(net_domain* _domain, const struct sockaddr* address,
if (domain == NULL || address == NULL) if (domain == NULL || address == NULL)
return false; return false;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
net_interface* interface = NULL; net_interface* interface = NULL;
net_interface* fallback = NULL; net_interface* fallback = NULL;
@@ -466,7 +466,7 @@ datalink_get_interface_with_address(net_domain* _domain,
if (domain == NULL) if (domain == NULL)
return NULL; return NULL;
MutexLocker _(domain->lock); RecursiveLocker _(domain->lock);
net_interface* interface = NULL; net_interface* interface = NULL;
src/add-ons/kernel/network/stack/domains.cpp
+11 -13
View File
@@ -36,11 +36,10 @@ static mutex sDomainLock;
static list sDomains; 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. You need to hold the sDomainLock when calling this function.
*/ */
static net_domain_private* static net_domain_private*
lookup_domain(int family) lookup_domain(int family)
{ {
net_domain_private* domain = NULL; net_domain_private* domain = NULL;
@@ -60,10 +59,9 @@ lookup_domain(int family)
// #pragma mark - // #pragma mark -
/*! /*! Gets the domain of the specified family.
Gets the domain of the specified family.
*/ */
net_domain* net_domain*
get_domain(int family) get_domain(int family)
{ {
MutexLocker locker(sDomainLock); MutexLocker locker(sDomainLock);
@@ -117,7 +115,7 @@ list_domain_interfaces(void* _buffer, size_t* bufferSize)
if (domain == NULL) if (domain == NULL)
break; break;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
net_interface* interface = NULL; net_interface* interface = NULL;
while (true) { while (true) {
@@ -163,7 +161,7 @@ add_interface_to_domain(net_domain* _domain,
if (deviceInterface == NULL) if (deviceInterface == NULL)
return ENODEV; return ENODEV;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
net_interface_private* interface = NULL; net_interface_private* interface = NULL;
status_t status; 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 // and domain locks are required, we MUST obtain the receive
// lock before the domain lock. // lock before the domain lock.
RecursiveLocker _1(device->receive_lock); RecursiveLocker _1(device->receive_lock);
MutexLocker _2(domain->lock); RecursiveLocker _2(domain->lock);
net_interface* interface = find_interface(domain, name); net_interface* interface = find_interface(domain, name);
if (interface != NULL) { if (interface != NULL) {
@@ -273,7 +271,7 @@ domain_interface_control(net_domain_private* domain, int32 option,
void void
domain_interface_went_down(net_interface* interface) 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", TRACE(("domain_interface_went_down(%i, %s)\n",
interface->domain->family, interface->name)); interface->domain->family, interface->name));
@@ -293,7 +291,7 @@ domain_removed_device_interface(net_device_interface* deviceInterface)
if (domain == NULL) if (domain == NULL)
break; break;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
net_interface_private* interface = find_interface(domain, net_interface_private* interface = find_interface(domain,
deviceInterface->device->name); deviceInterface->device->name);
@@ -322,7 +320,7 @@ register_domain(int family, const char* name,
if (domain == NULL) if (domain == NULL)
return B_NO_MEMORY; return B_NO_MEMORY;
mutex_init(&domain->lock, name); recursive_lock_init(&domain->lock, name);
domain->family = family; domain->family = family;
domain->name = name; domain->name = name;
@@ -359,7 +357,7 @@ unregister_domain(net_domain* _domain)
delete_interface(interface); delete_interface(interface);
} }
mutex_destroy(&domain->lock); recursive_lock_destroy(&domain->lock);
delete domain; delete domain;
return B_OK; return B_OK;
} }
src/add-ons/kernel/network/stack/domains.h
+14 -15
View File
@@ -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. * Distributed under the terms of the MIT License.
* *
* Authors: * Authors:
@@ -22,7 +22,7 @@ struct net_device_interface;
struct net_domain_private : net_domain { struct net_domain_private : net_domain {
struct list_link link; struct list_link link;
mutex lock; recursive_lock lock;
RouteList routes; RouteList routes;
RouteInfoList route_infos; RouteInfoList route_infos;
@@ -33,19 +33,18 @@ status_t init_domains();
status_t uninit_domains(); status_t uninit_domains();
uint32 count_domain_interfaces(); uint32 count_domain_interfaces();
status_t list_domain_interfaces(void *buffer, size_t *_bufferSize); status_t list_domain_interfaces(void* buffer, size_t* _bufferSize);
status_t add_interface_to_domain(net_domain *domain, struct ifreq& request); status_t add_interface_to_domain(net_domain* domain, struct ifreq& request);
status_t remove_interface_from_domain(net_interface *interface); status_t remove_interface_from_domain(net_interface* interface);
void domain_interface_went_down(net_interface *); void domain_interface_went_down(net_interface* interface);
void domain_removed_device_interface(net_device_interface *); void domain_removed_device_interface(net_device_interface* interface);
status_t domain_interface_control(net_domain_private *domain, int32 option, status_t domain_interface_control(net_domain_private* domain, int32 option,
struct ifreq *request); struct ifreq* request);
net_domain *get_domain(int family); net_domain* get_domain(int family);
status_t register_domain(int family, const char *name, status_t register_domain(int family, const char* name,
struct net_protocol_module_info *module, struct net_protocol_module_info* module,
struct net_address_module_info *addressModule, struct net_address_module_info* addressModule, net_domain* *_domain);
net_domain **_domain); status_t unregister_domain(net_domain* domain);
status_t unregister_domain(net_domain *domain);
#endif // DOMAINS_H #endif // DOMAINS_H
src/add-ons/kernel/network/stack/interfaces.cpp
+4 -3
View File
@@ -327,7 +327,7 @@ put_interface(struct net_interface_private* interface)
{ {
// TODO: reference counting // TODO: reference counting
// TODO: better locking scheme // 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) get_interface(net_domain* _domain, const char* name)
{ {
net_domain_private* domain = (net_domain_private*)_domain; net_domain_private* domain = (net_domain_private*)_domain;
mutex_lock(&domain->lock); recursive_lock_lock(&domain->lock);
net_interface_private* interface = NULL; net_interface_private* interface = NULL;
while (true) { while (true) {
@@ -344,11 +344,12 @@ get_interface(net_domain* _domain, const char* name)
if (interface == NULL) if (interface == NULL)
break; break;
// TODO: We keep the domain locked for now
if (!strcmp(interface->name, name)) if (!strcmp(interface->name, name))
return interface; return interface;
} }
mutex_unlock(&domain->lock); recursive_lock_unlock(&domain->lock);
return NULL; return NULL;
} }
src/add-ons/kernel/network/stack/routes.cpp
+24 -29
View File
@@ -206,13 +206,17 @@ find_route(net_domain* _domain, const sockaddr* address)
static void static void
put_route_internal(struct net_domain_private* domain, net_route* _route) put_route_internal(struct net_domain_private* domain, net_route* _route)
{ {
ASSERT_LOCKED_RECURSIVE(&domain->lock);
net_route_private* route = (net_route_private*)_route; net_route_private* route = (net_route_private*)_route;
if (route == NULL || atomic_add(&route->ref_count, -1) != 1) if (route == NULL || atomic_add(&route->ref_count, -1) != 1)
return; 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; delete route;
} }
@@ -221,6 +225,7 @@ static struct net_route*
get_route_internal(struct net_domain_private* domain, get_route_internal(struct net_domain_private* domain,
const struct sockaddr* address) const struct sockaddr* address)
{ {
ASSERT_LOCKED_RECURSIVE(&domain->lock);
net_route_private* route = NULL; net_route_private* route = NULL;
if (address->sa_family == AF_LINK) { if (address->sa_family == AF_LINK) {
@@ -256,6 +261,7 @@ get_route_internal(struct net_domain_private* domain,
static void static void
update_route_infos(struct net_domain_private* domain) update_route_infos(struct net_domain_private* domain)
{ {
ASSERT_LOCKED_RECURSIVE(&domain->lock);
RouteInfoList::Iterator iterator = domain->route_infos.GetIterator(); RouteInfoList::Iterator iterator = domain->route_infos.GetIterator();
while (iterator.HasNext()) { while (iterator.HasNext()) {
@@ -304,7 +310,7 @@ fill_route_entry(route_entry* target, void* _buffer, size_t bufferSize,
uint32 uint32
route_table_size(net_domain_private* domain) route_table_size(net_domain_private* domain)
{ {
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
uint32 size = 0; uint32 size = 0;
RouteList::Iterator iterator = domain->routes.GetIterator(); RouteList::Iterator iterator = domain->routes.GetIterator();
@@ -331,6 +337,8 @@ route_table_size(net_domain_private* domain)
status_t status_t
list_routes(net_domain_private* domain, void* buffer, size_t size) list_routes(net_domain_private* domain, void* buffer, size_t size)
{ {
RecursiveLocker _(domain->lock);
RouteList::Iterator iterator = domain->routes.GetIterator(); RouteList::Iterator iterator = domain->routes.GetIterator();
size_t spaceLeft = size; 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)) || !domain->address_module->check_mask(newRoute->mask))
return B_BAD_VALUE; return B_BAD_VALUE;
RecursiveLocker _(domain->lock);
net_route_private* route = find_route(domain, newRoute); net_route_private* route = find_route(domain, newRoute);
if (route != NULL) if (route != NULL)
return B_FILE_EXISTS; return B_FILE_EXISTS;
@@ -480,9 +490,6 @@ add_route(struct net_domain* _domain, const struct net_route* newRoute)
route->mtu = 0; route->mtu = 0;
route->ref_count = 1; route->ref_count = 1;
// TODO: for now...
//MutexLocker locker(domain->lock);
// Insert the route sorted by completeness of its mask // Insert the route sorted by completeness of its mask
RouteList::Iterator iterator = domain->routes.GetIterator(); 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(), AddressString(domain, removeRoute->gateway ? removeRoute->gateway : NULL).Data(),
removeRoute->flags)); removeRoute->flags));
// TODO: for now... RecursiveLocker locker(domain->lock);
//MutexLocker locker(domain->lock);
net_route_private* route = find_route(domain, removeRoute); net_route_private* route = find_route(domain, removeRoute);
if (route == NULL) if (route == NULL)
return B_ENTRY_NOT_FOUND; return B_ENTRY_NOT_FOUND;
domain->routes.Remove(route);
put_route_internal(domain, route); put_route_internal(domain, route);
update_route_infos(domain); update_route_infos(domain);
@@ -555,7 +563,7 @@ get_route_information(struct net_domain* _domain, void* value, size_t length)
if (status != B_OK) if (status != B_OK)
return status; return status;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
net_route_private* route = find_route(domain, (sockaddr*)&destination); net_route_private* route = find_route(domain, (sockaddr*)&destination);
if (route == NULL) if (route == NULL)
@@ -582,19 +590,6 @@ invalidate_routes(net_domain* _domain, net_interface* interface)
while (iterator.HasNext()) { while (iterator.HasNext()) {
net_route* route = iterator.Next(); 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) if (route->interface->index == interface->index)
remove_route(domain, route); remove_route(domain, route);
} }
@@ -605,7 +600,7 @@ struct net_route*
get_route(struct net_domain* _domain, const struct sockaddr* address) get_route(struct net_domain* _domain, const struct sockaddr* address)
{ {
struct net_domain_private* domain = (net_domain_private*)_domain; struct net_domain_private* domain = (net_domain_private*)_domain;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
return get_route_internal(domain, address); 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; net_domain_private* domain = (net_domain_private*)_domain;
MutexLocker _(domain->lock); RecursiveLocker _(domain->lock);
net_interface_private* interface = NULL; 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; net_domain_private* domain = (net_domain_private*)_domain;
MutexLocker _(domain->lock); RecursiveLocker _(domain->lock);
net_route* route = get_route_internal(domain, buffer->destination); net_route* route = get_route_internal(domain, buffer->destination);
if (route == NULL) if (route == NULL)
@@ -677,7 +672,7 @@ void
put_route(struct net_domain* _domain, net_route* route) put_route(struct net_domain* _domain, net_route* route)
{ {
struct net_domain_private* domain = (net_domain_private*)_domain; struct net_domain_private* domain = (net_domain_private*)_domain;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
put_route_internal(domain, (net_route*)route); 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) register_route_info(struct net_domain* _domain, struct net_route_info* info)
{ {
struct net_domain_private* domain = (net_domain_private*)_domain; struct net_domain_private* domain = (net_domain_private*)_domain;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
domain->route_infos.Add(info); domain->route_infos.Add(info);
info->route = get_route_internal(domain, &info->address); 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) unregister_route_info(struct net_domain* _domain, struct net_route_info* info)
{ {
struct net_domain_private* domain = (net_domain_private*)_domain; struct net_domain_private* domain = (net_domain_private*)_domain;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
domain->route_infos.Remove(info); domain->route_infos.Remove(info);
if (info->route != NULL) if (info->route != NULL)
@@ -714,7 +709,7 @@ status_t
update_route_info(struct net_domain* _domain, struct net_route_info* info) update_route_info(struct net_domain* _domain, struct net_route_info* info)
{ {
struct net_domain_private* domain = (net_domain_private*)_domain; struct net_domain_private* domain = (net_domain_private*)_domain;
MutexLocker locker(domain->lock); RecursiveLocker locker(domain->lock);
put_route_internal(domain, info->route); put_route_internal(domain, info->route);
info->route = get_route_internal(domain, &info->address); info->route = get_route_internal(domain, &info->address);
src/add-ons/kernel/network/stack/routes.h
+2 -1
View File
@@ -15,7 +15,8 @@
#include <util/DoublyLinkedList.h> #include <util/DoublyLinkedList.h>
struct net_route_private : net_route, public DoublyLinkedListLinkImpl<net_route_private> { struct net_route_private
: net_route, DoublyLinkedListLinkImpl<net_route_private> {
int32 ref_count; int32 ref_count;
net_route_private(); net_route_private();