Mike Christie wrote: > Mike Christie wrote: >> Dev, Vasu wrote: >>>> -----Original Message----- >>>> From: Mike Christie [mailto:[EMAIL PROTECTED] >>>> Sent: Tuesday, August 26, 2008 6:24 PM >>>> To: Dev, Vasu >>>> Cc: [email protected] >>>> Subject: Re: [Open-FCoE] [PATCH 3/4] libfc: Added exch release >>>> fc_exch_mgr_delete_ep() >>>> >>>> Vasu Dev wrote: >>>>> This will ensure exch will get freed if exch ref dropped to zero >>>>> in fc_exch_mgr_delete_ep(). >>>>> struct fc_exch_mgr *mp; >>>>> >>>> If we can call fc_exch_mgr_delete_ep() while the last refcount is >>>> dropped then if the last fc_exch_release is done here before the hold >>>> then it will be freed when we call fc_exch_hold and we could be doing >>>> the hold on freed memory. >>>> >>>> I do not think we should be able to get to this point. >>> Correct, otherwise we are already accessing wrong memory in >>> fc_exch_mgr_delete_ep(). >>> >>>> Everyone calling >>>> it should have a valid ref (or if called from a resp handler calling a >>>> exch_done then the caller of the resp handler should have a refcount to >>>> the ep). If not then the ref counting is not right. >>>> >>> The exch ref is mostly correct except in fc_exch_recv_req(), the >>> fc_exch_recv_req() passed new exchage to upper layer without any >>> additional exch hold and release. So without this patch ep was not >>> freed from fc_exch_recv_req() when upper layer called fc_exc_done(). >>> I'm okay either way on this patch applied or not but now it is kind >>> of weired that ep is deleted from mp and along with mp->total_exches >>> is also updated, that means now mp->total_exches could be zero while >>> some ep are still around till their final fc_exch_release() >> >> >> Ah, I was thinking of them as two issues like you mentioned below. The >> refcounting is for just the lifetime management of the ep struct >> memory. Then calling exch_done/fc_exch_mgr_delete_ep manages the mp >> resources like the id reuse. >> >>> is called. I guess it may be okay since mp->total_exches is keeping >>> track of only exches in mp. So may be now we need another ep tracking >>> for its memory since ep memory was not freed in fc_exch_recv_req() >>> code paths and it went undetected though my unit test was hitting >>> fc_exch_recv_req() code paths. >> >> Do you mean you were running tests on that code path and then in some >> tests you ended up getting to fc_exch_mgr_free? >> >> I have hit the WARN_ON in fc_exch_mgr_free and errors in >> mempool_destroy or kmem_cache_destroy errors indicating there were >> still eps around somewhere. >> >> >>> I fixed exch ref in fc_exch_recv_req() path instead depending on this >>> patch in attached fixed-exch-rel.patch and similarly we always need to >>> ensure correct exch ref in other code paths without this patch. (no >>> intentional new line in this para, to test my ongoing mail server issue >>> causing paragraph appearing as long line without explicit new line) >> >> Path looked ok. > > I just had one question on that code path. > > fr_seq(fp) = NULL; > > If we set fr_seq to null here > > reject = fc_seq_lookup_recip(mp, fp); > if (reject == FC_RJT_NONE) { > sp = fr_seq(fp); /* sequence will be held */ > > Then here sp is NULL, right? > > ep = fc_seq_exch(sp); > > > So if we do a container_of on a NULL pointer what do we get? Is the ep > that is returned valid? Is ep->resp ever set in the bottom of the function? >
Oh nevermind I guess fc_seq_lookup_recip will always set a sp when returning reject. But in that case should fc_seq_lookup_recip be doing a hold from the fc_exch_find, that the caller of fc_seq_lookup_recip releases? Or if fc_exch_mgr_reset ran between the time fc_seq_lookup_recip drops the ref from the fc_exch_find and when fc_exch_recv_req runs, can fc_exch_mgr_reset free the ep we are trying to access before the added holds in your patch? _______________________________________________ devel mailing list [email protected] http://www.open-fcoe.org/mailman/listinfo/devel
