From ecbf2b8a1f0a67903715d716668e62de326fe8bb Mon Sep 17 00:00:00 2001 From: Prakash Bailkeri Date: Sun, 12 Oct 2014 23:41:07 -0700 Subject: [PATCH] Fix concurrency issue in updating DBEntry flags Fixes bug: #1375226 Cause: DBEntry flag is updated from two mutually non-exclusive task. This results in corrupting the flags and unexpected free/delete on entry. The problem is caused while setting "OnRemoveQ" flag on DBEntry as it can be done from any task context. Fix: Move OnRemoveQ out of DBEntry flags and make it as atomic bool variable. Add assert to catch the case where route gets deleted with active paths Add assert to catch the case where path is inserted to a deleted route Added new test for repeated route update(from a agent with different nexthop) which discovered this bug. Change-Id: I5df04f89f00959799a921e9db37388c0eb56334e --- src/bgp/bgp_route.cc | 2 ++ src/bgp/test/bgp_xmpp_inetvpn_test.cc | 43 +++++++++++++++++++++++++++ src/db/db_entry.h | 15 +++++++--- 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/bgp/bgp_route.cc b/src/bgp/bgp_route.cc index 256f9db5fbe..a724c1e2451 100644 --- a/src/bgp/bgp_route.cc +++ b/src/bgp/bgp_route.cc @@ -24,6 +24,7 @@ BgpRoute::BgpRoute() { } BgpRoute::~BgpRoute() { + assert(GetPathList().empty()); } // @@ -38,6 +39,7 @@ const BgpPath *BgpRoute::BestPath() const { // Insert given path and redo path selection. // void BgpRoute::InsertPath(BgpPath *path) { + assert(!IsDeleted()); const Path *prev_front = front(); insert(path); diff --git a/src/bgp/test/bgp_xmpp_inetvpn_test.cc b/src/bgp/test/bgp_xmpp_inetvpn_test.cc index 014004935f5..436492bf349 100644 --- a/src/bgp/test/bgp_xmpp_inetvpn_test.cc +++ b/src/bgp/test/bgp_xmpp_inetvpn_test.cc @@ -272,6 +272,49 @@ TEST_F(BgpXmppInetvpn2ControlNodeTest, RouteChangeLocalPref) { agent_b_->SessionDown(); } +// +// Agent flaps a route repeatedly. +// +TEST_F(BgpXmppInetvpn2ControlNodeTest, RouteFlap) { + Configure(); + task_util::WaitForIdle(); + + // Create XMPP Agent A connected to XMPP server X. + agent_a_.reset( + new test::NetworkAgentMock(&evm_, "agent-a", xs_x_->GetPort(), + "127.0.0.1", "127.0.0.1")); + TASK_UTIL_EXPECT_TRUE(agent_a_->IsEstablished()); + + // Create XMPP Agent B connected to XMPP server Y. + agent_b_.reset( + new test::NetworkAgentMock(&evm_, "agent-b", xs_y_->GetPort(), + "127.0.0.2", "127.0.0.2")); + TASK_UTIL_EXPECT_TRUE(agent_b_->IsEstablished()); + + // Register to blue instance + agent_a_->Subscribe("blue", 1); + agent_b_->Subscribe("blue", 1); + + // Add route from agent A and change it repeatedly. + stringstream route_a; + route_a << "10.1.1.1/32"; + for (int idx = 0; idx < 1024; ++idx) { + agent_a_->AddRoute("blue", route_a.str(), "192.168.1.1"); + agent_a_->AddRoute("blue", route_a.str(), "192.168.1.2"); + } + + // Delete route from agent A. + agent_a_->DeleteRoute("blue", route_a.str()); + task_util::WaitForIdle(); + + // Verify that route is deleted at agents A and B. + VerifyRouteNoExists(agent_a_, "blue", route_a.str()); + VerifyRouteNoExists(agent_b_, "blue", route_a.str()); + + // Close the sessions. + agent_a_->SessionDown(); + agent_b_->SessionDown(); +} class TestEnvironment : public ::testing::Environment { virtual ~TestEnvironment() { } diff --git a/src/db/db_entry.h b/src/db/db_entry.h index 74747c15e2e..a1dfb28a3e1 100644 --- a/src/db/db_entry.h +++ b/src/db/db_entry.h @@ -7,6 +7,8 @@ #include +#include + #include "db/db_table.h" #include @@ -23,6 +25,7 @@ class DBEntryBase { typedef std::auto_ptr KeyPtr; DBEntryBase() : tpart_(NULL), flags(0), last_change_at_(UTCTimestampUsec()) { + onremoveq_ = false; } virtual ~DBEntryBase() { } virtual std::string ToString() const = 0; @@ -46,9 +49,13 @@ class DBEntryBase { void clear_onlist() { flags &= ~Onlist; } bool is_onlist() { return (flags & Onlist); } - void SetOnRemoveQ() { flags |= OnRemoveQ; } - void ClearOnRemoveQ() { flags &= ~OnRemoveQ; } - bool IsOnRemoveQ() { return (flags & OnRemoveQ); } + void SetOnRemoveQ() { + onremoveq_.fetch_and_store(true); + } + bool IsOnRemoveQ() { return (onremoveq_); } + void ClearOnRemoveQ() { + onremoveq_.fetch_and_store(false); + } //member hook in change list boost::intrusive::list_member_hook<> chg_list_; @@ -65,12 +72,12 @@ class DBEntryBase { enum DbEntryFlags { Onlist = 1 << 0, DeleteMarked = 1 << 1, - OnRemoveQ = 1 << 2, }; typedef std::map StateMap; DBTablePartBase *tpart_; StateMap state_; uint8_t flags; + tbb::atomic onremoveq_; uint64_t last_change_at_; // time at which entry was last 'changed' DISALLOW_COPY_AND_ASSIGN(DBEntryBase); };