From: Victor Julien <vjulien@oisf.net>
Date: Sat, 14 Sep 2024 19:26:45 +0000 (+0200)
Subject: flow: improve thread safety during timeout checks
X-Git-Tag: suricata-8.0.0-beta1~597
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7b8214302caa761aae13bc0b5c4614391af088e3;p=thirdparty%2Fsuricata.git

flow: improve thread safety during timeout checks

Timeout checks would access certain fields w/o locking, which could lead
to thread safety issues.
---

diff --git a/src/flow-hash.c b/src/flow-hash.c
index bdcc0dc440..95f55b9e0a 100644
--- a/src/flow-hash.c
+++ b/src/flow-hash.c
@@ -906,16 +906,15 @@ Flow *FlowGetFlowFromHash(ThreadVars *tv, FlowLookupStruct *fls, Packet *p, Flow
     f = fb->head;
     do {
         Flow *next_f = NULL;
+        FLOWLOCK_WRLOCK(f);
         const bool timedout = (fb_nextts < (uint32_t)SCTIME_SECS(p->ts) &&
                                FlowIsTimedOut(f, (uint32_t)SCTIME_SECS(p->ts), emerg));
         if (timedout) {
-            FLOWLOCK_WRLOCK(f);
             next_f = f->next;
             MoveToWorkQueue(tv, fls, fb, f, prev_f);
             FLOWLOCK_UNLOCK(f);
             goto flow_removed;
         } else if (FlowCompare(f, p) != 0) {
-            FLOWLOCK_WRLOCK(f);
             /* found a matching flow that is not timed out */
             if (unlikely(TcpSessionPacketSsnReuse(p, f, f->protoctx))) {
                 Flow *new_f = TcpReuseReplace(tv, fls, fb, f, hash, p);
@@ -933,6 +932,8 @@ Flow *FlowGetFlowFromHash(ThreadVars *tv, FlowLookupStruct *fls, Packet *p, Flow
             FlowReference(dest, f);
             FBLOCK_UNLOCK(fb);
             return f; /* return w/o releasing flow lock */
+        } else {
+            FLOWLOCK_UNLOCK(f);
         }
         /* unless we removed 'f', prev_f needs to point to
          * current 'f' when adding a new flow below. */
diff --git a/src/flow-manager.c b/src/flow-manager.c
index 4c6f165888..99348fc404 100644
--- a/src/flow-manager.c
+++ b/src/flow-manager.c
@@ -327,6 +327,8 @@ static void FlowManagerHashRowTimeout(FlowManagerTimeoutThread *td, Flow *f, SCT
     do {
         checked++;
 
+        FLOWLOCK_WRLOCK(f);
+
         /* check flow timeout based on lastts and state. Both can be
          * accessed w/o Flow lock as we do have the hash row lock (so flow
          * can't disappear) and flow_state is atomic. lastts can only
@@ -334,6 +336,7 @@ static void FlowManagerHashRowTimeout(FlowManagerTimeoutThread *td, Flow *f, SCT
 
         /* timeout logic goes here */
         if (FlowManagerFlowTimeout(f, ts, next_ts, emergency) == false) {
+            FLOWLOCK_UNLOCK(f);
             counters->flows_notimeout++;
 
             prev_f = f;
@@ -341,8 +344,6 @@ static void FlowManagerHashRowTimeout(FlowManagerTimeoutThread *td, Flow *f, SCT
             continue;
         }
 
-        FLOWLOCK_WRLOCK(f);
-
         Flow *next_flow = f->next;
 
 #ifdef CAPTURE_OFFLOAD