threading: don't pass locked flow between threads

Previously the flow manager would share evicted flows with the workers
while keeping the flows mutex locked. This reduced the number of unlock/
lock cycles while there was guaranteed to be no contention.

This turns out to be undefined behavior. A lock is supposed to be locked
and unlocked from the same thread. It appears that FreeBSD is stricter on
this than Linux.

This patch addresses the issue by unlocking before handing a flow off
to another thread, and locking again from the new thread.

Issue was reported and largely analyzed by Bill Meeks.

Bug: #4478

diff --git a/src/flow-hash.c b/src/flow-hash.c
index 10b59ae2d4124d377769482b62b9468a534f2041..70416aa53892c03cf8c9ce3a3bafcd107947a505 100644 (file)
--- a/src/flow-hash.c
+++ b/src/flow-hash.c
@@ -692,6 +692,7 @@ static inline void MoveToWorkQueue(ThreadVars *tv, FlowLookupStruct *fls,
         f->fb = NULL;
         f->next = NULL;
         FlowQueuePrivateAppendFlow(&fls->work_queue, f);
+        FLOWLOCK_UNLOCK(f);
     } else {
         /* implied: TCP but our thread does not own it. So set it
          * aside for the Flow Manager to pick it up. */

diff --git a/src/flow-manager.c b/src/flow-manager.c
index b2a869ccd576b7058fef3ec67841ae2631cd799b..1d4306a90e10c0e529e744a714b29ff979cc84b9 100644 (file)
--- a/src/flow-manager.c
+++ b/src/flow-manager.c
@@ -333,9 +333,9 @@ static uint32_t ProcessAsideQueue(FlowManagerTimeoutThread *td, FlowTimeoutCount
                 FlowForceReassemblyNeedReassembly(f) == 1)
         {
             FlowForceReassemblyForFlow(f);
+            FLOWLOCK_UNLOCK(f);
             /* flow ownership is passed to the worker thread */
 
-            /* flow remains locked */
             counters->flows_aside_needs_work++;
             continue;
         }

diff --git a/src/flow-timeout.c b/src/flow-timeout.c
index 972b35076bdf27776f07abac1328d0bbf8afc792..d6cca4900873aaaffe9e183738e504426f5083f9 100644 (file)
--- a/src/flow-timeout.c
+++ b/src/flow-timeout.c
@@ -401,6 +401,7 @@ static inline void FlowForceReassemblyForHash(void)
                 RemoveFromHash(f, prev_f);
                 f->flow_end_flags |= FLOW_END_FLAG_SHUTDOWN;
                 FlowForceReassemblyForFlow(f);
+                FLOWLOCK_UNLOCK(f);
                 f = next_f;
                 continue;
             }

diff --git a/src/flow-worker.c b/src/flow-worker.c
index 69dbb6ac575f2cf865cbfdca0cdedf5112f275fc..dccf3581dd5bc0803c25541b8b7160efe66945cf 100644 (file)
--- a/src/flow-worker.c
+++ b/src/flow-worker.c
@@ -168,6 +168,7 @@ static void CheckWorkQueue(ThreadVars *tv, FlowWorkerThreadData *fw,
 {
     Flow *f;
     while ((f = FlowQueuePrivateGetFromTop(fq)) != NULL) {
+        FLOWLOCK_WRLOCK(f);
         f->flow_end_flags |= FLOW_END_FLAG_TIMEOUT; //TODO emerg
 
         const FlowStateType state = f->flow_state;