From: Akhilesh MY (amuttuva) Date: Mon, 7 Oct 2024 11:34:23 +0000 (+0000) Subject: Pull request #4446: Avoid data race when latency is enabled during flow ip profiling X-Git-Tag: 3.4.0.0~8 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2a5f973c230e5df8eaffed48e9c82d4a2e8ee9f7;p=thirdparty%2Fsnort3.git 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 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 4441d7555..15d6c83c6 100644 --- a/src/latency/latency_module.cc +++ b/src/latency/latency_module.cc @@ -27,7 +27,10 @@ #include #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 7feac2f52..5550dfa3e 100644 --- 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 be14fb1dc..19c93a27c 100644 --- 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 bf02b082e..b6375dbe2 100644 --- 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 3ae84221e..c89954386 100644 --- 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 1db0ae6b1..834a20e79 100644 --- 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 a4f04d4d4..2fc2df7a6 100644 --- 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 af72e6e9a..e67657428 100644 --- 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 09233b601..fb9e7c873 100644 --- 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 3cbaf2431..0d7f84da9 100644 --- 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 7bc5e90ad..e7aa89717 100644 --- 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 32c62694b..b036ef635 100644 --- 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 6f02f7e87..33cfa82b2 100644 --- 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 04d89c584..e5935584a 100644 --- 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 2bf8beffb..28d928e7e 100644 --- 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* 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 )