Fix access to DBEntry after its deleted

Problem:
When flow-mgmt-dbclient module gets FREE_DBENTRY event, the DBState was
freed under following conditions,
        - gen_id in the DBState matches
        - DBEntry was in deleted state
However, it misses following sequence of events
        1. DBEntry is deleted
           gen_id set to 1 and DELETE_DBENTRY event enqueued
           The event is not yet notified by flow-mgmt module
        2. DBEntry is added
           gen_id retained as 1 and ADD_DBENTRY event is enqueued
        3. DBEntry is deleted again. The DBEntry is not notified yet
        4. The DELETE_DBENTRY event is processed
           It will in-turn generate FREE_DBSTATE event
        5. The FREE_DBSTATE event is processed before DBEntry delete in
           (3) is processed
        6. The DBEntry delete is notified
           All clients remove their state even before event in (4) is
           processed
           DBEntry is freed
        7. Event from (4) is procssed and enqueues CLEAR_DBSTATE
           enqueued again
        8. CLEAR_DBSTATE event triggers ClearState for a freed-dbentry
           resulting in crash

Fix:
Maintain deleted_ flag in DBState and check for deleted_ flag before
freeing DBState.
With this change, the deleted_ flag will be reset in (2) and in (3), the
DBState will not be freed

Change-Id: I9d6617a4a7d74936f95372a00025e236684cf515
Closes-Bug: #1551577

src/vnsw/agent/pkt/flow_mgmt.cc

@@ -162,10 +162,24 @@ void FlowMgmtManager::RetryVrfDeleteEvent(const VrfEntry *vrf) {
     request_queue_.Enqueue(req);
 }
 
+void FlowMgmtManager::DummyEvent() {
+    boost::shared_ptr<FlowMgmtRequest>req
+        (new FlowMgmtRequest(FlowMgmtRequest::DUMMY));
+    request_queue_.Enqueue(req);
+}
+
 void FlowMgmtManager::EnqueueFlowEvent(FlowEvent *event) {
     agent_->pkt()->get_flow_proto()->EnqueueFlowEvent(event);
 }
 
+void FlowMgmtManager::FlowUpdateQueueDisable(bool disabled) {
+    request_queue_.set_disable(disabled);
+}
+
+size_t FlowMgmtManager::FlowUpdateQueueLength() {
+    return request_queue_.Length();
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // Handlers for events from the work-queue
 /////////////////////////////////////////////////////////////////////////////
@@ -377,6 +391,9 @@ bool FlowMgmtManager::RequestHandler(boost::shared_ptr<FlowMgmtRequest> req) {
         break;
     }
 
+    case FlowMgmtRequest::DUMMY:
+        break;
+
     default:
          assert(0);
 

src/vnsw/agent/pkt/flow_mgmt.h

@@ -1029,6 +1029,8 @@ class FlowMgmtManager {
     void DeleteEvent(const DBEntry *entry, uint32_t gen_id);
     void RetryVrfDeleteEvent(const VrfEntry *vrf);
     void RetryVrfDelete(uint32_t vrf_id);
+    // Dummy event used for testing
+    void DummyEvent();
 
     void VnFlowCounters(const VnEntry *vn,
                         uint32_t *ingress_flow_count,
@@ -1044,6 +1046,9 @@ class FlowMgmtManager {
                              uint32_t source_port);
     void EnqueueUveAddEvent(const FlowEntry *flow) const;
     void EnqueueUveDeleteEvent(const FlowEntry *flow) const;
+
+    void FlowUpdateQueueDisable(bool val);
+    size_t FlowUpdateQueueLength();
 private:
     // Handle Add/Change of a flow. Builds FlowMgmtKeyTree for all objects
     void AddFlow(FlowEntryPtr &flow);

src/vnsw/agent/pkt/flow_mgmt_dbclient.cc

@@ -48,6 +48,7 @@ void FlowMgmtDbClient::AddEvent(const DBEntry *entry, FlowMgmtState *state) {
 
 void FlowMgmtDbClient::DeleteEvent(const DBEntry *entry, FlowMgmtState *state) {
     state->gen_id_++;
+    state->deleted_ = true;
     mgr_->DeleteEvent(entry, state->gen_id_);
 }
 
@@ -66,6 +67,10 @@ static DBState *ValidateGenId(DBTableBase *table, DBEntry *entry,
     if (state == NULL)
         return NULL;
 
+    // If DBEntry is re-added in meanwhile, we do not want to free DBState
+    if (state->deleted_ == false)
+        return NULL;
+
     if (state->gen_id_ > gen_id)
         return NULL;
 
@@ -77,9 +82,6 @@ void FlowMgmtDbClient::FreeInterfaceState(Interface *intf, uint32_t gen_id) {
     if (vm_port == NULL)
         return;
 
-    if ((intf->IsDeleted() == false) && (vm_port->vn() != NULL))
-        return;
-
     DBState *state = ValidateGenId(intf->get_table(), intf,
                                    interface_listener_id_, gen_id);
     if (state == NULL)
@@ -100,7 +102,7 @@ void FlowMgmtDbClient::InterfaceNotify(DBTablePartBase *part, DBEntryBase *e) {
 
     VmIntfFlowHandlerState *state = static_cast<VmIntfFlowHandlerState *>
         (e->GetState(part->parent(), interface_listener_id_));
-    if (intf->IsDeleted() || new_vn == NULL) {
+    if (intf->IsDeleted()) {
         if (state) {
             DeleteEvent(vm_port, state);
         }
@@ -120,6 +122,11 @@ void FlowMgmtDbClient::InterfaceNotify(DBTablePartBase *part, DBEntryBase *e) {
         state->vrf_assign_acl_ = vm_port->vrf_assign_acl();
         changed = true;
     } else {
+        if (state->deleted_) {
+            state->deleted_ = false;
+            changed = true;
+        }
+
         if (state->vn_.get() != new_vn) {
             changed = true;
             state->vn_ = new_vn;
@@ -147,10 +154,6 @@ void FlowMgmtDbClient::InterfaceNotify(DBTablePartBase *part, DBEntryBase *e) {
 // VN notification handler
 ////////////////////////////////////////////////////////////////////////////
 void FlowMgmtDbClient::FreeVnState(VnEntry *vn, uint32_t gen_id) {
-    if (vn->IsDeleted() == false) {
-        return;
-    }
-
     DBState *state = ValidateGenId(vn->get_table(), vn, vn_listener_id_,
                                    gen_id);
     if (state == NULL)
@@ -213,6 +216,11 @@ void FlowMgmtDbClient::VnNotify(DBTablePartBase *part, DBEntryBase *e) {
         changed = true;
     }
 
+    if (state->deleted_) {
+        state->deleted_ = false;
+        changed = true;
+    }
+
     if (changed) {
         AddEvent(vn, state);
     }
@@ -222,9 +230,6 @@ void FlowMgmtDbClient::VnNotify(DBTablePartBase *part, DBEntryBase *e) {
 // ACL notification handler
 ////////////////////////////////////////////////////////////////////////////
 void FlowMgmtDbClient::FreeAclState(AclDBEntry *acl, uint32_t gen_id) {
-    if (acl->IsDeleted() == false)
-        return;
-
     DBState *state = ValidateGenId(acl->get_table(), acl, acl_listener_id_,
                                    gen_id);
     if (state == NULL)
@@ -250,16 +255,14 @@ void FlowMgmtDbClient::AclNotify(DBTablePartBase *part, DBEntryBase *e) {
         state = new AclFlowHandlerState();
         e->SetState(part->parent(), acl_listener_id_, state);
     }
+    state->deleted_ = false;
     AddEvent(acl, state);
 }
 
 ////////////////////////////////////////////////////////////////////////////
 // NH notification handler
 ////////////////////////////////////////////////////////////////////////////
 void FlowMgmtDbClient::FreeNhState(NextHop *nh, uint32_t gen_id) {
-    if (nh->IsDeleted() == false)
-        return;
-
     DBState *state = ValidateGenId(nh->get_table(), nh, nh_listener_id_,
                                    gen_id);
     if (state == NULL)
@@ -286,6 +289,7 @@ void FlowMgmtDbClient::NhNotify(DBTablePartBase *part, DBEntryBase *e) {
         state = new NhFlowHandlerState();
         nh->SetState(part->parent(), nh_listener_id_, state);
     }
+    state->deleted_ = false;
     AddEvent(nh, state);
     return;
 }
@@ -378,6 +382,7 @@ void FlowMgmtDbClient::VrfNotify(DBTablePartBase *part, DBEntryBase *e) {
         vrf->SetState(part->parent(), vrf_listener_id_, state);
         AddEvent(vrf, state);
     }
+    state->deleted_ = false;
     return;
 }
 
@@ -537,14 +542,19 @@ void FlowMgmtDbClient::RouteNotify(VrfFlowHandlerState *vrf_state,
     }
 
     bool new_route = false;
+    bool changed = false;
     if (state == NULL) {
         state  = new RouteFlowHandlerState();
         route->SetState(partition->parent(), id, state);
         AddEvent(route, state);
         new_route = true;
+    } else {
+        if (state->deleted_) {
+            state->deleted_ = false;
+            changed = true;
+        }
     }
 
-    bool changed = false;
     // Handle SG change
     if (state->sg_l_ != new_sg_l) {
         state->sg_l_ = new_sg_l;

src/vnsw/agent/pkt/flow_mgmt_dbclient.h

@@ -13,11 +13,12 @@ class EcmpLoadBalance;
 class FlowMgmtDbClient {
 public:
     struct FlowMgmtState : public DBState {
-        FlowMgmtState() : gen_id_(0) { }
+        FlowMgmtState() : gen_id_(0), deleted_(false) { }
         virtual ~FlowMgmtState() { }
 
         void IncrementGenId() { gen_id_++; }
         uint32_t gen_id_;
+        bool deleted_;
     };
 
     struct VnFlowHandlerState : public FlowMgmtState {
@@ -104,6 +105,7 @@ class FlowMgmtDbClient {
     bool FreeDBState(const DBEntry *entry, uint32_t gen_id);
 
 private:
+    friend class FlowMgmtRouteTest;
     void AddEvent(const DBEntry *entry, FlowMgmtState *state);
     void DeleteEvent(const DBEntry *entry, FlowMgmtState *state);
     void ChangeEvent(const DBEntry *entry, FlowMgmtState *state);

src/vnsw/agent/pkt/flow_mgmt_request.h

@@ -22,32 +22,40 @@ class FlowMgmtRequest {
         RETRY_DELETE_VRF,
         UPDATE_FLOW_INDEX,
         DELETE_BGP_AAS_FLOWS,
-        UPDATE_FLOW_STATS
+        UPDATE_FLOW_STATS,
+        DUMMY
+
     };
 
     FlowMgmtRequest(Event event, FlowEntryPtr &flow) :
-        event_(event), flow_(flow), db_entry_(NULL), vrf_id_(0) {
+        event_(event), flow_(flow), db_entry_(NULL), vrf_id_(0), gen_id_(),
+        bytes_(), packets_(), oflow_bytes_() {
             if (event == RETRY_DELETE_VRF)
                 assert(vrf_id_);
-        }
+    }
 
     FlowMgmtRequest(Event event, FlowEntryPtr &flow, uint32_t bytes,
                     uint32_t packets, uint32_t oflow_bytes) :
-        event_(event), flow_(flow), db_entry_(NULL), vrf_id_(0),
+        event_(event), flow_(flow), db_entry_(NULL), vrf_id_(0), gen_id_(),
         bytes_(bytes), packets_(packets), oflow_bytes_(oflow_bytes) {
             if (event == RETRY_DELETE_VRF)
                 assert(vrf_id_);
-        }
+    }
 
     FlowMgmtRequest(Event event, const DBEntry *db_entry, uint32_t gen_id) :
         event_(event), flow_(NULL), db_entry_(db_entry), vrf_id_(0),
-        gen_id_(gen_id) {
+        gen_id_(gen_id), bytes_(), packets_(), oflow_bytes_() {
             if (event == RETRY_DELETE_VRF) {
                 const VrfEntry *vrf = dynamic_cast<const VrfEntry *>(db_entry);
                 assert(vrf);
                 vrf_id_ = vrf->vrf_id();
             }
-        }
+    }
+
+    FlowMgmtRequest(Event event) :
+        event_(event), flow_(NULL), db_entry_(NULL), vrf_id_(),
+        gen_id_(), bytes_(), packets_(), oflow_bytes_() {
+    }
 
     virtual ~FlowMgmtRequest() { }
 

src/vnsw/agent/pkt/flow_proto.cc

@@ -136,6 +136,10 @@ void FlowProto::DisableFlowMgmtQueue(bool disabled) {
     flow_update_queue_.set_disable(disabled);
 }
 
+size_t FlowProto::FlowMgmtQueueLength() {
+    return flow_update_queue_.Length();
+}
+
 /////////////////////////////////////////////////////////////////////////////
 // FlowTable related routines
 /////////////////////////////////////////////////////////////////////////////

src/vnsw/agent/pkt/flow_proto.h

@@ -79,6 +79,8 @@ class FlowProto : public Proto {
 
     void DisableFlowEventQueue(uint32_t index, bool disabled);
     void DisableFlowMgmtQueue(bool disabled);
+    size_t FlowMgmtQueueLength();
+
     const FlowStats *flow_stats() const { return &stats_; }
 
     void SetProfileData(ProfileData *data);