Skip to content

Commit

Permalink
Fix ToR Agent crash in KSyncObject::NotifyEvent
Browse files Browse the repository at this point in the history
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
(cherry picked from commit e8035ff)
  • Loading branch information
Prabhjot Singh Sethi committed May 9, 2016
1 parent 39ac2fe commit 87afc0a
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 0 deletions.
Expand Up @@ -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;
}

Expand Down
30 changes: 30 additions & 0 deletions src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.cc
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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");
Expand Down
4 changes: 4 additions & 0 deletions src/vnsw/agent/ovs_tor_agent/ovsdb_client/ovsdb_client_idl.h
Expand Up @@ -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 *);
Expand Down
Expand Up @@ -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_;
Expand Down
Expand Up @@ -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 {
Expand Down

0 comments on commit 87afc0a

Please sign in to comment.