Merge pull request #1846 in SNORT/snort3 from ~DAVMCPHE/snort3:stream_reload_tweaks...

Squashed commit of the following:

commit d4f864cf104f1cad64a800948461613e75fac1d4
Author: davis mcpherson <davmcphe@cisco.com>
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 bbd6793cdad04bc0ab64e211d66349207e7d2e25..72c45091586878881c9a14a8ed4a4facbff27f38 100644 (file)
--- 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 74d57b8bb37ebb03ac8534a1c6b269c33cdd7f50..f700326a7c74a03a11396586069cee8462c02ed2 100644 (file)
--- 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 750c09a4ace9c8c4cfba098e14723ad026dedf35..fc178a96f56f1972053d3db6530b124070e29411 100644 (file)
--- 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 bf358a0e533b9af4ff1f13e5c63f5451a1aeb991..ee9170afd498606fe6b343b398f2b87eff7d0a1d 100644 (file)
--- 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<ReloadResourceTuner*>(sc->get_reload_resource_tuners());
+                std::list<ReloadResourceTuner*>::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<ReloadResourceTuner*>*)*ac_state;

diff --git a/src/main/snort_config.h b/src/main/snort_config.h
index 9cb93dbeb9d8189e5b714943a1b8664e911a65d7..d51864526994f6cc24623ded233d22f78640b4ee 100644 (file)
--- 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 8e88a97c3427a530900764a5066586adbd4f235b..4eb969c7a7201bc21a74968b8eeb8617f3c29449 100644 (file)
--- 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 f2b6508b06645e1d08d993173811169d82f8c0df..c6d3e1128a6f9d0c2f6fd2583d86f1b61700d880 100644 (file)
--- 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;
 };