From fbc88ab4cffa569b7af88d3f5b58f69661612786 Mon Sep 17 00:00:00 2001
From: Manish <manishs@juniper.net>
Date: Mon, 1 Aug 2016 12:22:52 +0530
Subject: [PATCH] Agent crash @ BgpAsAServiceFlowMgmtTree::GetCNIndex

Problem:
Flow is marked as short-flow and rflow is NULL. Update of this flow is called,
which results in extraction of keys is triggered. For Bgpaas control-node index
is extracted by referring to dest-ip in rflow. Here rflow is NULL and agent
crashes.

Solution:
If no rflow is present then dont extract any keys for Bgpaas. There is no info
to conclude on control-node and tree to be selected.
This is done only for add operation.
During deletion cn_index is stored and need not be re-computed from fabric side
flow.

Change-Id: I0df10ee49d3c0913587401c084ed092c34724fe7
Closes-bug: #1607868
(cherry picked from commit ddee34c0a136dfb8dc9dfad8cd7afb8a0afd292c)
---
 src/vnsw/agent/pkt/flow_mgmt.cc | 25 ++++++++++++++++---------
 src/vnsw/agent/pkt/flow_mgmt.h  |  7 +++++--
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/src/vnsw/agent/pkt/flow_mgmt.cc b/src/vnsw/agent/pkt/flow_mgmt.cc
index 5a829b898bc..9e162029163 100644
--- a/src/vnsw/agent/pkt/flow_mgmt.cc
+++ b/src/vnsw/agent/pkt/flow_mgmt.cc
@@ -39,7 +39,7 @@ FlowMgmtManager::FlowMgmtManager(Agent *agent, uint16_t table_index) :
     log_queue_.set_name("Flow Log Queue");
     for (uint8_t count = 0; count < MAX_XMPP_SERVERS; count++) {
         bgp_as_a_service_flow_mgmt_tree_[count].reset(
-            new BgpAsAServiceFlowMgmtTree(this));
+            new BgpAsAServiceFlowMgmtTree(this, count));
     }
 }
 
@@ -342,7 +342,7 @@ void BgpAsAServiceFlowMgmtTree::ExtractKeys(FlowEntry *flow,
     BgpAsAServiceFlowMgmtKey *key =
         new BgpAsAServiceFlowMgmtKey(vm_intf->GetUuid(),
                                  flow->bgp_as_a_service_port(),
-                                 BgpAsAServiceFlowMgmtTree::GetCNIndex(flow));
+                                 index_);
     AddFlowMgmtKey(tree, key);
 }
 
@@ -371,11 +371,14 @@ void BgpAsAServiceFlowMgmtTree::DeleteAll() {
     }
 }
 
-uint8_t BgpAsAServiceFlowMgmtTree::GetCNIndex(const FlowEntry *flow) {
+int BgpAsAServiceFlowMgmtTree::GetCNIndex(const FlowEntry *flow) {
     IpAddress dest_ip = IpAddress();
     if (flow->is_flags_set(FlowEntry::ReverseFlow)) {
         dest_ip = flow->key().src_addr;
     } else {
+        //No reverse flow means no CN to map to so dont add flow key.
+        if (flow->reverse_flow_entry() == NULL)
+            return BgpAsAServiceFlowMgmtTree::kInvalidCnIndex;
         dest_ip = flow->reverse_flow_entry()->key().src_addr;
     }
     for (uint8_t count = 0; count < MAX_XMPP_SERVERS; count++) {
@@ -384,7 +387,7 @@ uint8_t BgpAsAServiceFlowMgmtTree::GetCNIndex(const FlowEntry *flow) {
             return count;
         }
     }
-    return 0;
+    return BgpAsAServiceFlowMgmtTree::kInvalidCnIndex;
 }
 
 bool
@@ -545,9 +548,11 @@ void FlowMgmtManager::MakeFlowMgmtKeyTree(FlowEntry *flow,
     bridge_route_flow_mgmt_tree_.ExtractKeys(flow, tree);
     nh_flow_mgmt_tree_.ExtractKeys(flow, tree);
     if (flow->is_flags_set(FlowEntry::BgpRouterService)) {
-        uint8_t count = BgpAsAServiceFlowMgmtTree::GetCNIndex(flow);
-        bgp_as_a_service_flow_mgmt_tree_[count].get()->
-            ExtractKeys(flow, tree);
+        int cn_index = BgpAsAServiceFlowMgmtTree::GetCNIndex(flow);
+        if (cn_index != BgpAsAServiceFlowMgmtTree::kInvalidCnIndex) {
+            bgp_as_a_service_flow_mgmt_tree_[cn_index].get()->
+                ExtractKeys(flow, tree);
+        }
     }
 }
 
@@ -791,9 +796,11 @@ void FlowMgmtManager::AddFlowMgmtKey(FlowEntry *flow, FlowEntryInfo *info,
         break;
 
     case FlowMgmtKey::BGPASASERVICE: {
-        uint8_t count = BgpAsAServiceFlowMgmtTree::GetCNIndex(flow);
-        bgp_as_a_service_flow_mgmt_tree_[count].get()->Add(key, flow,
+        int cn_index = BgpAsAServiceFlowMgmtTree::GetCNIndex(flow);
+        if (cn_index != BgpAsAServiceFlowMgmtTree::kInvalidCnIndex) {
+            bgp_as_a_service_flow_mgmt_tree_[cn_index].get()->Add(key, flow,
                                                   (ret.second)? node : NULL);
+        }
         break;
     }
 
diff --git a/src/vnsw/agent/pkt/flow_mgmt.h b/src/vnsw/agent/pkt/flow_mgmt.h
index 11415d1c303..ae8d95d16cf 100644
--- a/src/vnsw/agent/pkt/flow_mgmt.h
+++ b/src/vnsw/agent/pkt/flow_mgmt.h
@@ -989,7 +989,9 @@ class BgpAsAServiceFlowMgmtEntry : public FlowMgmtEntry {
 
 class BgpAsAServiceFlowMgmtTree : public FlowMgmtTree {
 public:
-    BgpAsAServiceFlowMgmtTree(FlowMgmtManager *mgr) : FlowMgmtTree(mgr) {}
+    static const int kInvalidCnIndex = -1;
+    BgpAsAServiceFlowMgmtTree(FlowMgmtManager *mgr, int index) :
+        FlowMgmtTree(mgr), index_(index) {}
     virtual ~BgpAsAServiceFlowMgmtTree() {}
 
     void ExtractKeys(FlowEntry *flow, FlowMgmtKeyTree *tree);
@@ -998,10 +1000,11 @@ class BgpAsAServiceFlowMgmtTree : public FlowMgmtTree {
                              const FlowMgmtRequest *req);
     void DeleteAll();
     //Gets CN index from flow.
-    static uint8_t GetCNIndex(const FlowEntry *flow);
+    static int GetCNIndex(const FlowEntry *flow);
     // Called just before entry is deleted. Used to implement cleanup operations
     virtual void FreeNotify(FlowMgmtKey *key, uint32_t gen_id);
 private:
+    int index_;
     DISALLOW_COPY_AND_ASSIGN(BgpAsAServiceFlowMgmtTree);
 };