Skip to content

Commit

Permalink
Fix Vrouter Agent crash @ update flow handle
Browse files Browse the repository at this point in the history
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 2a743d7)
  • Loading branch information
Prabhjot Singh Sethi committed Aug 22, 2016
1 parent 9416391 commit f05357f
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 7 deletions.
6 changes: 6 additions & 0 deletions src/vnsw/agent/vrouter/ksync/flowtable_ksync.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions src/vnsw/agent/vrouter/ksync/flowtable_ksync.h
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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_;
Expand Down
17 changes: 10 additions & 7 deletions src/vnsw/agent/vrouter/ksync/ksync_flow_index_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down

0 comments on commit f05357f

Please sign in to comment.