From: Mike Stepanek (mstepane) Date: Tue, 19 Nov 2019 17:49:16 +0000 (+0000) Subject: Merge pull request #1846 in SNORT/snort3 from ~DAVMCPHE/snort3:stream_reload_tweaks... X-Git-Tag: 3.0.0-265~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=02b3037c49a5d2c379bb3b62d446291ce567e02e;p=thirdparty%2Fsnort3.git Merge pull request #1846 in SNORT/snort3 from ~DAVMCPHE/snort3:stream_reload_tweaks to master Squashed commit of the following: commit d4f864cf104f1cad64a800948461613e75fac1d4 Author: davis mcpherson Date: Mon Nov 18 08:25:24 2019 -0500 snort: update reload resource tuner to return status indicating if there is work to be done in the packet thread. stream: register reload resource tuner unconditionally. move checks for config changes to the tuner tinit method analyzer_command: update ACSwap execute to check return status from resource tuner tinit and delete tuner if no work (return status == false) --- diff --git a/src/flow/flow_cache.h b/src/flow/flow_cache.h index bbd6793cd..72c450915 100644 --- a/src/flow/flow_cache.h +++ b/src/flow/flow_cache.h @@ -88,11 +88,16 @@ public: void unlink_uni(snort::Flow*); - void set_flow_cache_config(FlowCacheConfig& cfg) { config = cfg; } + void set_flow_cache_config(FlowCacheConfig& cfg) + { config = cfg; } + + const FlowCacheConfig& get_flow_cache_config() const + { return config; } unsigned get_flows_allocated() const { return flows_allocated; } + private: void push(snort::Flow*); void link_uni(snort::Flow*); diff --git a/src/flow/flow_control.cc b/src/flow/flow_control.cc index 74d57b8bb..f700326a7 100644 --- a/src/flow/flow_control.cc +++ b/src/flow/flow_control.cc @@ -84,18 +84,20 @@ void FlowControl::clear_counts() // cache foo //------------------------------------------------------------------------- +void FlowControl::set_flow_cache_config(FlowCacheConfig& cfg) +{ cache->set_flow_cache_config(cfg); } + +const FlowCacheConfig& FlowControl::get_flow_cache_config() const +{ return cache->get_flow_cache_config(); } + unsigned FlowControl::get_flows_allocated() const { return cache->get_flows_allocated(); } Flow* FlowControl::find_flow(const FlowKey* key) -{ - return cache->find(key); -} +{ return cache->find(key); } Flow* FlowControl::new_flow(const FlowKey* key) -{ - return cache->allocate(key); -} +{ return cache->allocate(key); } void FlowControl::release_flow(const FlowKey* key) { @@ -104,25 +106,17 @@ void FlowControl::release_flow(const FlowKey* key) } void FlowControl::release_flow(Flow* flow, PruneReason reason) -{ - cache->release(flow, reason); -} +{ cache->release(flow, reason); } void FlowControl::purge_flows () -{ - cache->purge(); -} +{ cache->purge(); } unsigned FlowControl::delete_flows(unsigned num_to_delete) -{ - return cache->delete_flows(num_to_delete); -} +{ return cache->delete_flows(num_to_delete); } // hole for memory manager/prune handler bool FlowControl::prune_one(PruneReason reason, bool do_cleanup) -{ - return cache->prune_one(reason, do_cleanup); -} +{ return cache->prune_one(reason, do_cleanup); } void FlowControl::timeout_flows(time_t cur_time) { @@ -497,11 +491,6 @@ bool FlowControl::expected_flow(Flow* flow, Packet* p) return ignore; } -void FlowControl::update_flow_cache_cfg(FlowCacheConfig& cfg) -{ - cache->set_flow_cache_config(cfg); -} - int FlowControl::add_expected( const Packet* ctrlPkt, PktType type, IpProtocol ip_proto, const SfIp *srcIP, uint16_t srcPort, diff --git a/src/flow/flow_control.h b/src/flow/flow_control.h index 750c09a4a..fc178a96f 100644 --- a/src/flow/flow_control.h +++ b/src/flow/flow_control.h @@ -52,24 +52,21 @@ public: FlowControl(const FlowCacheConfig& fc); ~FlowControl(); -public: - bool process(PktType, snort::Packet*, bool* new_flow = nullptr); - - snort::Flow* find_flow(const snort::FlowKey*); - snort::Flow* new_flow(const snort::FlowKey*); - unsigned get_flows_allocated() const; - + void set_flow_cache_config(FlowCacheConfig& cfg); + const FlowCacheConfig& get_flow_cache_config() const; void init_proto(PktType, snort::InspectSsnFunc); void init_exp(uint32_t max); + unsigned get_flows_allocated() const; + bool process(PktType, snort::Packet*, bool* new_flow = nullptr); + snort::Flow* find_flow(const snort::FlowKey*); + snort::Flow* new_flow(const snort::FlowKey*); void release_flow(const snort::FlowKey*); void release_flow(snort::Flow*, PruneReason); void purge_flows(); unsigned delete_flows(unsigned num_to_delete); bool prune_one(PruneReason, bool do_cleanup); snort::Flow* stale_flow_cleanup(FlowCache*, snort::Flow*, snort::Packet*); - - void update_flow_cache_cfg(FlowCacheConfig& cfg); void timeout_flows(time_t cur_time); bool expected_flow(snort::Flow*, snort::Packet*); bool is_expected(snort::Packet*); diff --git a/src/main/analyzer_command.cc b/src/main/analyzer_command.cc index bf358a0e5..ee9170afd 100644 --- a/src/main/analyzer_command.cc +++ b/src/main/analyzer_command.cc @@ -113,10 +113,15 @@ bool ACSwap::execute(Analyzer& analyzer, void** ac_state) if ( !*ac_state ) { reload_tuners = new std::list(sc->get_reload_resource_tuners()); + std::list::iterator rtt = reload_tuners->begin(); + while ( rtt != reload_tuners->end() ) + { + if ( (*rtt)->tinit() ) + ++rtt; + else + rtt = reload_tuners->erase(rtt); + } *ac_state = reload_tuners; - for (auto const& rtt : *reload_tuners) - rtt->tinit(); - } else reload_tuners = (std::list*)*ac_state; diff --git a/src/main/snort_config.h b/src/main/snort_config.h index 9cb93dbeb..d51864526 100644 --- a/src/main/snort_config.h +++ b/src/main/snort_config.h @@ -162,13 +162,15 @@ public: virtual ~ReloadResourceTuner() = default; - virtual void tinit() { } + // returns true if resource tuning required, false otherwise + virtual bool tinit() = 0; + + // each of these returns true if resource tuning is complete, false otherwise virtual bool tune_packet_context() = 0; virtual bool tune_idle_context() = 0; protected: ReloadResourceTuner() = default; - virtual bool tune_resources(unsigned work_limit) = 0; unsigned max_work = RELOAD_MAX_WORK_PER_PACKET; unsigned max_work_idle = RELOAD_MAX_WORK_WHEN_IDLE; diff --git a/src/stream/base/stream_module.cc b/src/stream/base/stream_module.cc index 8e88a97c3..4eb969c7a 100644 --- a/src/stream/base/stream_module.cc +++ b/src/stream/base/stream_module.cc @@ -29,6 +29,7 @@ #include "main/snort.h" #include "main/snort_config.h" #include "main/snort_debug.h" +#include "stream/flush_bucket.h" using namespace snort; using namespace std; @@ -177,31 +178,10 @@ bool StreamModule::set(const char* fqn, Value& v, SnortConfig* c) // FIXIT-L the detection of stream.xxx_cache changes below is a temporary workaround // remove this check when stream.xxx_cache params become reloadable -bool StreamModule::end(const char* fqn, int, SnortConfig* cfg) +bool StreamModule::end(const char*, int, SnortConfig* sc) { - static StreamModuleConfig saved_config = {}; - - if ( saved_config.flow_cache_cfg.max_flows ) - { - int max_flows_change = config.flow_cache_cfg.max_flows - saved_config.flow_cache_cfg.max_flows; - if ( max_flows_change ) - { - // register handler - reload_resource_manager.initialize(config.flow_cache_cfg, max_flows_change); - cfg->register_reload_resource_tuner(reload_resource_manager); - } - } - - if ( !strcmp(fqn, "stream") ) - { - if ( saved_config.flow_cache_cfg.max_flows // saved config is valid - and config.footprint != saved_config.footprint ) - { - ReloadError("Changing of stream.footprint requires a restart\n"); - } - else - saved_config = config; - } + if ( reload_resource_manager.initialize(config) ) + sc->register_reload_resource_tuner(reload_resource_manager); return true; } @@ -216,19 +196,44 @@ void StreamModule::reset_stats() { base_reset(); } // Stream handler to adjust allocated resources as needed on a config reload -void StreamReloadResourceManager::initialize(FlowCacheConfig& config_, int max_flows_change_) +bool StreamReloadResourceManager::initialize(StreamModuleConfig& config_) { + // FIXIT-L - saving config here to check footprint change is a bit of a hack, + if ( Snort::is_reloading() ) + { + if ( config.footprint != config_.footprint ) + { + // FIXIT-M - reinit FlushBucket... + ReloadError("Changing of stream.footprint requires a restart\n"); + return false; + } + + config = config_; + return true; + } + config = config_; - max_flows_change = max_flows_change_; + return false; } -void StreamReloadResourceManager::tinit() +bool StreamReloadResourceManager::tinit() { - flow_con->update_flow_cache_cfg(config); - if ( max_flows_change < 0 ) - stream_base_stats.reload_total_deletes += abs(max_flows_change); - else - stream_base_stats.reload_total_adds += max_flows_change; + bool must_tune = false; + + max_flows_change = + config.flow_cache_cfg.max_flows - flow_con->get_flow_cache_config().max_flows; + if ( max_flows_change ) + { + if ( max_flows_change < 0 ) + stream_base_stats.reload_total_deletes += abs(max_flows_change); + else + stream_base_stats.reload_total_adds += max_flows_change; + + flow_con->set_flow_cache_config(config.flow_cache_cfg); + must_tune = true; + } + + return must_tune; } bool StreamReloadResourceManager::tune_packet_context() @@ -244,14 +249,15 @@ bool StreamReloadResourceManager::tune_idle_context() bool StreamReloadResourceManager::tune_resources(unsigned work_limit) { // we are done if new max is > currently allocated flow objects - if ( flow_con->get_flows_allocated() <= config.max_flows ) + if ( flow_con->get_flows_allocated() <= config.flow_cache_cfg.max_flows ) return true; - unsigned flows_to_delete = flow_con->get_flows_allocated() - config.max_flows; + unsigned flows_to_delete = + flow_con->get_flows_allocated() - config.flow_cache_cfg.max_flows; if ( flows_to_delete > work_limit ) - flows_to_delete -= flow_con->delete_flows(work_limit); + flows_to_delete -= flow_con->delete_flows(work_limit); else - flows_to_delete -= flow_con->delete_flows(flows_to_delete); + flows_to_delete -= flow_con->delete_flows(flows_to_delete); return ( flows_to_delete ) ? false : true; } diff --git a/src/stream/base/stream_module.h b/src/stream/base/stream_module.h index f2b6508b0..c6d3e1128 100644 --- a/src/stream/base/stream_module.h +++ b/src/stream/base/stream_module.h @@ -72,7 +72,7 @@ extern THREAD_LOCAL BaseStats stream_base_stats; struct StreamModuleConfig { FlowCacheConfig flow_cache_cfg; - unsigned footprint; + unsigned footprint = 0; }; class StreamReloadResourceManager : public snort::ReloadResourceTuner @@ -80,16 +80,17 @@ class StreamReloadResourceManager : public snort::ReloadResourceTuner public: StreamReloadResourceManager() {} - void initialize(FlowCacheConfig&, int max_flows_change_); - void tinit() override; + bool tinit() override; bool tune_packet_context() override; bool tune_idle_context() override; + bool initialize(StreamModuleConfig&); + private: - bool tune_resources(unsigned work_limit) override; + bool tune_resources(unsigned work_limit); private: - FlowCacheConfig config; + StreamModuleConfig config; int max_flows_change = 0; };