From 801591dbdea634316f6ff0b0fd0b18f5eaafee6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Tue, 29 Apr 2008 22:03:03 +0000 Subject: [PATCH] * Added an atomic_pointer_set() template function to util/atomic.h. * Made the pointers const. * Changed how the ARP module maintains its arp_entry::request_buffer: it now uses the atomic_pointer*() functions to make sure there is no race condition, and it's deleted only once. * Getting an ARP entry would return uninitialized data, if the entry hadn't been resolved yet. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@25263 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- headers/private/kernel/util/atomic.h | 15 ++- .../network/datalink_protocols/arp/arp.cpp | 99 +++++++++++-------- 2 files changed, 70 insertions(+), 44 deletions(-) diff --git a/headers/private/kernel/util/atomic.h b/headers/private/kernel/util/atomic.h index f769acdc77..18f82f7b4a 100644 --- a/headers/private/kernel/util/atomic.h +++ b/headers/private/kernel/util/atomic.h @@ -14,8 +14,8 @@ #ifdef __cplusplus template PointerType* -atomic_pointer_test_and_set(PointerType** _pointer, PointerType* set, - PointerType* test) +atomic_pointer_test_and_set(PointerType** _pointer, const PointerType* set, + const PointerType* test) { #if LONG_MAX == INT_MAX return (PointerType*)atomic_test_and_set((vint32*)_pointer, (int32)set, @@ -26,6 +26,17 @@ atomic_pointer_test_and_set(PointerType** _pointer, PointerType* set, #endif } + +template PointerType* +atomic_pointer_set(PointerType** _pointer, const PointerType* set) +{ +#if LONG_MAX == INT_MAX + return (PointerType*)atomic_set((vint32*)_pointer, (int32)set); +#else + return (PointerType*)atomic_set64((vint64*)_pointer, (int64)set); +#endif +} + #endif // __cplusplus #endif /* _KERNEL_UTIL_ATOMIC_H */ diff --git a/src/add-ons/kernel/network/datalink_protocols/arp/arp.cpp b/src/add-ons/kernel/network/datalink_protocols/arp/arp.cpp index a0dda26d72..add6f199df 100644 --- a/src/add-ons/kernel/network/datalink_protocols/arp/arp.cpp +++ b/src/add-ons/kernel/network/datalink_protocols/arp/arp.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2006-2007, Haiku, Inc. All Rights Reserved. + * Copyright 2006-2008, Haiku, Inc. All Rights Reserved. * Distributed under the terms of the MIT License. * * Authors: @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -68,7 +69,6 @@ struct arp_entry { sockaddr_dl hardware_address; uint32 flags; net_buffer *request_buffer; - int32 request_buffer_ref_count; net_timer timer; uint32 timer_state; bigtime_t timestamp; @@ -110,6 +110,8 @@ struct arp_protocol : net_datalink_protocol { }; +static const net_buffer* kDeletedBuffer = (net_buffer*)~0; + static void arp_timer(struct net_timer *timer, void *data); net_buffer_module_info *gBufferModule; @@ -120,27 +122,43 @@ static bool sIgnoreReplies; static net_buffer* -get_request_buffer_reference(arp_entry* entry) +get_request_buffer(arp_entry* entry) { - if (atomic_test_and_set(&entry->request_buffer_ref_count, 2, 1) == 1) - return entry->request_buffer; + net_buffer* buffer = entry->request_buffer; + if (buffer == NULL || buffer == kDeletedBuffer) + return NULL; - return NULL; + buffer = atomic_pointer_test_and_set(&entry->request_buffer, + (net_buffer*)NULL, buffer); + if (buffer == kDeletedBuffer) + return NULL; + + return buffer; } static void -put_request_buffer_reference(arp_entry* entry) +put_request_buffer(arp_entry* entry, net_buffer* buffer) { - if (atomic_add(&entry->request_buffer_ref_count, -1) == 0) { - if (entry->request_buffer != NULL) { - gBufferModule->free(entry->request_buffer); - entry->request_buffer = NULL; - } + net_buffer* requestBuffer = atomic_pointer_test_and_set( + &entry->request_buffer, buffer, (net_buffer*)NULL); + if (requestBuffer != NULL) { + // someone else took over ownership of the request buffer + gBufferModule->free(buffer); } } +static void +delete_request_buffer(arp_entry* entry) +{ + net_buffer* buffer = atomic_pointer_set(&entry->request_buffer, + kDeletedBuffer); + if (buffer != NULL && buffer != kDeletedBuffer) + gBufferModule->free(buffer); +} + + /*static*/ int arp_entry::Compare(void *_entry, const void *_key) { @@ -164,7 +182,7 @@ arp_entry::Hash(void *_entry, const void *_key, uint32 range) #define HASH(o) ((((o) >> 24) ^ ((o) >> 16) ^ ((o) >> 8) ^ (o)) % range) #if 0 - in_addr_t a = entry ? entry->protocol_address : *key; + in_addr_t a = entry ? entry->protocol_address : *key; dprintf("%ld.%ld.%ld.%ld: Hash: %lu\n", a >> 24, (a >> 16) & 0xff, (a >> 8) & 0xff, a & 0xff, HASH(a)); #endif @@ -197,7 +215,6 @@ arp_entry::Add(in_addr_t protocolAddress, sockaddr_dl *hardwareAddress, entry->timestamp = system_time(); entry->protocol = NULL; entry->request_buffer = NULL; - entry->request_buffer_ref_count = 0; entry->timer_state = ARP_NO_STATE; sStackModule->init_timer(&entry->timer, arp_timer, entry); @@ -319,13 +336,13 @@ arp_update_entry(in_addr_t protocolAddress, sockaddr_dl *hardwareAddress, { arp_entry *entry = arp_entry::Lookup(protocolAddress); if (entry != NULL) { - // We disallow updating of entries that had been resolved before, + // We disallow updating of entries that had been resolved before, // but to a different address (only for those that belong to a // specific address - redefining INADDR_ANY is always allowed). // Right now, you have to manually purge the ARP entries (or wait some // time) to let us switch to the new address. if (protocolAddress != INADDR_ANY - && entry->hardware_address.sdl_alen != 0 + && entry->hardware_address.sdl_alen != 0 && memcmp(LLADDR(&entry->hardware_address), LLADDR(hardwareAddress), ETHER_ADDRESS_LENGTH)) { dprintf("ARP host %08x updated with different hardware address " @@ -344,7 +361,7 @@ arp_update_entry(in_addr_t protocolAddress, sockaddr_dl *hardwareAddress, return B_NO_MEMORY; } - put_request_buffer_reference(entry); + delete_request_buffer(entry); if ((entry->flags & ARP_FLAG_PERMANENT) == 0) { // (re)start the stale timer @@ -559,30 +576,22 @@ arp_timer(struct net_timer *timer, void *data) TRACE((" send request for ARP entry %p!\n", entry)); - net_buffer *request = get_request_buffer_reference(entry); + net_buffer *request = get_request_buffer(entry); if (request == NULL) break; - // TODO: The reference counting does still not solve request_buffer - // access problem completely: A duplicate reply between this point - // and the "entry->request_buffer = NULL" will release two - // references in arp_update_entry() and thus delete the buffer. if (entry->timer_state < ARP_STATE_LAST_REQUEST) { // we'll still need our buffer, so in order to prevent it being // freed by a successful send, we need to clone it - request = gBufferModule->clone(request, true); - if (request == NULL) { + net_buffer* clone = gBufferModule->clone(request, true); + if (clone == NULL) { // cloning failed - that means we won't be able to send as // many requests as originally planned - request = entry->request_buffer; entry->timer_state = ARP_STATE_LAST_REQUEST; - } else - put_request_buffer_reference(entry); - } - - if (entry->timer_state == ARP_STATE_LAST_REQUEST) { - // the request buffer will be deleted, detach it - entry->request_buffer = NULL; + } else { + put_request_buffer(entry, request); + request = clone; + } } // we're trying to resolve the address, so keep sending requests @@ -599,8 +608,7 @@ arp_timer(struct net_timer *timer, void *data) } -/*! - Address resolver function: prepares and sends the ARP request necessary +/*! Address resolver function: prepares and triggers the ARP request necessary to retrieve the hardware address for \a address. You need to have the sCacheLock held when calling this function - but note that the lock will be interrupted here if everything goes well. @@ -621,7 +629,6 @@ arp_start_resolve(net_datalink_protocol *protocol, in_addr_t address, // TODO: do something with the entry return B_NO_MEMORY; } - entry->request_buffer_ref_count = 1; NetBufferPrepend bufferHeader(entry->request_buffer); status_t status = bufferHeader.Status(); @@ -642,9 +649,10 @@ arp_start_resolve(net_datalink_protocol *protocol, in_addr_t address, header.opcode = htons(ARP_OPCODE_REQUEST); memcpy(header.hardware_sender, device->address.data, ETHER_ADDRESS_LENGTH); - if (protocol->interface->address != NULL) - header.protocol_sender = ((sockaddr_in *)protocol->interface->address)->sin_addr.s_addr; - else + if (protocol->interface->address != NULL) { + header.protocol_sender + = ((sockaddr_in *)protocol->interface->address)->sin_addr.s_addr; + } else header.protocol_sender = 0; // TODO: test if this actually works - maybe we should use INADDR_BROADCAST instead memset(header.hardware_target, 0, ETHER_ADDRESS_LENGTH); @@ -714,8 +722,12 @@ arp_control(const char *subsystem, uint32 function, void *buffer, if (entry == NULL || !(entry->flags & ARP_FLAG_VALID)) return B_ENTRY_NOT_FOUND; - memcpy(control.ethernet_address, entry->hardware_address.sdl_data, - ETHER_ADDRESS_LENGTH); + if (entry->hardware_address.sdl_alen == ETHER_ADDRESS_LENGTH) { + memcpy(control.ethernet_address, + entry->hardware_address.sdl_data, ETHER_ADDRESS_LENGTH); + } else + memset(control.ethernet_address, 0, ETHER_ADDRESS_LENGTH); + control.flags = entry->flags; return user_memcpy(buffer, &control, sizeof(struct arp_control)); } @@ -738,8 +750,11 @@ arp_control(const char *subsystem, uint32 function, void *buffer, control.cookie++; control.address = entry->protocol_address; - memcpy(control.ethernet_address, entry->hardware_address.sdl_data, - ETHER_ADDRESS_LENGTH); + if (entry->hardware_address.sdl_alen == ETHER_ADDRESS_LENGTH) { + memcpy(control.ethernet_address, + entry->hardware_address.sdl_data, ETHER_ADDRESS_LENGTH); + } else + memset(control.ethernet_address, 0, ETHER_ADDRESS_LENGTH); control.flags = entry->flags; return user_memcpy(buffer, &control, sizeof(struct arp_control));