From e8035ffd935af2e49489111f97a0b56d84c5eba2 Mon Sep 17 00:00:00 2001 From: Prabhjot Singh Sethi Date: Mon, 9 May 2016 16:04:23 +0530 Subject: [PATCH] Fix ToR Agent crash in KSyncObject::NotifyEvent Issue: ------ while KSyncObject is processing delete table request, it expects KSync Entry tree to be manipulated by itself only however it possibly seems like there is a parallel access to KSync Entry Tree causing next pointer to be free'd before processing delete event on it. one such event was identified as intrusive pointer release on Task destructor which can be called in task scheduler context after completing the Run and can cause parallel access to KSync Entry Tree Fix: ---- on task complete reset intrusive pointer to ensure the reference management happens only in context of KSync task Also added Concurrency Check to assure client IDL sanity Closes-Bug: 1572287 Change-Id: I85594163231c2ba92afae1a833698e4c905c9b5d --- .../ovsdb_client/logical_switch_ovsdb.cc | 4 +++ .../ovsdb_client/ovsdb_client_idl.cc | 30 +++++++++++++++++++ .../ovsdb_client/ovsdb_client_idl.h | 4 +++ .../ovsdb_client/ovsdb_client_session.h | 3 ++ .../ovsdb_client/test/test_ovs_agent_init.h | 2 ++ 5 files changed, 43 insertions(+) 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 index fc7b8260792..270b162ae48 100644 --- 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 @@ -350,6 +350,10 @@ bool LogicalSwitchEntry::ProcessDeleteOvsReqTask::Run() { entry->TriggerDeleteAdd(); } + // on task completion reset intrusive pointer in context of task itself + // rather than processing it in the Task destructor, since in case of + // task completion task destructor will be called in context of scheduler + entry_ = NULL; return true; } diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc index 821d3db6152..4de4f4be1d2 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc @@ -287,6 +287,7 @@ bool OvsdbClientIdl::ProcessMessage(OvsdbMsg *msg) { struct ovsdb_idl_txn *OvsdbClientIdl::CreateTxn(OvsdbEntryBase *entry, KSyncEntry::KSyncEvent ack_event) { + assert(ConcurrencyCheck()); if (deleted_) { // Don't create new transactions for deleted idl. return NULL; @@ -317,6 +318,7 @@ struct ovsdb_idl_txn *OvsdbClientIdl::CreateTxn(OvsdbEntryBase *entry, struct ovsdb_idl_txn *OvsdbClientIdl::CreateBulkTxn(OvsdbEntryBase *entry, KSyncEntry::KSyncEvent ack_event) { + assert(ConcurrencyCheck()); if (deleted_) { // Don't create new transactions for deleted idl. return NULL; @@ -347,6 +349,7 @@ struct ovsdb_idl_txn *OvsdbClientIdl::CreateBulkTxn(OvsdbEntryBase *entry, bool OvsdbClientIdl::EncodeSendTxn(struct ovsdb_idl_txn *txn, OvsdbEntryBase *skip_entry) { + assert(ConcurrencyCheck()); // return false to wait for bulk txn to complete if (txn == bulk_txn_) { return false; @@ -374,6 +377,7 @@ bool OvsdbClientIdl::EncodeSendTxn(struct ovsdb_idl_txn *txn, } void OvsdbClientIdl::DeleteTxn(struct ovsdb_idl_txn *txn) { + assert(ConcurrencyCheck()); pending_txn_.erase(txn); // third party code and handle only one txn at a time, // if there is a pending bulk entry encode and send before @@ -577,6 +581,32 @@ uint64_t OvsdbClientIdl::pending_send_msg_count() const { return pending_send_msgs_.size(); } +bool OvsdbClientIdl::ConcurrencyCheck() const { + Task *current = Task::Running(); + static int ksync_task_id = -1; + static int db_task_id = -1; + + if (ksync_task_id == -1) + ksync_task_id = agent_->task_scheduler()->GetTaskId("Agent::KSync"); + + if (db_task_id == -1) + db_task_id = agent_->task_scheduler()->GetTaskId("db::DBTable"); + + if (current == NULL) { + return session_->TestConcurrencyAllow(); + } + + if (current->GetTaskId() == ksync_task_id) { + return true; + } + + if (current->GetTaskId() == db_task_id) { + return true; + } + + return false; +} + void OvsdbClientIdl::ConnectOperDB() { OVSDB_SESSION_TRACE(Trace, session_, "Received Monitor Response connecting to OperDb"); diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h index cbff436ea32..6435f7c233f 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h @@ -181,6 +181,10 @@ class OvsdbClientIdl { uint64_t pending_txn_count() const; uint64_t pending_send_msg_count() const; + // Concurrency Check to validate all idl transactions happen only in + // db::DBTable or Agent::KSync task context + bool ConcurrencyCheck() const; + private: friend void ovsdb_wrapper_idl_callback(void *, int, struct ovsdb_idl_row *); friend void ovsdb_wrapper_idl_txn_ack(void *, struct ovsdb_idl_txn *); diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_session.h b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_session.h index 98efbfb7750..66b6ae6590f 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_session.h +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_session.h @@ -56,6 +56,9 @@ class OvsdbClientSession { void AddSessionInfo(SandeshOvsdbClientSession &session); + // UT overrides this to allow concurrency check on NULL task + virtual bool TestConcurrencyAllow() { return false; } + protected: // ovsdb io task ID. static int ovsdb_io_task_id_; diff --git a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/test/test_ovs_agent_init.h b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/test/test_ovs_agent_init.h index 3130707fc3f..474081100ff 100644 --- a/src/vnsw/agent/ovs_tor_agent/ovsdb_client/test/test_ovs_agent_init.h +++ b/src/vnsw/agent/ovs_tor_agent/ovsdb_client/test/test_ovs_agent_init.h @@ -33,6 +33,8 @@ class OvsdbClientTcpSessionTest : public OvsdbClientTcpSession { TcpServer *server, Socket *sock, bool async_ready = true); virtual ~OvsdbClientTcpSessionTest(); + + virtual bool TestConcurrencyAllow() { return true; } }; class OvsdbClientTcpTest : public OvsdbClientTcp {