Mike Christie wrote:
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?
Ok so now I am really lost :) See the attached patch. It looks like if
fc_seq_lookup_recip returns FC_RJT_NONE then from the fc_exch_find call
it returns from fc_seq_lookup_recip with a hold on the ep. So in
fc_exch_recv_req we need to just release it?
diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index c9161ee..948d2fc 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -665,6 +665,8 @@ static struct fc_exch *fc_exch_resp(struct fc_exch_mgr *mp,
struct fc_frame *fp)
/*
* Find a sequence for receive where the other end is originating the sequence.
+ * If fc_pf_rjt_reason is FC_RJT_NONE then this function will have a hold
+ * on the ep that should be released by the caller.
*/
static enum fc_pf_rjt_reason
fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame *fp)
@@ -693,9 +695,8 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame
*fp)
if (ep->rxid == FC_XID_UNKNOWN)
ep->rxid = ntohs(fh->fh_rx_id);
else if (ep->rxid != ntohs(fh->fh_rx_id)) {
- fc_exch_release(ep);
reject = FC_RJT_OX_ID;
- goto out;
+ goto rel;
}
} else {
xid = ntohs(fh->fh_rx_id); /* we are the responder */
@@ -717,10 +718,9 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct
fc_frame *fp)
ep = fc_exch_find(mp, xid);
if ((f_ctl & FC_FC_FIRST_SEQ) && fc_sof_is_init(fr_sof(fp))) {
if (ep) {
- fc_exch_release(ep);
atomic_inc(&mp->stats.xid_busy);
reject = FC_RJT_RX_ID;
- goto out;
+ goto rel;
}
ep = fc_exch_resp(mp, fp);
if (!ep) {
@@ -743,7 +743,7 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame
*fp)
sp = fc_seq_start_next(&ep->seq);
if (!sp) {
reject = FC_RJT_SEQ_XS; /* exchange shortage */
- goto out;
+ goto rel;
}
sp->id = fh->fh_seq_id;
sp->ssb_stat |= SSB_ST_RESP;
@@ -752,7 +752,7 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame
*fp)
if (sp->id != fh->fh_seq_id) {
atomic_inc(&mp->stats.seq_not_found);
reject = FC_RJT_SEQ_ID; /* sequence/exch should exist */
- goto out;
+ goto rel;
}
}
WARN_ON(ep != fc_seq_exch(sp));
@@ -763,6 +763,9 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame
*fp)
fr_seq(fp) = sp;
out:
return reject;
+rel:
+ fc_exch_release(ep);
+ return reject;
}
/*
@@ -1162,6 +1165,7 @@ static void fc_exch_recv_req(struct fc_lport *lp, struct
fc_exch_mgr *mp,
ep->resp(sp, fp, ep->resp_arg);
else
lp->tt.lport_recv(lp, sp, fp);
+ fc_exch_release(ep); /* release from lookup */
} else {
if (fc_exch_debug)
FC_DBG("exch/seq lookup failed: reject %x\n", reject);
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
