[Open-FCoE] [PATCH 1/2] libfc: fixed race in fc_exch_rrq()

Vasu Dev vasu.dev at intel.com
Fri Sep 5 18:28:52 UTC 2008


Removed storing of aborted_ep in rrq_ep since fc_exch_reset() can
occur on rrq_ep before aborted_ep is stored, in that case rrq_ep
will be a dangling reference and won't be good to store aborted_ep.
Therefore storing aborted_ep had racing issue and also this race would
have left aborted_ep un-freed if rrq_ep is freed before aborted_ep
could be stored in rrq_ep.

This patch removed storing of aborted_ep to fix this race and
instead used aborted_ep from passed ep argument to rrq resp handler,
so that rrq resp handler will certainly free the aborted_ep before
rrq ep is freed.

Also modified fc_exch_rrq() to finish sending rrq with exch lock
held to prevent fc_exch_reset() running while rrq send is in progess.
This will eliminate possibility of race with fc_exch_reset() and it
does saves extra exch locking and unlocking statements in
fc_exch_rrq() error cases.

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

 drivers/scsi/libfc/fc_exch.c |   27 +++++----------------------
 1 files changed, 5 insertions(+), 22 deletions(-)


diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
index 11a03bd..f307008 100644
--- a/drivers/scsi/libfc/fc_exch.c
+++ b/drivers/scsi/libfc/fc_exch.c
@@ -95,8 +95,6 @@ struct fc_exch {
 	u8		fh_type;	/* frame type */
 	enum fc_class	class;		/* class of service */
 	struct fc_seq	seq;		/* single sequence */
-	struct fc_exch	*aborted_ep;	/* ref to ep rrq is cleaning up */
-
 	/*
 	 * Handler for responses to this current exchange.
 	 */
@@ -441,9 +439,9 @@ static void fc_exch_timeout(unsigned long ep_arg)
 	e_stat = ep->esb_stat;
 	if (e_stat & ESB_ST_COMPLETE) {
 		ep->esb_stat = e_stat & ~ESB_ST_REC_QUAL;
-		spin_unlock_bh(&ep->ex_lock);
 		if (e_stat & ESB_ST_REC_QUAL)
 			fc_exch_rrq(ep);
+		spin_unlock_bh(&ep->ex_lock);
 		goto done;
 	} else {
 		resp = ep->resp;
@@ -1640,9 +1638,7 @@ reject:
  */
 static void fc_exch_rrq_resp(struct fc_seq *sp, struct fc_frame *fp, void *arg)
 {
-	struct fc_exch *ep = fc_seq_exch(sp);
-	struct fc_exch *aborted_ep;
-
+	struct fc_exch *aborted_ep = arg;
 	unsigned int op;
 
 	if (IS_ERR(fp)) {
@@ -1669,16 +1665,9 @@ static void fc_exch_rrq_resp(struct fc_seq *sp, struct fc_frame *fp, void *arg)
 	}
 
 cleanup:
-	spin_lock_bh(&ep->ex_lock);
-	aborted_ep = ep->aborted_ep;
-	ep->aborted_ep = NULL;
-	spin_unlock_bh(&ep->ex_lock);
-
-	if (aborted_ep) {
-		fc_exch_done(&aborted_ep->seq);
-		/* drop hold for rec qual */
-		fc_exch_release(aborted_ep);
-	}
+	fc_exch_done(&aborted_ep->seq);
+	/* drop hold for rec qual */
+	fc_exch_release(aborted_ep);
 }
 
 /*
@@ -1692,7 +1681,6 @@ static void fc_exch_rrq(struct fc_exch *ep)
 	struct fc_els_rrq *rrq;
 	struct fc_frame *fp;
 	struct fc_seq *rrq_sp;
-	struct fc_exch *rrq_ep;
 	u32 did;
 
 	lp = ep->lp;
@@ -1714,15 +1702,10 @@ static void fc_exch_rrq(struct fc_exch *ep)
 	rrq_sp = fc_exch_seq_send(lp, fp, fc_exch_rrq_resp, ep, lp->e_d_tov,
 				  lp->fid, did, FC_FC_SEQ_INIT | FC_FC_END_SEQ);
 	if (!rrq_sp) {
-		spin_lock_bh(&ep->ex_lock);
 		ep->esb_stat |= ESB_ST_REC_QUAL;
 		fc_exch_timer_set_locked(ep, ep->r_a_tov);
-		spin_unlock_bh(&ep->ex_lock);
 		return;
 	}
-
-	rrq_ep = fc_seq_exch(rrq_sp);
-	rrq_ep->aborted_ep = ep;
 }
 
 




More information about the devel mailing list