Hi Vu, Ack, code review only not tested
Thanks, Ravi -----Original Message----- From: Vu Minh Nguyen [mailto:[email protected]] Sent: Friday, March 16, 2018 2:51 PM To: [email protected]; [email protected]; [email protected]; [email protected]; [email protected] Cc: [email protected]; Vu Minh Nguyen <[email protected]> Subject: [PATCH 1/1] imm: fix race-condition in imm agent [#2810] IMM application gets coredump during upgrade due to failed assertion in IMMA library. There was a race condition b/w IMMA internal thread (MDS thread) and IMM application thread (IMM dispatching). When a CCB was aborted from IMMND side, IMMA internal thread (MDS) did some changes on the event IMMA_EVT_ND2A_OI_CCB_ABORT_UC. If IMM application was dispatching on any IMM event during that time, race-condition on ccb record database could happen. This patch adds lock/unlock to ensure no race happen and remove the assertion on `isAborted` at the nearly end of saImmOiAugmentCcbInitialize's work. --- src/imm/agent/imma_db.cc | 14 +++++++++++--- src/imm/agent/imma_oi_api.cc | 12 ++++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/imm/agent/imma_db.cc b/src/imm/agent/imma_db.cc index d0e3682..071edbe 100644 --- a/src/imm/agent/imma_db.cc +++ b/src/imm/agent/imma_db.cc @@ -533,11 +533,19 @@ void imma_oi_ccb_record_augment(IMMA_CLIENT_NODE *cl_node, SaImmOiCcbIdT ccbId, SaImmHandleT privateOmHandle, SaImmAdminOwnerHandleT privateAoHandle) { TRACE_ENTER(); - struct imma_oi_ccb_record *tmp = imma_oi_ccb_record_find(cl_node, ccbId); - osafassert(tmp && tmp->isCcbAugOk); + struct imma_oi_ccb_record *tmp = imma_oi_ccb_record_find(cl_node, + ccbId); osafassert(tmp); // Perform saImmOiAugmentCcbInitialize() on + just-aborted CCB, // but since most of OiAugmentCcbInitialize's work + have been finished, // we don't interrupt the on-going work such as + change error code, finalize // private om handle, etc, they will be + handled when IMM app dispatches // CCB abort callback event. + if (tmp->isAborted) { + TRACE_1("Abort upcall received by mds thread on this CCB 0x%llx", + ccbId); } - osafassert(!(tmp->isAborted)); + osafassert(tmp->isCcbAugOk); osafassert(!(tmp->isCritical)); diff --git a/src/imm/agent/imma_oi_api.cc b/src/imm/agent/imma_oi_api.cc index 28cea8a..29fb39d 100644 --- a/src/imm/agent/imma_oi_api.cc +++ b/src/imm/agent/imma_oi_api.cc @@ -4077,12 +4077,20 @@ done: } if (rc == SA_AIS_OK || rc == SA_AIS_ERR_TRY_AGAIN) { - /* mark oi_ccb_record with privateOmHandle to avoid repeated open/close + /* Mark oi_ccb_record with privateOmHandle to avoid repeated + open/close of private-om-handle for each try again or each ccb op. The handle is closed when the ccb is terminated (apply-uc or abort-uc). - */ + + And the CCB record could be changed in MDS thread if CCB is aborted. + Lock/unlock is here to ensure no race-condition b/w the MDS thread + and IMM application thread. */ + m_NCS_LOCK(&cb->cb_lock, NCS_LOCK_WRITE); + imma_oi_ccb_record_augment(cl_node, ccbId, privateOmHandle, privateAoHandle); + + m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE); + if (privateAoHandle) { *ownerHandle = privateAoHandle; } -- 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
