threading: fix shutdown of IPS autofp modes

For IPS modes with a verdict thread in autofp there was an issue with
the verdict thread not shutting down, leading to a long shutdown time
until an error condition was reached.

The problem was that when the packet threads, of which the verdict
thread is one, were told to enter their flow timeout loop the verdict
thread got stuck as it immediately progressed to THV_RUNNING_DONE
instead of the expected THV_FLOW_LOOP.

This patch updates the shutdown logic to only apply the flow timeout
logic to the relevant threads, and skip the verdict thread(s).

Add TM_FLAG_VERDICT_TM to indicate a thread has a verdict module to more
explicitly shut it down.

Fixes: 12f8f03532e5 ("threads: fix autofp shutdown race condition")
Bug: #7681.

diff --git a/src/source-ipfw.c b/src/source-ipfw.c
index b7569e590408f874814bc019d0ce4ef5b0f44d66..24a6dd334a615433af7e57136ccf881033824643 100644 (file)
--- a/src/source-ipfw.c
+++ b/src/source-ipfw.c
@@ -67,6 +67,7 @@ void TmModuleVerdictIPFWRegister (void)
     tmm_modules[TMM_VERDICTIPFW].Func = NULL;
     tmm_modules[TMM_VERDICTIPFW].ThreadExitPrintStats = NULL;
     tmm_modules[TMM_VERDICTIPFW].ThreadDeinit = NULL;
+    tmm_modules[TMM_VERDICTIPFW].flags = TM_FLAG_VERDICT_TM;
 }
 
 void TmModuleDecodeIPFWRegister (void)
@@ -177,6 +178,7 @@ void TmModuleVerdictIPFWRegister (void)
     tmm_modules[TMM_VERDICTIPFW].ThreadDeinit = VerdictIPFWThreadDeinit;
     tmm_modules[TMM_VERDICTIPFW].cap_flags = SC_CAP_NET_ADMIN | SC_CAP_NET_RAW |
                                              SC_CAP_NET_BIND_SERVICE; /** \todo untested */
+    tmm_modules[TMM_VERDICTIPFW].flags = TM_FLAG_VERDICT_TM;
 }
 
 /**

diff --git a/src/source-nfq.c b/src/source-nfq.c
index e0e9869c14d9c8c7fed68a1a270b14a3407fb88c..6498887f593dc2f9add1cc0f4a0871f6e24d1e78 100644 (file)
--- a/src/source-nfq.c
+++ b/src/source-nfq.c
@@ -75,6 +75,7 @@ void TmModuleVerdictNFQRegister (void)
     tmm_modules[TMM_VERDICTNFQ].ThreadExitPrintStats = NULL;
     tmm_modules[TMM_VERDICTNFQ].ThreadDeinit = NULL;
     tmm_modules[TMM_VERDICTNFQ].cap_flags = SC_CAP_NET_ADMIN;
+    tmm_modules[TMM_VERDICTNFQ].flags = TM_FLAG_VERDICT_TM;
 }
 
 void TmModuleDecodeNFQRegister (void)
@@ -187,6 +188,7 @@ void TmModuleVerdictNFQRegister (void)
     tmm_modules[TMM_VERDICTNFQ].ThreadInit = VerdictNFQThreadInit;
     tmm_modules[TMM_VERDICTNFQ].Func = VerdictNFQ;
     tmm_modules[TMM_VERDICTNFQ].ThreadDeinit = VerdictNFQThreadDeinit;
+    tmm_modules[TMM_VERDICTNFQ].flags = TM_FLAG_VERDICT_TM;
 }
 
 void TmModuleDecodeNFQRegister (void)

diff --git a/src/source-windivert.c b/src/source-windivert.c
index 225e07fe7fbf47217b2b045d35225b077d4e9e11..938c623efcc18c5e40062da392cbf35cd804b594 100644 (file)
--- a/src/source-windivert.c
+++ b/src/source-windivert.c
@@ -69,6 +69,7 @@ void TmModuleVerdictWinDivertRegister(void)
 {
     tmm_modules[TMM_VERDICTWINDIVERT].name = "VerdictWinDivert";
     tmm_modules[TMM_VERDICTWINDIVERT].ThreadInit = NoWinDivertSupportExit;
+    tmm_modules[TMM_VERDICTWINDIVERT].flags = TM_FLAG_VERDICT_TM;
 }
 
 void TmModuleDecodeWinDivertRegister(void)
@@ -382,6 +383,7 @@ void TmModuleVerdictWinDivertRegister(void)
     tm_ptr->ThreadInit = VerdictWinDivertThreadInit;
     tm_ptr->Func = VerdictWinDivert;
     tm_ptr->ThreadDeinit = VerdictWinDivertThreadDeinit;
+    tm_ptr->flags = TM_FLAG_VERDICT_TM;
 }
 
 void TmModuleDecodeWinDivertRegister(void)

diff --git a/src/suricata.c b/src/suricata.c
index 4ce1a9d10f789ca6bee081631d3a3ca1435b95d3..a862a0aaedbb2b78546451806c5e83a7fd6682bc 100644 (file)
--- a/src/suricata.c
+++ b/src/suricata.c
@@ -2290,12 +2290,13 @@ void PostRunDeinit(const int runmode, struct timeval *start_time)
     FlowDisableFlowManagerThread();
     /* disable capture */
     TmThreadDisableReceiveThreads();
-    /* tell packet threads to enter flow timeout loop */
-    TmThreadDisablePacketThreads(THV_REQ_FLOW_LOOP, THV_FLOW_LOOP);
+    /* tell relevant packet threads to enter flow timeout loop */
+    TmThreadDisablePacketThreads(
+            THV_REQ_FLOW_LOOP, THV_FLOW_LOOP, (TM_FLAG_RECEIVE_TM | TM_FLAG_DETECT_TM));
     /* run cleanup on the flow hash */
     FlowWorkToDoCleanup();
-    /* gracefully shut down packet threads */
-    TmThreadDisablePacketThreads(THV_KILL, THV_RUNNING_DONE);
+    /* gracefully shut down all packet threads */
+    TmThreadDisablePacketThreads(THV_KILL, THV_RUNNING_DONE, TM_FLAG_PACKET_ALL);
     SCPrintElapsedTime(start_time);
     FlowDisableFlowRecyclerThread();
 

diff --git a/src/tm-modules.h b/src/tm-modules.h
index f155089e2cc90393e6edd105b00201f8f7b5f8cd..ef7b63f889d72e01e17c183ff8072e7a0f75899a 100644 (file)
--- a/src/tm-modules.h
+++ b/src/tm-modules.h
@@ -35,6 +35,12 @@
 #define TM_FLAG_DETECT_TM       0x08
 #define TM_FLAG_MANAGEMENT_TM   0x10
 #define TM_FLAG_COMMAND_TM      0x20
+#define TM_FLAG_VERDICT_TM      0x40
+
+/* all packet modules combined */
+#define TM_FLAG_PACKET_ALL                                                                         \
+    (TM_FLAG_RECEIVE_TM | TM_FLAG_DECODE_TM | TM_FLAG_STREAM_TM | TM_FLAG_DETECT_TM |              \
+            TM_FLAG_VERDICT_TM)
 
 typedef TmEcode (*ThreadInitFunc)(ThreadVars *, const void *, void **);
 typedef TmEcode (*ThreadDeinitFunc)(ThreadVars *, void *);

diff --git a/src/tm-threads.c b/src/tm-threads.c
index fa05e72753f3cde59a01698953fb42ba1028df40..dd65de620e7ad154bc58884517834e08979a1b34 100644 (file)
--- a/src/tm-threads.c
+++ b/src/tm-threads.c
@@ -517,7 +517,7 @@ static void *TmThreadsSlotVar(void *td)
             TmThreadsHandleInjectedPackets(tv);
         }
 
-        if (TmThreadsCheckFlag(tv, THV_REQ_FLOW_LOOP)) {
+        if (TmThreadsCheckFlag(tv, (THV_KILL | THV_REQ_FLOW_LOOP))) {
             run = false;
         }
     }
@@ -1508,10 +1508,21 @@ static void TmThreadDebugValidateNoMorePackets(void)
 #endif
 }
 
+/** \internal
+ *  \brief check if a thread has any of the modules indicated by TM_FLAG_*
+ *  \param tv thread
+ *  \param flags TM_FLAG_*'s
+ *  \retval bool true if at least on of the flags is present */
+static inline bool CheckModuleFlags(const ThreadVars *tv, const uint8_t flags)
+{
+    return (tv->tmm_flags & flags) != 0;
+}
+
 /**
  * \brief Disable all packet threads
  * \param set flag to set
  * \param check flag to check
+ * \param module_flags bitflags of TmModule's to apply the `set` flag to.
  *
  * Support 2 stages in shutting down the packet threads:
  * 1. set THV_REQ_FLOW_LOOP and wait for THV_FLOW_LOOP
@@ -1519,8 +1530,11 @@ static void TmThreadDebugValidateNoMorePackets(void)
  *
  * During step 1 the main loop is exited, and the flow loop logic is entered.
  * During step 2, the flow loop logic is done and the thread closes.
+ *
+ * `module_flags` limits which threads are disabled
  */
-void TmThreadDisablePacketThreads(const uint16_t set, const uint16_t check)
+void TmThreadDisablePacketThreads(
+        const uint16_t set, const uint16_t check, const uint8_t module_flags)
 {
     struct timeval start_ts;
     struct timeval cur_ts;
@@ -1544,6 +1558,11 @@ again:
     /* loop through the packet threads and kill them */
     SCMutexLock(&tv_root_lock);
     for (ThreadVars *tv = tv_root[TVT_PPT]; tv != NULL; tv = tv->next) {
+        /* only set flow worker threads to THV_REQ_FLOW_LOOP */
+        if (!CheckModuleFlags(tv, module_flags)) {
+            SCLogDebug("%s does not have any of the modules %02x, skip", tv->name, module_flags);
+            continue;
+        }
         TmThreadsSetFlag(tv, set);
 
         /* separate worker threads (autofp) will still wait at their
@@ -1559,8 +1578,9 @@ again:
         }
 
         /* wait for it to reach the expected state */
-        while (!TmThreadsCheckFlag(tv, check)) {
+        if (!TmThreadsCheckFlag(tv, check)) {
             SCMutexUnlock(&tv_root_lock);
+            SCLogDebug("%s did not reach state %u, again", tv->name, check);
 
             SleepMsec(1);
             goto again;

diff --git a/src/tm-threads.h b/src/tm-threads.h
index 092a91c4166eb9a8e2c47d061519cfb721f26814..8998d075328f5a3d4adb052efa674ced02c30bee 100644 (file)
--- a/src/tm-threads.h
+++ b/src/tm-threads.h
@@ -122,7 +122,8 @@ void TmThreadWaitForFlag(ThreadVars *, uint32_t);
 
 TmEcode TmThreadsSlotVarRun (ThreadVars *tv, Packet *p, TmSlot *slot);
 
-void TmThreadDisablePacketThreads(const uint16_t set, const uint16_t check);
+void TmThreadDisablePacketThreads(
+        const uint16_t set, const uint16_t check, const uint8_t module_flags);
 void TmThreadDisableReceiveThreads(void);
 
 uint32_t TmThreadCountThreadsByTmmFlags(uint8_t flags);