detect: remove lock from global keyword logic

The global keyword registration and per thread init handling used
the lock from the DetectEngineMasterCtx. This lead to a dead lock
situation at multi-tenancy tenant reloads.

The lock was unnecessary however, as the only time the registration
list is updated is at engine initialization. At that time Suricata
is still running in a single thread. After this, the data structure
doesn't change anymore.

Bug #2516.

diff --git a/src/detect-engine.c b/src/detect-engine.c
index 4cadfa523ecd0d9eb858275c83380c4b83e78b0c..8e61aa999150866f7990a7ec17f50fca277e48e8 100644 (file)
--- a/src/detect-engine.c
+++ b/src/detect-engine.c
@@ -1980,33 +1980,28 @@ void DetectEngineResetMaxSigId(DetectEngineCtx *de_ctx)
 
 static int DetectEngineThreadCtxInitGlobalKeywords(DetectEngineThreadCtx *det_ctx)
 {
-    DetectEngineMasterCtx *master = &g_master_de_ctx;
-    SCMutexLock(&master->lock);
+    const DetectEngineMasterCtx *master = &g_master_de_ctx;
 
     if (master->keyword_id > 0) {
         det_ctx->global_keyword_ctxs_array = (void **)SCCalloc(master->keyword_id, sizeof(void *));
         if (det_ctx->global_keyword_ctxs_array == NULL) {
             SCLogError(SC_ERR_DETECT_PREPARE, "setting up thread local detect ctx");
-            goto error;
+            return TM_ECODE_FAILED;
         }
         det_ctx->global_keyword_ctxs_size = master->keyword_id;
 
-        DetectEngineThreadKeywordCtxItem *item = master->keyword_list;
+        const DetectEngineThreadKeywordCtxItem *item = master->keyword_list;
         while (item) {
             det_ctx->global_keyword_ctxs_array[item->id] = item->InitFunc(item->data);
             if (det_ctx->global_keyword_ctxs_array[item->id] == NULL) {
                 SCLogError(SC_ERR_DETECT_PREPARE, "setting up thread local detect ctx "
                         "for keyword \"%s\" failed", item->name);
-                goto error;
+                return TM_ECODE_FAILED;
             }
             item = item->next;
         }
     }
-    SCMutexUnlock(&master->lock);
     return TM_ECODE_OK;
-error:
-    SCMutexUnlock(&master->lock);
-    return TM_ECODE_FAILED;
 }
 
 static void DetectEngineThreadCtxDeinitGlobalKeywords(DetectEngineThreadCtx *det_ctx)
@@ -2016,11 +2011,9 @@ static void DetectEngineThreadCtxDeinitGlobalKeywords(DetectEngineThreadCtx *det
         return;
     }
 
-    DetectEngineMasterCtx *master = &g_master_de_ctx;
-    SCMutexLock(&master->lock);
-
+    const DetectEngineMasterCtx *master = &g_master_de_ctx;
     if (master->keyword_id > 0) {
-        DetectEngineThreadKeywordCtxItem *item = master->keyword_list;
+        const DetectEngineThreadKeywordCtxItem *item = master->keyword_list;
         while (item) {
             if (det_ctx->global_keyword_ctxs_array[item->id] != NULL)
                 item->FreeFunc(det_ctx->global_keyword_ctxs_array[item->id]);
@@ -2031,7 +2024,6 @@ static void DetectEngineThreadCtxDeinitGlobalKeywords(DetectEngineThreadCtx *det
         SCFree(det_ctx->global_keyword_ctxs_array);
         det_ctx->global_keyword_ctxs_array = NULL;
     }
-    SCMutexUnlock(&master->lock);
 }
 
 static int DetectEngineThreadCtxInitKeywords(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx)
@@ -2604,14 +2596,12 @@ int DetectRegisterThreadCtxGlobalFuncs(const char *name,
     BUG_ON(InitFunc == NULL || FreeFunc == NULL);
 
     DetectEngineMasterCtx *master = &g_master_de_ctx;
-    SCMutexLock(&master->lock);
 
     /* if already registered, return existing id */
     DetectEngineThreadKeywordCtxItem *item = master->keyword_list;
     while (item != NULL) {
         if (strcmp(name, item->name) == 0) {
             id = item->id;
-            SCMutexUnlock(&master->lock);
             return id;
         }
 
@@ -2620,7 +2610,6 @@ int DetectRegisterThreadCtxGlobalFuncs(const char *name,
 
     item = SCCalloc(1, sizeof(*item));
     if (unlikely(item == NULL)) {
-        SCMutexUnlock(&master->lock);
         return -1;
     }
     item->InitFunc = InitFunc;
@@ -2633,7 +2622,6 @@ int DetectRegisterThreadCtxGlobalFuncs(const char *name,
     item->id = master->keyword_id++;
 
     id = item->id;
-    SCMutexUnlock(&master->lock);
     return id;
 }
 

diff --git a/src/detect.h b/src/detect.h
index 8776d4725e600a376de19e453a56ab99c2ab282e..60dc08ae93475303feeb3eb7cffac6e79d2deeb4 100644 (file)
--- a/src/detect.h
+++ b/src/detect.h
@@ -1353,7 +1353,9 @@ typedef struct DetectEngineMasterCtx_ {
      *  structures. */
     DetectEngineTenantMapping *tenant_mapping_list;
 
-    /** list of keywords that need thread local ctxs */
+    /** list of keywords that need thread local ctxs,
+     *  only updated by keyword registration at start up. Not
+     *  covered by the lock. */
     DetectEngineThreadKeywordCtxItem *keyword_list;
     int keyword_id;
 } DetectEngineMasterCtx;