[Open-FCoE] [PATCH 2/2] libfc: fixed some more possiblefc_exch_reset() races

Dev, Vasu vasu.dev at intel.com
Sat Sep 6 01:43:00 UTC 2008


Please ignore this patch instead review ver-2 of this
patch
http://www.open-fcoe.org/pipermail/devel/2008-September/000700.html. 

In ver-2, I change exch lock location in fc_exch_alloc() inside 
mp lock since fc_exch_reset() can occur as soon as mp lock released in
this
function and also added check for lport reset state to prevent adding
new
ep in this state.

	--Vasu

>-----Original Message-----
>From: devel-bounces at open-fcoe.org [mailto:devel-bounces at open-fcoe.org]
On
>Behalf Of Vasu Dev
>Sent: Friday, September 05, 2008 11:29 AM
>To: devel at open-fcoe.org
>Subject: [Open-FCoE] [PATCH 2/2] libfc: fixed some more
>possiblefc_exch_reset() races
>
>Added exch lock for callers of exch_get()/fc_exch_alloc() to
>prevent fc_exch_reset() from freeing exch while these callers
>are is 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().
>
>Signed-off-by: Vasu Dev <vasu.dev at intel.com>
>---
>
> drivers/scsi/libfc/fc_exch.c |   28 +++++++++++++++++++---------
> 1 files changed, 19 insertions(+), 9 deletions(-)
>
>
>diff --git a/drivers/scsi/libfc/fc_exch.c
b/drivers/scsi/libfc/fc_exch.c
>index f307008..3a630c5 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 */
> }
>
> /*
>@@ -553,6 +553,7 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr
*mp,
>u16 xid)
> 	fc_seq_alloc(ep, ep->seq_id++);
> 	mp->total_exches++;
> 	spin_unlock_bh(&mp->em_lock);
>+	fc_exch_hold(ep);	/* hold for exch in mp */
>
> 	/*
> 	 *  update exchange
>@@ -567,7 +568,12 @@ struct fc_exch *fc_exch_alloc(struct fc_exch_mgr
*mp,
>u16 xid)
> 	spin_lock_init(&ep->ex_lock);
> 	setup_timer(&ep->ex_timer, fc_exch_timeout, (unsigned long)ep);
>
>-	fc_exch_hold(ep);	/* hold for caller */
>+	/*
>+	 * Hold exch lock for caller to prevent
>+	 * fc_exch_reset() from releasing exch
>+	 * while caller is still working on exch.
>+	 */
>+	spin_lock_bh(&ep->ex_lock);
> out:
> 	return ep;
> }
>@@ -652,6 +658,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 +673,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 +723,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 +766,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 +1857,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 +1903,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 +1914,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);
>
>_______________________________________________
>devel mailing list
>devel at open-fcoe.org
>http://www.open-fcoe.org/mailman/listinfo/devel



More information about the devel mailing list