Pull request #4446: Avoid data race when latency is enabled during flow ip profiling

Merge in SNORT/snort3 from ~AMUTTUVA/snort3:latency_fix to master

Squashed commit of the following:

commit 6539c68b6d81b515cc74bd98d251805a141a47e3
Author: Akhilesh MY <amuttuva@cisco.com>
Date:   Thu Sep 12 02:29:55 2024 -0400

    perf_monitor,latency: avoid data race when latency is enabled during flow ip profiling

diff --git a/src/latency/latency_module.cc b/src/latency/latency_module.cc
index 4441d7555fd2c1f5cb4d2ba904cb40ee02720bac..15d6c83c6db3b0136e874e7623f21b14a57f4b3a 100644 (file)
--- a/src/latency/latency_module.cc
+++ b/src/latency/latency_module.cc
@@ -27,7 +27,10 @@
 #include <chrono>
 
 #include "main/snort_config.h"
+#include "main/reload_tuner.h"
 #include "trace/trace.h"
+#include "latency/packet_latency.h"
+#include "latency/rule_latency.h"
 
 #include "latency_config.h"
 #include "latency_rules.h"
@@ -171,6 +174,33 @@ static inline bool latency_set(const Value& v, RuleLatencyConfig& config)
     return true;
 }
 
+class LatencyTuner : public snort::ReloadResourceTuner
+{
+public:
+    explicit LatencyTuner(bool enable_packet, bool enable_rule)
+        : enable_packet(enable_packet), enable_rule(enable_rule)
+    {}
+    ~LatencyTuner() override = default;
+
+    bool tinit() override
+    {
+        packet_latency::set_force_enable(enable_packet);
+        rule_latency::set_force_enable(enable_rule);
+
+        return false;
+    }
+
+    bool tune_packet_context() override
+    { return true; }
+
+    bool tune_idle_context() override
+    { return true; }
+
+private:
+    bool enable_packet = false;
+    bool enable_rule = false;
+};
+
 LatencyModule::LatencyModule() : Module(s_name, s_help, s_params)
 { }
 
@@ -201,16 +231,20 @@ bool LatencyModule::set(const char* fqn, Value& v, SnortConfig* sc)
     return true;
 }
 
-bool LatencyModule::end(const char*, int, SnortConfig* sc)
+bool LatencyModule::end(const char* fqn, int, SnortConfig* sc)
 {
-    PacketLatencyConfig& packet_config = sc->latency->packet_latency;
-    RuleLatencyConfig& rule_config = sc->latency->rule_latency;
+    const PacketLatencyConfig& packet_config = sc->latency->packet_latency;
+    const RuleLatencyConfig& rule_config = sc->latency->rule_latency;
 
     if ( packet_config.max_time > CLOCK_ZERO )
-        packet_config.force_enable = true;
+        packet_latency::set_force_enable(true);
 
     if ( rule_config.max_time > CLOCK_ZERO )
-        rule_config.force_enable = true;
+        rule_latency::set_force_enable(true);
+
+    if ( strcmp(fqn, "latency") == 0 )
+        sc->register_reload_handler(new LatencyTuner(packet_latency::force_enabled(), 
+            rule_latency::force_enabled()));
 
     return true;
 }

diff --git a/src/latency/packet_latency.cc b/src/latency/packet_latency.cc
index 7feac2f5229202dc4224e69a91b046355bb66b11..5550dfa3e8e46c7da29254ddab69b18e7c482d6e 100644 (file)
--- a/src/latency/packet_latency.cc
+++ b/src/latency/packet_latency.cc
@@ -47,6 +47,17 @@ static THREAD_LOCAL uint64_t elapsed = 0;
 
 namespace packet_latency
 {
+THREAD_LOCAL bool force_enable = false;
+
+bool force_enabled()
+{
+    return force_enable;
+}
+
+void set_force_enable(bool force)
+{
+    force_enable = force;
+}
 // -----------------------------------------------------------------------------
 // helpers
 // -----------------------------------------------------------------------------
@@ -233,7 +244,7 @@ static inline Impl<>& get_impl()
 
 void PacketLatency::push()
 {
-    if ( packet_latency::config->force_enabled())
+    if ( packet_latency::force_enabled())
     {
         packet_latency::get_impl().push();
         ++latency_stats.total_packets;
@@ -242,7 +253,7 @@ void PacketLatency::push()
 
 void PacketLatency::pop(const Packet* p)
 {
-    if ( packet_latency::config->force_enabled())
+    if ( packet_latency::force_enabled())
     {
         if ( packet_latency::get_impl().pop(p) )
             ++latency_stats.packet_timeouts;
@@ -260,7 +271,7 @@ void PacketLatency::pop(const Packet* p)
 
 bool PacketLatency::fastpath()
 {
-    if ( packet_latency::config->enabled() )
+    if ( packet_latency::force_enabled() )
         return packet_latency::get_impl().fastpath();
 
     return false;

diff --git a/src/latency/packet_latency.h b/src/latency/packet_latency.h
index be14fb1dcfdfad8eb4c611d41f072e8583322384..19c93a27c4324da1ad2d52790b48cf55bd5aa9ba 100644 (file)
--- a/src/latency/packet_latency.h
+++ b/src/latency/packet_latency.h
@@ -21,11 +21,20 @@
 #ifndef PACKET_LATENCY_H
 #define PACKET_LATENCY_H
 
+#include "main/snort_types.h"
+
 namespace snort
 {
 struct Packet;
 }
 
+namespace packet_latency
+{
+    SO_PUBLIC bool force_enabled();
+
+    SO_PUBLIC void set_force_enable(bool force);
+}
+
 class PacketLatency
 {
 public:

diff --git a/src/latency/packet_latency_config.h b/src/latency/packet_latency_config.h
index bf02b082e0a3db6f3bddc37bffdfc28393e4112c..b6375dbe25d130c9d0cb2cdb61f3872e8909633a 100644 (file)
--- a/src/latency/packet_latency_config.h
+++ b/src/latency/packet_latency_config.h
@@ -27,7 +27,7 @@ struct PacketLatencyConfig
 {
     hr_duration max_time = CLOCK_ZERO;
     bool fastpath = false;
-    bool force_enable = false;
+    bool plugin_forced = false;
 #ifdef REG_TEST
     bool test_timeout = false;
 #endif
@@ -40,11 +40,6 @@ struct PacketLatencyConfig
 #endif
         return max_time > CLOCK_ZERO;
     }
-
-    bool force_enabled() const
-    {
-        return force_enable;
-    }
 };
 
 #endif

diff --git a/src/latency/rule_latency.cc b/src/latency/rule_latency.cc
index 3ae84221ed5cfaff3f08ab279c9d55f363aa7307..c899543865e1b785999e39cbb082c7919e95d2d4 100644 (file)
--- a/src/latency/rule_latency.cc
+++ b/src/latency/rule_latency.cc
@@ -47,8 +47,20 @@
 
 using namespace snort;
 
+
 namespace rule_latency
 {
+THREAD_LOCAL bool force_enable = false;
+
+bool force_enabled()
+{
+    return force_enable;
+}
+
+void set_force_enable(bool force)
+{
+    force_enable = force;
+}
 // -----------------------------------------------------------------------------
 // helpers
 // -----------------------------------------------------------------------------
@@ -340,7 +352,7 @@ static inline Impl<>& get_impl()
 
 void RuleLatency::push(const detection_option_tree_root_t& root, Packet* p)
 {
-    if ( rule_latency::config->force_enabled() )
+    if ( rule_latency::force_enabled() )
     {
         if ( rule_latency::get_impl().push(root, p) )
             ++latency_stats.rule_tree_enables;
@@ -351,7 +363,7 @@ void RuleLatency::push(const detection_option_tree_root_t& root, Packet* p)
 
 void RuleLatency::pop()
 {
-    if ( rule_latency::config->force_enabled() )
+    if ( rule_latency::force_enabled() )
     {
         if ( rule_latency::get_impl().pop() )
             ++latency_stats.rule_eval_timeouts;
@@ -360,7 +372,7 @@ void RuleLatency::pop()
 
 bool RuleLatency::suspended()
 {
-    if ( rule_latency::config->enabled() )
+    if ( rule_latency::force_enabled() )
         return rule_latency::get_impl().suspended();
 
     return false;

diff --git a/src/latency/rule_latency.h b/src/latency/rule_latency.h
index 1db0ae6b1773743c2bae74cb694034b00dc6ef7c..834a20e794d870c0e36efcbc607f873aa11252d5 100644 (file)
--- a/src/latency/rule_latency.h
+++ b/src/latency/rule_latency.h
@@ -21,12 +21,20 @@
 #ifndef RULE_LATENCY_H
 #define RULE_LATENCY_H
 
+#include "main/snort_types.h"
+
 struct detection_option_tree_root_t;
 namespace snort
 {
 struct Packet;
 }
 
+namespace rule_latency
+{
+    SO_PUBLIC bool force_enabled();
+    SO_PUBLIC void set_force_enable(bool force);
+}
+
 class RuleLatency
 {
 public:

diff --git a/src/latency/rule_latency_config.h b/src/latency/rule_latency_config.h
index a4f04d4d4d8c6092461f40c8b350ec373999c8ca..2fc2df7a637faf271a7718bc6aef67f04e6bd619 100644 (file)
--- a/src/latency/rule_latency_config.h
+++ b/src/latency/rule_latency_config.h
@@ -26,7 +26,6 @@
 struct RuleLatencyConfig
 {
     hr_duration max_time = 0_ticks;
-    bool force_enable = false;
     bool suspend = false;
     unsigned suspend_threshold = 0;
     hr_duration max_suspend_time = 0_ticks;
@@ -43,11 +42,6 @@ struct RuleLatencyConfig
         return max_time > 0_ticks;
     }
 
-    bool force_enabled() const
-    {
-        return force_enable;
-    }
-
     bool allow_reenable() const { return max_suspend_time > 0_ticks; }
 };
 

diff --git a/src/main/analyzer.cc b/src/main/analyzer.cc
index af72e6e9a7129c3bdcf4224066a4d9c269b1a47c..e67657428bf7fad90e14aa2e1e16a59fe948fa3c 100644 (file)
--- a/src/main/analyzer.cc
+++ b/src/main/analyzer.cc
@@ -47,6 +47,7 @@
 #include "framework/data_bus.h"
 #include "latency/packet_latency.h"
 #include "latency/rule_latency.h"
+#include "latency/latency_config.h"
 #include "log/messages.h"
 #include "main/swapper.h"
 #include "main.h"
@@ -648,6 +649,9 @@ void Analyzer::init_unprivileged()
     HostAttributesManager::initialize();
     RuleContext::set_enabled(sc->profiler->rule.show);
     TimeProfilerStats::set_enabled(sc->profiler->time.show);
+    packet_latency::set_force_enable(sc->latency->packet_latency.enabled() ||
+        sc->latency->packet_latency.plugin_forced);
+    rule_latency::set_force_enable(sc->latency->rule_latency.enabled());
 
     // in case there are HA messages waiting, process them first
     HighAvailabilityManager::process_receive();

diff --git a/src/main/snort_config.cc b/src/main/snort_config.cc
index 09233b601a1f32ce2c72c9322c4936decb3e2df4..fb9e7c87337658de6de427ce7323bac5b9761b9f 100644 (file)
--- a/src/main/snort_config.cc
+++ b/src/main/snort_config.cc
@@ -789,40 +789,16 @@ void SnortConfig::set_overlay_trace_config(TraceConfig* tc)
     overlay_trace_config = tc;
 }
 
-bool SnortConfig::set_packet_latency(bool is_enabled) const
+bool SnortConfig::set_packet_latency() const
 {
     if ( latency )
     {
-        latency->packet_latency.force_enable = is_enabled;
-        return is_enabled;
-    }
-    return false;
-}
-
-bool SnortConfig::get_packet_latency() const
-{
-    if ( latency->packet_latency.force_enabled() )
+        latency->packet_latency.plugin_forced = true;
         return true;
-    return false;
-}
-
-bool SnortConfig::set_rule_latency(bool is_enabled) const
-{
-    if ( latency )
-    {
-        latency->rule_latency.force_enable = is_enabled;
-        return is_enabled;
     }
     return false;
 }
 
-bool SnortConfig::get_rule_latency() const
-{
-    if ( latency->rule_latency.force_enabled() )
-        return true;
-    return false;
-}
-
 void SnortConfig::set_tunnel_verdicts(const char* args)
 {
     char* tmp, * tok;

diff --git a/src/main/snort_config.h b/src/main/snort_config.h
index 3cbaf2431fafff9f5e6c958ad2012b53637c0b7d..0d7f84da9ef27d23f1eb021fe2269c9e41346ff9 100644 (file)
--- a/src/main/snort_config.h
+++ b/src/main/snort_config.h
@@ -468,10 +468,7 @@ public:
     void set_utc(bool);
     void set_watchdog(uint16_t);
     void set_watchdog_min_thread_count(uint16_t);
-    SO_PUBLIC bool set_packet_latency(bool) const;
-    SO_PUBLIC bool get_packet_latency() const;
-    SO_PUBLIC bool set_rule_latency(bool) const;
-    SO_PUBLIC bool get_rule_latency() const;
+    SO_PUBLIC bool set_packet_latency() const;
 
     //------------------------------------------------------
     // accessor methods

diff --git a/src/main/test/distill_verdict_stubs.h b/src/main/test/distill_verdict_stubs.h
index 7bc5e90adedfcec3e65be35c53e92d3c09268db2..e7aa89717b359bbc361675c66eb394ef28ea4356 100644 (file)
--- a/src/main/test/distill_verdict_stubs.h
+++ b/src/main/test/distill_verdict_stubs.h
@@ -96,7 +96,9 @@ void EventTrace_Term() { }
 void detection_filter_init(DetectionFilterConfig*) { }
 void detection_filter_term() { }
 void RuleLatency::tterm() { }
+void rule_latency::set_force_enable(bool) { }
 void PacketLatency::tterm() { }
+void packet_latency::set_force_enable(bool) { }
 void SideChannelManager::thread_init() { }
 void SideChannelManager::thread_term() { }
 void CodecManager::thread_init() { }

diff --git a/src/network_inspectors/perf_monitor/flow_ip_tracker.cc b/src/network_inspectors/perf_monitor/flow_ip_tracker.cc
index 32c62694ba6e65a63a4a933cda78b95620f8c43a..b036ef635eeb7729b1cd2987112b395ab5459d45 100644 (file)
--- a/src/network_inspectors/perf_monitor/flow_ip_tracker.cc
+++ b/src/network_inspectors/perf_monitor/flow_ip_tracker.cc
@@ -193,8 +193,7 @@ void FlowIPTracker::update(Packet* p)
         uint64_t curr_flow_latency = 0;
         uint64_t curr_rule_latency = 0;
 
-        PerfMonitor* perf_monitor = (PerfMonitor*)PigPen::get_inspector(PERF_NAME, true);
-        if ( perf_monitor->get_constraints()->flow_ip_all == true )
+        if ( t_constraints->flow_ip_all == true )
         {
             if ( p->flow )
             {

diff --git a/src/network_inspectors/perf_monitor/perf_module.cc b/src/network_inspectors/perf_monitor/perf_module.cc
index 6f02f7e872a26b8c536a8dac44e11870a99e28ee..33cfa82b283a77233a5c39eb0a38d404da5543b6 100644 (file)
--- a/src/network_inspectors/perf_monitor/perf_module.cc
+++ b/src/network_inspectors/perf_monitor/perf_module.cc
@@ -32,6 +32,8 @@
 #include "main/analyzer_command.h"
 #include "main/snort_config.h"
 #include "managers/module_manager.h"
+#include "latency/packet_latency.h"
+#include "latency/rule_latency.h"
 
 #include "perf_monitor.h"
 #include "perf_pegs.h"
@@ -118,7 +120,7 @@ private:
     PerfMonitor* perf_monitor;
 };
 
-static bool current_packet_latency, current_rule_latency = false;
+static THREAD_LOCAL bool current_packet_latency, current_rule_latency = false;
 
 static const Parameter flow_ip_profiling_params[] =
 {
@@ -137,10 +139,27 @@ static const Parameter flow_ip_profiling_params[] =
 bool PerfMonFlowIPDebug::execute(Analyzer&, void**)
 {
     if (enable)
+    {
+        if ( constraints->flow_ip_all )
+        {
+            if (rule_latency::force_enabled())
+                current_rule_latency = true;
+            if (packet_latency::force_enabled())
+                current_packet_latency = true;
+            rule_latency::set_force_enable(true);
+            packet_latency::set_force_enable(true);
+        }
         perf_monitor->enable_profiling(constraints);
+    }
     else
-        perf_monitor->disable_profiling(constraints);
+    {
+        if ( !current_packet_latency )
+            packet_latency::set_force_enable(false);
+        if ( !current_rule_latency )
+            rule_latency::set_force_enable(false);
 
+        perf_monitor->disable_profiling(constraints);
+    }
     return true;
 }
 
@@ -162,17 +181,6 @@ static int enable_flow_ip_profiling(lua_State* L)
     ControlConn* ctrlcon = ControlConn::query_from_lua(L);
     main_broadcast_command(new PerfMonFlowIPDebug(new_constraints, true, perf_monitor), ctrlcon);
 
-    if ( new_constraints->flow_ip_all )
-    {
-        const SnortConfig* sc = SnortConfig::get_conf();
-        if ( sc->get_packet_latency() )
-            current_packet_latency = true;
-        if ( sc->get_rule_latency() )
-            current_rule_latency = true;
-        sc->set_packet_latency(true);
-        sc->set_rule_latency(true);
-    }
-
     LogMessage("Enabling flow ip profiling with sample interval %d packet count %d all stats tracking %s\n",
             new_constraints->sample_interval, new_constraints->pkt_cnt,
         ( new_constraints->flow_ip_all ) ? "enabled" : "disabled" );
@@ -204,14 +212,6 @@ static int disable_flow_ip_profiling(lua_State* L)
     ControlConn* ctrlcon = ControlConn::query_from_lua(L);
     main_broadcast_command(new PerfMonFlowIPDebug(new_constraints, false, perf_monitor), ctrlcon);
 
-    const SnortConfig* sc = SnortConfig::get_conf();
-
-    if ( !current_packet_latency )
-        sc->set_packet_latency(false);
-
-    if ( !current_rule_latency )
-        sc->set_rule_latency(false);
-
     LogMessage("Disabling flow ip profiling\n");
 
     return 0;
@@ -366,8 +366,8 @@ bool PerfMonModule::end(const char* fqn, int idx, SnortConfig* sc)
 
     if ( config->flow_ip_all )
     {
-        sc->set_packet_latency(true); 
-        sc->set_rule_latency(true);
+        packet_latency::set_force_enable(true);
+        rule_latency::set_force_enable(true);
     }
 
     return true;

diff --git a/src/network_inspectors/perf_monitor/perf_module.h b/src/network_inspectors/perf_monitor/perf_module.h
index 04d89c58403979bb690ab20a5955c9b646f9c6a0..e5935584a465effac97273778455562a10efc263 100644 (file)
--- a/src/network_inspectors/perf_monitor/perf_module.h
+++ b/src/network_inspectors/perf_monitor/perf_module.h
@@ -136,6 +136,7 @@ private:
 
 extern THREAD_LOCAL PerfPegStats pmstats;
 extern THREAD_LOCAL snort::ProfileStats perfmonStats;
+extern THREAD_LOCAL PerfConstraints* t_constraints;
 
 #endif
 

diff --git a/src/network_inspectors/perf_monitor/perf_monitor.cc b/src/network_inspectors/perf_monitor/perf_monitor.cc
index 2bf8beffb8ac9ce3afbb2185d33658b2c009aa34..28d928e7ea8bf3d18a45d50a4c7cc70395dc1011 100644 (file)
--- a/src/network_inspectors/perf_monitor/perf_monitor.cc
+++ b/src/network_inspectors/perf_monitor/perf_monitor.cc
@@ -54,7 +54,7 @@ THREAD_LOCAL ProfileStats perfmonStats;
 static THREAD_LOCAL std::vector<PerfTracker*>* trackers;
 static THREAD_LOCAL FlowIPTracker* flow_ip_tracker = nullptr;
 
-static THREAD_LOCAL PerfConstraints* t_constraints;
+THREAD_LOCAL PerfConstraints* t_constraints;
 
 //-------------------------------------------------------------------------
 // class stuff
@@ -123,7 +123,7 @@ public:
         uint64_t flow_latency = 0;
         uint64_t rule_latency = 0;
 
-        if ( perf_monitor.get_constraints()->flow_ip_all )
+        if ( t_constraints->flow_ip_all )
         {
             const AppIdSessionApi* appid_session_api = appid_api.get_appid_session_api(*flow);
             if ( appid_session_api )