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
(cherry picked from commit 9551cd05357925e8bec8e0030d5f98fd07f17839)

diff --git a/src/flow-hash.c b/src/flow-hash.c
index 16e2939df83d3249157fe748fe38139242ae77f9..c0f2408d5dc6f448137249dbeea35680fc337eac 100644 (file)
--- a/src/flow-hash.c
+++ b/src/flow-hash.c
@@ -671,6 +671,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 719d0fde26f071e0ccad7590a3fbe07c0c3c26ed..76d6ac852a25f09096cbd1c1f016e7ec7a557eb8 100644 (file)
--- a/src/flow-manager.c
+++ b/src/flow-manager.c
@@ -332,9 +332,9 @@ static uint32_t ProcessAsideQueue(FlowManagerTimeoutThread *td, FlowTimeoutCount
                 FlowForceReassemblyNeedReassembly(f) == 1) {
             /* Send the flow to its thread */
             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 43dd38c0ea7cc510a76f675af78180525d5baeb6..ba7b956a4b1b5d584cc67b0c8b0bb6eb57edfadc 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;