From f62a92ae05268cc5570e81b1c2ce77763596dd81 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Mon, 22 Apr 2019 18:41:25 -0400 Subject: [PATCH] network/stack: Fix a race condition in device interface teardown. Inside the device_consumer thread, the sequence of operations was this: 1. Call fifo_dequeue_buffer() 2. If it returned B_INTERRUPTED, exit the thread. Otherwise, process the buffer it returned as normal. 3. Loop. Thus, if the FIFO was destroyed not during a call to fifo_dequeue_buffer, the next loop, fifo_dequeue_buffer would be called on a destroyed FIFO. It would then try to lock the mutex in the FIFO which had been destroyed, causing an assertion failure and thus a panic. Now, we check the ref_count on every loop, and set it to 0 before calling uninit_fifo(). Thus, even if we are in the middle of a loop inside the FIFO thread, the loop iteration condition will fail and thus the thread will exit, avoiding the race. Probably this was not an issue before because the timing required to hit this is incredibly unlikely. With the new ipro1000 driver (or kallisti5's WIP TUN/TAP driver), the timing makes this much more likely. Should fix #15024. --- src/add-ons/kernel/network/stack/device_interfaces.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/add-ons/kernel/network/stack/device_interfaces.cpp b/src/add-ons/kernel/network/stack/device_interfaces.cpp index e873d247d9..543eea7126 100644 --- a/src/add-ons/kernel/network/stack/device_interfaces.cpp +++ b/src/add-ons/kernel/network/stack/device_interfaces.cpp @@ -88,7 +88,7 @@ device_consumer_thread(void* _interface) net_device* device = interface->device; net_buffer* buffer; - while (true) { + while (atomic_get(&interface->ref_count) > 0) { ssize_t status = fifo_dequeue_buffer(&interface->receive_queue, 0, B_INFINITE_TIMEOUT, &buffer); if (status != B_OK) { @@ -380,13 +380,16 @@ put_device_interface(struct net_device_interface* interface) if (atomic_add(&interface->ref_count, -1) != 1) return; + // Indicate we are in the process of destroying this interface + // by setting its ref_count to 0. + interface->ref_count = 0; + MutexLocker locker(sLock); sInterfaces.Remove(interface); locker.Unlock(); uninit_fifo(&interface->receive_queue); - status_t status; - wait_for_thread(interface->consumer_thread, &status); + wait_for_thread(interface->consumer_thread, NULL); net_device* device = interface->device; const char* moduleName = device->module->info.name;