Hi Vu, Ack. I have not tested
I have written a comment but this is not a remark saying that anything has to be changed. One thing I would recommend is to make one more check to see if lock is set in all places where the protected variables are handled. I checked and found some place where I am not sure that's the case. This lock is however problematic and is used with way too big sections containing functions calling functions... etc. so it is hard to be sure by just reading the code. Thanks Lennart > -----Original Message----- > From: Vu Minh Nguyen [mailto:[email protected]] > Sent: den 16 mars 2018 10:21 > To: [email protected]; Hans Nordebäck > <[email protected]>; Zoran Milinkovic > <[email protected]>; Anders Widell > <[email protected]>; Lennart Lund <[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); + // [Lennart] Note: Just some reflections. This is not a remark + // It is good practice to avoid calling functions within a + // section protected by mutex. It is a big risk that the scope of the + // protected section becomes to big. It is better to make the function + // thread safe (or group of functions handling common variables) when + // possible. In this case nothing except calling the function is done in + // this protected section. This is also the only place where this function + // is called. + // However in this case it is correct both to use this cb_lock and + // use it outside of the function since other functions handling the same + // variables are called within other locked sections using this lock. + // + // I have taken a quick look and I think I have found places where cl_node + // data is handled without using the cb_lock. Maybe you should check that + // no more locking is missing?> + > 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
