Merge "Fix ToR agent crash in KSyncObjectManager::Process" into R3.0

Juniper · May 20, 2016 · 26af87a · 26af87a
2 parents 2218ba5 + 4f04f87
commit 26af87a
Show file tree

Hide file tree

Showing 9 changed files with 29 additions and 20 deletions.
diff --git a/src/ksync/ksync_object.cc b/src/ksync/ksync_object.cc
@@ -109,6 +109,7 @@ KSyncEntry *KSyncObject::Find(const KSyncEntry *key) {
 }
 
 KSyncEntry *KSyncObject::Next(const KSyncEntry *entry) const {
+    tbb::recursive_mutex::scoped_lock lock(lock_);
     Tree::const_iterator it;
     if (entry == NULL) {
         it = tree_.begin();
@@ -1524,7 +1525,8 @@ bool KSyncObjectManager::Process(KSyncObjectEvent *event) {
     case KSyncObjectEvent::DELETE:
         {
             int count = 0;
-            KSyncEntry *entry;
+            // hold reference to entry to ensure the pointer sanity
+            KSyncEntry::KSyncEntryPtr entry(NULL);
             if (event->ref_.get() == NULL) {
                 event->obj_->set_delete_scheduled();
                 if (event->obj_->IsEmpty()) {
@@ -1539,23 +1541,30 @@ bool KSyncObjectManager::Process(KSyncObjectEvent *event) {
                 entry = event->ref_.get();
             }
 
-            while (entry != NULL) {
-                KSyncEntry *next_entry = event->obj_->Next(entry);
+            // hold reference to entry to ensure the pointer sanity
+            // next entry can get free'd in certain cases while processing
+            // current entry
+            KSyncEntry::KSyncEntryPtr next_entry(NULL);
+            while (entry.get() != NULL) {
+                next_entry = event->obj_->Next(entry.get());
                 count++;
                 if (entry->IsDeleted() == false) {
                     // trigger delete if entry is not marked delete already.
-                    event->obj_->Delete(entry);
+                    event->obj_->Delete(entry.get());
                 }
 
-                if (count == kMaxEntriesProcess && next_entry != NULL) {
+                if (count == kMaxEntriesProcess && next_entry.get() != NULL) {
                     // update reference with which entry to start with
                     // in next iteration.
-                    event->ref_ = next_entry;
+                    event->ref_ = next_entry.get();
                     // yeild and re-enqueue event for processing later.
                     event_queue_->Enqueue(event);
+
+                    // release reference to deleted entry.
+                    entry = NULL;
                     return false;
                 }
-                entry = next_entry;
+                entry = next_entry.get();
             }
             break;
         }

diff --git a/src/ksync/ksync_object.h b/src/ksync/ksync_object.h
@@ -153,7 +153,7 @@ class KSyncObject {
     void ClearStale(KSyncEntry *entry);
     // Big lock on the tree
     // TODO: Make this more fine granular
-    tbb::recursive_mutex  lock_;
+    mutable tbb::recursive_mutex  lock_;
     void ChangeKey(KSyncEntry *entry, uint32_t arg);
     virtual void UpdateKey(KSyncEntry *entry, uint32_t arg) { }
 

diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ha_stale_dev_vn.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ha_stale_dev_vn.cc
@@ -141,10 +141,10 @@ void HaStaleDevVnEntry::TriggerAck(HaStaleL2RouteTable *table) {
     assert(old_l2_table_ == table);
     if (l2_table_ != NULL) {
         old_l2_table_ = NULL;
-        object->NotifyEvent(this, KSyncEntry::ADD_ACK);
+        object->SafeNotifyEvent(this, KSyncEntry::ADD_ACK);
     } else {
         old_l2_table_ = NULL;
-        object->NotifyEvent(this, KSyncEntry::DEL_ACK);
+        object->SafeNotifyEvent(this, KSyncEntry::DEL_ACK);
     }
 }
 

diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/logical_switch_ovsdb.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/logical_switch_ovsdb.cc
@@ -147,7 +147,7 @@ void LogicalSwitchEntry::DeleteMsg(struct ovsdb_idl_txn *txn) {
 
 void LogicalSwitchEntry::OvsdbChange() {
     if (!IsResolved())
-        table_->NotifyEvent(this, KSyncEntry::ADD_CHANGE_REQ);
+        table_->SafeNotifyEvent(this, KSyncEntry::ADD_CHANGE_REQ);
 }
 
 const std::string &LogicalSwitchEntry::name() const {

diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/multicast_mac_local_ovsdb.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/multicast_mac_local_ovsdb.cc
@@ -62,7 +62,7 @@ void MulticastMacLocalEntry::EvaluateVrfDependency(VrfEntry *vrf) {
         OnVrfDelete();
         // Issue a ADD_CHANGE_REQ to push entry into defer state.
         // Whenever VN entry is back this will be updated.
-        table_->NotifyEvent(this, KSyncEntry::ADD_CHANGE_REQ);
+        table_->SafeNotifyEvent(this, KSyncEntry::ADD_CHANGE_REQ);
     }
 }
 

diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_entry.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_entry.cc
@@ -47,7 +47,7 @@ void OvsdbEntry::Ack(bool success) {
     }
 
     OvsdbObject *object = static_cast<OvsdbObject*>(GetObject());
-    object->NotifyEvent(this, ack_event_);
+    object->SafeNotifyEvent(this, ack_event_);
 }
 
 OvsdbDBEntry::OvsdbDBEntry(OvsdbDBObject *table) : KSyncDBEntry(), table_(table),
@@ -206,9 +206,9 @@ void OvsdbDBEntry::Ack(bool success) {
 
     if (success) {
         if (IsDelAckWaiting())
-            object->NotifyEvent(this, KSyncEntry::DEL_ACK);
+            object->SafeNotifyEvent(this, KSyncEntry::DEL_ACK);
         else if (IsAddChangeAckWaiting())
-            object->NotifyEvent(this, KSyncEntry::ADD_ACK);
+            object->SafeNotifyEvent(this, KSyncEntry::ADD_ACK);
         else
             delete this;
     } else {
@@ -225,7 +225,7 @@ void OvsdbDBEntry::Ack(bool success) {
             }
 
             if (trigger_ack) {
-                object->NotifyEvent(this, KSyncEntry::DEL_ACK);
+                object->SafeNotifyEvent(this, KSyncEntry::DEL_ACK);
             }
         } else if (IsAddChangeAckWaiting()) {
             OVSDB_TRACE(Error, "Add Transaction failed for " + ToString());
@@ -236,7 +236,7 @@ void OvsdbDBEntry::Ack(bool success) {
                 object->Change(this);
             }
 
-            object->NotifyEvent(this, KSyncEntry::ADD_ACK);
+            object->SafeNotifyEvent(this, KSyncEntry::ADD_ACK);
         } else {
             // should never happen
             assert(0);

diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_object.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_object.cc
@@ -128,7 +128,7 @@ void OvsdbDBObject::NotifyDeleteOvsdb(OvsdbDBEntry *key,
             // we were not waiting for delete to happen, state for
             // OVSDB server and client mismatch happend.
             // Trigger Add/Change Req on entry to resync
-            NotifyEvent(entry, KSyncEntry::ADD_CHANGE_REQ);
+            SafeNotifyEvent(entry, KSyncEntry::ADD_CHANGE_REQ);
         }
     }
 }

diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_remote_ovsdb.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/unicast_mac_remote_ovsdb.cc
@@ -167,7 +167,7 @@ void UnicastMacRemoteEntry::DeleteMsg(struct ovsdb_idl_txn *txn) {
 
 void UnicastMacRemoteEntry::OvsdbChange() {
     if (!IsResolved())
-        table_->NotifyEvent(this, KSyncEntry::ADD_CHANGE_REQ);
+        table_->SafeNotifyEvent(this, KSyncEntry::ADD_CHANGE_REQ);
 }
 
 bool UnicastMacRemoteEntry::Sync(DBEntry *db_entry) {

diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vrf_ovsdb.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/vrf_ovsdb.cc
@@ -100,7 +100,7 @@ void VrfOvsdbEntry::TriggerAck(UnicastMacRemoteTable *table) {
     OvsdbDBObject *object = static_cast<OvsdbDBObject*>(GetObject());
     assert(route_table_ == table);
     route_table_ = NULL;
-    object->NotifyEvent(this, KSyncEntry::DEL_ACK);
+    object->SafeNotifyEvent(this, KSyncEntry::DEL_ACK);
 }
 
 VrfOvsdbObject::VrfOvsdbObject(OvsdbClientIdl *idl) : OvsdbDBObject(idl, true) {