From b9305b495fd69810d0e7600a4ef13fdd8c404190 Mon Sep 17 00:00:00 2001 From: Praveen K V Date: Fri, 11 Mar 2016 20:47:19 +0530 Subject: [PATCH] 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 | 17 ++++ src/vnsw/agent/pkt/flow_mgmt.h | 5 + src/vnsw/agent/pkt/flow_mgmt_dbclient.cc | 40 +++++--- src/vnsw/agent/pkt/flow_mgmt_dbclient.h | 4 +- src/vnsw/agent/pkt/flow_mgmt_request.h | 22 +++-- src/vnsw/agent/pkt/flow_proto.cc | 4 + src/vnsw/agent/pkt/flow_proto.h | 2 + .../agent/pkt/test/test_flow_mgmt_route.cc | 95 ++++++++++++++++++- 8 files changed, 165 insertions(+), 24 deletions(-) diff --git a/src/vnsw/agent/pkt/flow_mgmt.cc b/src/vnsw/agent/pkt/flow_mgmt.cc index 01265bc0ce2..6b9f9e18a2a 100644 --- a/src/vnsw/agent/pkt/flow_mgmt.cc +++ b/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_ptrreq + (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 req) { break; } + case FlowMgmtRequest::DUMMY: + break; + default: assert(0); diff --git a/src/vnsw/agent/pkt/flow_mgmt.h b/src/vnsw/agent/pkt/flow_mgmt.h index 07036896957..036d8a315b1 100644 --- a/src/vnsw/agent/pkt/flow_mgmt.h +++ b/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); diff --git a/src/vnsw/agent/pkt/flow_mgmt_dbclient.cc b/src/vnsw/agent/pkt/flow_mgmt_dbclient.cc index 197cef4587a..86b9a7f8de0 100644 --- a/src/vnsw/agent/pkt/flow_mgmt_dbclient.cc +++ b/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 (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,6 +255,7 @@ void FlowMgmtDbClient::AclNotify(DBTablePartBase *part, DBEntryBase *e) { state = new AclFlowHandlerState(); e->SetState(part->parent(), acl_listener_id_, state); } + state->deleted_ = false; AddEvent(acl, state); } @@ -257,9 +263,6 @@ void FlowMgmtDbClient::AclNotify(DBTablePartBase *part, DBEntryBase *e) { // 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; diff --git a/src/vnsw/agent/pkt/flow_mgmt_dbclient.h b/src/vnsw/agent/pkt/flow_mgmt_dbclient.h index bbb1eecad49..27154845dda 100644 --- a/src/vnsw/agent/pkt/flow_mgmt_dbclient.h +++ b/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); diff --git a/src/vnsw/agent/pkt/flow_mgmt_request.h b/src/vnsw/agent/pkt/flow_mgmt_request.h index 8f2197504b3..2191d6f811b 100644 --- a/src/vnsw/agent/pkt/flow_mgmt_request.h +++ b/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(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() { } diff --git a/src/vnsw/agent/pkt/flow_proto.cc b/src/vnsw/agent/pkt/flow_proto.cc index e3f04132d84..faf6cc03659 100644 --- a/src/vnsw/agent/pkt/flow_proto.cc +++ b/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 ///////////////////////////////////////////////////////////////////////////// diff --git a/src/vnsw/agent/pkt/flow_proto.h b/src/vnsw/agent/pkt/flow_proto.h index 80ba4442700..1a127a80bf8 100644 --- a/src/vnsw/agent/pkt/flow_proto.h +++ b/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); diff --git a/src/vnsw/agent/pkt/test/test_flow_mgmt_route.cc b/src/vnsw/agent/pkt/test/test_flow_mgmt_route.cc index 486a9bb566f..1da4be4a9e1 100644 --- a/src/vnsw/agent/pkt/test/test_flow_mgmt_route.cc +++ b/src/vnsw/agent/pkt/test/test_flow_mgmt_route.cc @@ -9,6 +9,7 @@ #include "ksync/ksync_sock_user.h" #include "oper/tunnel_nh.h" #include "pkt/flow_mgmt.h" +#include "pkt/flow_mgmt_dbclient.h" #include #define vm1_ip "1.1.1.1" @@ -60,7 +61,7 @@ class FlowMgmtRouteTest : public ::testing::Test { FlushFlowTable(); client->Reset(); - DeleteVmportEnv(input, 3, true, 1); + DeleteVmportEnv(input, 2, true, 1); client->WaitForIdle(3); EXPECT_FALSE(VmPortFind(input, 0)); @@ -72,6 +73,38 @@ class FlowMgmtRouteTest : public ::testing::Test { DeleteBgpPeer(peer_); } + int CreateMpls(int tag) { + int label = agent_->mpls_table()->AllocLabel(); + MplsLabel::CreateVlanNh(agent_, label, MakeUuid(100), tag); + return label; + } + + void DeleteMpls(unsigned long label) { + MplsLabel::DeleteReq(agent_, label); + } + + void CreateTunnelNH(unsigned long addr) { + TunnelType::TypeBmap bmap = TunnelType::AllType(); + DBRequest req(DBRequest::DB_ENTRY_ADD_CHANGE); + req.key.reset(new TunnelNHKey(agent_->fabric_vrf_name(), + agent_->router_id(), + Ip4Address(addr), + true, TunnelType::ComputeType(bmap))); + req.data.reset(new TunnelNHData()); + Agent::GetInstance()->nexthop_table()->Enqueue(&req); + } + + void DeleteTunnelNH(unsigned long addr) { + TunnelType::TypeBmap bmap = TunnelType::AllType(); + DBRequest req(DBRequest::DB_ENTRY_DELETE); + req.key.reset(new TunnelNHKey(agent_->fabric_vrf_name(), + agent_->router_id(), + Ip4Address(addr), + true, TunnelType::ComputeType(bmap))); + req.data.reset(new TunnelNHData()); + Agent::GetInstance()->nexthop_table()->Enqueue(&req); + } + Agent *agent() {return agent_;} protected: @@ -274,6 +307,66 @@ TEST_F(FlowMgmtRouteTest, RouteDelete_5) { client->WaitForIdle(); } +//////////////////////////////////////////////////////////////////////////// +// UT for bug 1551577 +// Simulate the following scenario, +// 1. Delete a DBEntry and enqueue DELETE_DBENTRY event +// 2. Renew the DBEntry and enqueue ADD_DBENTRY +// 3. Delete the DBEntry and not notify DELETE +// 4. Process the DELETE_DBENTRY in (1) +// 5. Free DBState +// 6. Process the DELETE in (3) and let the DBEntry get freed +// 7. Process the ADD_DBENTRY from (2). +// Agent asserts since its trying to process deleted DBEntry +//////////////////////////////////////////////////////////////////////////// +TEST_F(FlowMgmtRouteTest, DB_Entry_Reuse) { +#define vm3_ip "1.1.1.3" + unsigned long addr = Ip4Address::from_string("100.100.100.1").to_ulong(); + + // Create TunnelNH + CreateTunnelNH(addr); + client->WaitForIdle(); + + // Create some MPLS Labels. They will be used to delay the DB-notification + // in later stages + ConcurrencyScope scope("db::DBTable"); + int label_list[2000]; + // Enqueue requests into flow-mgmt queue + for (int i = 100; i < 1000; i++) { + label_list[i] = CreateMpls(i); + } + client->WaitForIdle(); + WAIT_FOR(100, 1000, (flow_mgmt_->FlowUpdateQueueLength() == 0)); + + // Disable flow-management queue + flow_mgmt_->FlowUpdateQueueDisable(true); + + // Delete the NH. This should trigger clear of DBState + DeleteTunnelNH(addr); + client->WaitForIdle(); + + // Make dummy enqueues so that subsequent operation on tunnel-nh are + // delayed + for (int i = 0; i < 100000; i++) { + flow_mgmt_->DummyEvent(); + } + + // Revoke the tunnel-nh and delete it again + CreateTunnelNH(addr); + client->WaitForIdle(); + + DeleteTunnelNH(addr); + + // Delay notification tunnel-nh creation above to ensure the DBState + // for NH is cleared by flow-mgmt module + for (int i = 100; i < 1000; i++) { + DeleteMpls(label_list[i]); + } + + // Enable flow-mgmt queue + flow_mgmt_->FlowUpdateQueueDisable(false); + WAIT_FOR(1000, 1000, (flow_mgmt_->FlowUpdateQueueLength() == 0)); +} int main(int argc, char *argv[]) { int ret = 0;