From f05357fec7410fab2316bba417f96d253ad10f47 Mon Sep 17 00:00:00 2001
From: Prabhjot Singh Sethi <prabhjot@juniper.net>
Date: Sat, 20 Aug 2016 00:08:14 +0530
Subject: [PATCH] Fix Vrouter Agent crash @ update flow handle

Issue:
------
Even though Hash id is part of the key for ksync flow
entry, we do allow change of hash id to -1 when other
ksync entry for the same flow ends up receiving same
flow handle to handle two entries with same key.
however if there was a pending response(Add/Change Ack)
from vrouter we will end up processing the ack as a
index allocation from value -1 to flow handle for
delete ksync entry. This can end up acquiring the
index from the active entry and cause invalid state
for ksync flow entry, resulting in crash.

Fix:
----
Save the last vrouter hash id (hash id used while
sending message to vrouter) and compare the ack/response
against this hash id to evaluate if response has a new
index allocation.

Conflicts:
	src/vnsw/agent/pkt/test/test_flow_mgr_instances.cc

Closes-Bug: 1613553
Change-Id: I21a12fbd5684de871eabf5cd6dd1568c0a9d6c52
(cherry picked from commit 2a743d785986f7c276927b59749c1acb28534c70)
---
 src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc |  6 ++++++
 src/vnsw/agent/vrouter/ksync/flowtable_ksync.h  |  7 +++++++
 .../vrouter/ksync/ksync_flow_index_manager.cc   | 17 ++++++++++-------
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc b/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc
index 4d9079f377f..7258634b197 100644
--- a/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc
+++ b/src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc
@@ -121,6 +121,7 @@ void FlowTableKSyncEntry::Reset() {
     gen_id_ = 0;
     evict_gen_id_ = 0;
     vrouter_gen_id_ = 0;
+    vrouter_hash_id_ = FlowEntry::kInvalidFlowHandle;
     old_reverse_flow_id_ = FlowEntry::kInvalidFlowHandle;
     old_action_ = 0;
     old_component_nh_idx_ = 0xFFFF;
@@ -404,6 +405,11 @@ bool FlowTableKSyncEntry::Sync() {
         changed = true;
     }
 
+    if (vrouter_hash_id_ != hash_id_) {
+        vrouter_hash_id_ = hash_id_;
+        changed = true;
+    }
+
     MirrorKSyncObject* obj = ksync_obj_->ksync()->mirror_ksync_obj();
     // Lookup for fist and second mirror entries
     std::vector<MirrorActionSpec>::const_iterator it;
diff --git a/src/vnsw/agent/vrouter/ksync/flowtable_ksync.h b/src/vnsw/agent/vrouter/ksync/flowtable_ksync.h
index 8bf896d4404..d85987282cd 100644
--- a/src/vnsw/agent/vrouter/ksync/flowtable_ksync.h
+++ b/src/vnsw/agent/vrouter/ksync/flowtable_ksync.h
@@ -81,6 +81,7 @@ class FlowTableKSyncEntry : public KSyncNetlinkEntry {
     uint8_t evict_gen_id() { return evict_gen_id_; }
     void set_evict_gen_id(uint8_t gen_id) { evict_gen_id_ = gen_id; }
     uint8_t vrouter_gen_id() { return vrouter_gen_id_; }
+    uint32_t vrouter_hash_id() const { return vrouter_hash_id_; }
     FlowEvent::Event last_event() const { return last_event_; }
     void ReleaseToken();
     void ResetKSyncResponseInfo() {
@@ -115,6 +116,12 @@ class FlowTableKSyncEntry : public KSyncNetlinkEntry {
     uint8_t gen_id_;  // contains the last propagated genid from flow module
     uint8_t evict_gen_id_;  // contains current active gen-id in vrouter
     uint8_t vrouter_gen_id_;  // Used to identify the last genid sent to vrouter
+
+    // used to identify last flow index sent to vrouter
+    // helps in knowing whether the vrouter response is index
+    // allocation or not
+    uint32_t vrouter_hash_id_;
+
     uint32_t hash_id_;
     uint32_t old_reverse_flow_id_;
     uint32_t old_action_;
diff --git a/src/vnsw/agent/vrouter/ksync/ksync_flow_index_manager.cc b/src/vnsw/agent/vrouter/ksync/ksync_flow_index_manager.cc
index 8f42f6fca15..113f8afe358 100644
--- a/src/vnsw/agent/vrouter/ksync/ksync_flow_index_manager.cc
+++ b/src/vnsw/agent/vrouter/ksync/ksync_flow_index_manager.cc
@@ -129,6 +129,16 @@ void KSyncFlowIndexManager::UpdateFlowHandle(FlowTableKSyncEntry *kentry,
                                              uint32_t index,
                                              uint8_t gen_id) {
     assert(index != FlowEntry::kInvalidFlowHandle);
+
+    // Check if index and gen id is corresponding to the info sent
+    // to vrouter, if vrouter has allocated flow index, following
+    // check will fail and it will follow through
+    // to process index allocation
+    if (kentry->vrouter_hash_id() == index &&
+        kentry->vrouter_gen_id() == gen_id) {
+        return;
+    }
+
     FlowEntry *flow = kentry->flow_entry().get();
     FlowTableKSyncObject *object = flow->flow_table()->ksync_object();
     uint8_t evict_gen_id;
@@ -137,13 +147,6 @@ void KSyncFlowIndexManager::UpdateFlowHandle(FlowTableKSyncEntry *kentry,
         // ksync entry only
         assert(kentry == flow->ksync_entry_);
         FlowEntry *rflow = flow->reverse_flow_entry();
-        // Check if index and gen id is corresponding to the info sent
-        // to vrouter, if vrouter has allocated flow index, following
-        // check will fail and it will follow through
-        if (kentry->hash_id() == index &&
-            kentry->vrouter_gen_id() == gen_id) {
-            return;
-        }
 
         // Index allocation should happen only if flow_handle is
         // kInvalidFlowHandle