[Open-FCoE] [PATCH] libfc: fixed some more possible fc_exch_reset() races (ver-2)

Vasu Dev vasu.dev at intel.com
Sat Sep 6 01:37:19 UTC 2008


Added exch lock for callers of exch_get()/fc_exch_alloc() to
prevent fc_exch_reset() from freeing exch while these callers
are still working on just allocated exch.

This change impacted fc_exch_resp() call flow also
since fc_exch_resp() allocats new exchange, this patch
made fc_exch_resp() similar to fc_exch_find()
in terms of exch ref counting by having additional exch
ref to caller of fc_exch_resp() before dropping exch lock
from exchange allocation. This would allow fc_exch_reset()
running in fc_exch_resp() callers without exch being freed
by fc_exch_reset() while caller of fc_exch_resp() are still
working on exch.

This patch also associated exch hold from fc_exch_alloc()
to exch pointer in mp and then called fc_exch_release()
after removing exch pointer in mp to drop this hold
in fc_exch_mgr_delete_ep(). This change eliminated need
for extra exch hold in some error cases such as error
handling case in fc_exch_seq_send() and also removed
exch ref decrement without test in fc_exch_done_locked().

Added check for lport state to not to add new ep while
local port is in reset state. However not sure on lport
locking for this, so left comment for this.

Signed-off-by: Vasu Dev <vasu.dev at intel.com>
---

 drivers/scsi/libfc/fc_exch.c |   55 +++++++++++++++++++++++++++---------------
 1 files changed, 35 insertions(+), 20 deletions(-)


diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index f307008..bcc68c0 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -322,7 +322,6 @@ static int fc_exch_done_locked(struct fc_exch *ep)
 		ep->state |= FC_EX_DONE;
 		if (del_timer(&ep->ex_timer))
 			atomic_dec(&ep->ex_refcnt); /* drop hold for timer */
-		atomic_dec(&ep->ex_refcnt); /* drop hold from alloc */
 		rc = 0;
 	}
 	return rc;
@@ -339,6 +338,7 @@ static void fc_exch_mgr_delete_ep(struct fc_exch *ep)
 	mp->exches[ep->xid - mp->min_xid] = NULL;
 	list_del(&ep->ex_list);
 	spin_unlock_bh(&mp->em_lock);
+	fc_exch_release(ep);	/* drop hold for exch in mp */
 }
 
 /*
@@ -490,6 +490,7 @@ static struct fc_seq *fc_seq_alloc(struct fc_exch *ep, u8 seq_id)
  *
  * if xid is supplied zero then assign next free exchange ID
  * from exchange manager, otherwise use supplied xid.
+ * Returns with exch lock held.
  */
 struct fc_exch *fc_exch_alloc(struct fc_exch_mgr *mp, u16 xid)
 {
@@ -538,16 +539,24 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr *mp, u16 xid)
 			xid = fc_exch_next_xid(mp, xid);
 		}
 
-		if (likely(mp->exches[xid - min_xid] == NULL)) {
-			mp->last_xid = xid;
-		} else {
-			spin_unlock_bh(&mp->em_lock);
-			atomic_inc(&mp->stats.no_free_exch_xid);
-			mempool_free(ep, mp->ep_pool);
-			goto out;
-		}
+		if (unlikely(mp->exches[xid - min_xid] != NULL))
+			goto err;
+		mp->last_xid = xid;
 	}
 
+	/*  lport lock ? */
+	if (mp->lp->state == LPORT_ST_RESET)
+		goto err;	/*  don't add new ep during local port reset  */
+
+	fc_exch_hold(ep);	/* hold for exch in mp */
+	spin_lock_init(&ep->ex_lock);
+	/*
+	 * Hold exch lock for caller to prevent fc_exch_reset()
+	 * from releasing exch  while fc_exch_alloc() caller is
+	 * still working on exch.
+	 */
+	spin_lock_bh(&ep->ex_lock);
+
 	mp->exches[xid - min_xid] = ep;
 	list_add_tail(&ep->ex_list, &mp->ex_list);
 	fc_seq_alloc(ep, ep->seq_id++);
@@ -563,13 +572,14 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr *mp, u16 xid)
 	ep->f_ctl = FC_FC_FIRST_SEQ;	/* next seq is first seq */
 	ep->rxid = FC_XID_UNKNOWN;
 	ep->class = mp->class;
-
-	spin_lock_init(&ep->ex_lock);
 	setup_timer(&ep->ex_timer, fc_exch_timeout, (unsigned long)ep);
-
-	fc_exch_hold(ep);	/* hold for caller */
 out:
 	return ep;
+err:
+	spin_unlock_bh(&mp->em_lock);
+	atomic_inc(&mp->stats.no_free_exch_xid);
+	mempool_free(ep, mp->ep_pool);
+	return NULL;
 }
 EXPORT_SYMBOL(fc_exch_alloc);
 
@@ -652,6 +662,8 @@ static struct fc_exch *fc_exch_resp(struct fc_exch_mgr *mp, struct fc_frame *fp)
 			WARN_ON(rxid != FC_XID_UNKNOWN);
 			fh->fh_rx_id = htons(ep->rxid);
 		}
+		fc_exch_hold(ep);	/* hold for caller */
+		spin_unlock_bh(&ep->ex_lock);	/* lock from exch_get */
 	}
 	return ep;
 }
@@ -665,7 +677,7 @@ static enum fc_pf_rjt_reason
 fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame *fp)
 {
 	struct fc_frame_header *fh = fc_frame_header_get(fp);
-	struct fc_exch *ep = NULL, *new_ep = NULL;
+	struct fc_exch *ep = NULL;
 	struct fc_seq *sp = NULL;
 	enum fc_pf_rjt_reason reject = FC_RJT_NONE;
 	u32 f_ctl;
@@ -715,7 +727,7 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame *fp)
 				reject = FC_RJT_RX_ID;
 				goto rel;
 			}
-			new_ep = ep = fc_exch_resp(mp, fp);
+			ep = fc_exch_resp(mp, fp);
 			if (!ep) {
 				reject = FC_RJT_EXCH_EST;	/* XXX */
 				goto out;
@@ -758,9 +770,8 @@ fc_seq_lookup_recip(struct fc_exch_mgr *mp, struct fc_frame *fp)
 out:
 	return reject;
 rel:
-	fc_exch_release(ep);
-	if (new_ep)
-		fc_exch_release(new_ep);
+	fc_exch_done(&ep->seq);
+	fc_exch_release(ep);	/* hold from fc_exch_find/fc_exch_resp */
 	return reject;
 }
 
@@ -1850,6 +1861,7 @@ struct fc_seq *fc_exch_seq_send(struct fc_lport *lp,
 	struct fc_seq *sp = NULL;
 	struct fc_frame_header *fh;
 	u16 fill;
+	int rc = 1;
 
 	ep = lp->tt.exch_get(lp, fp);
 	if (!ep) {
@@ -1895,7 +1907,6 @@ struct fc_seq *fc_exch_seq_send(struct fc_lport *lp,
 	if (unlikely(lp->tt.frame_send(lp, fp)))
 		goto err;
 
-	spin_lock_bh(&ep->ex_lock);
 	if (timer_msec)
 		fc_exch_timer_set_locked(ep, timer_msec);
 	sp->f_ctl = f_ctl;	/* save for possible abort */
@@ -1907,7 +1918,10 @@ struct fc_seq *fc_exch_seq_send(struct fc_lport *lp,
 	spin_unlock_bh(&ep->ex_lock);
 	return sp;
 err:
-	fc_exch_done(sp);
+	rc = fc_exch_done_locked(ep);
+	spin_unlock_bh(&ep->ex_lock);
+	if (!rc)
+		fc_exch_mgr_delete_ep(ep);
 	return NULL;
 }
 EXPORT_SYMBOL(fc_exch_seq_send);
@@ -1921,6 +1935,7 @@ void fc_exch_recv(struct fc_lport *lp, struct fc_exch_mgr *mp,
 	struct fc_frame_header *fh = fc_frame_header_get(fp);
 	u32 f_ctl;
 
+	/* lport lock ? */
 	if (!lp || !mp || (lp->state == LPORT_ST_NONE)) {
 		FC_DBG("fc_lport or EM is not allocated and configured");
 		fc_frame_free(fp);




More information about the devel mailing list