Attachment 653689 Details for Bug 822088 – [PATCH] libfc: extend ex_lock to protect all of fc_seq_send

[patch] [PATCH] libfc: extend ex_lock to protect all of fc_seq_send

-libfc-extend-ex_lock-to-protect-all-of-fc_seq_send.patch (text/plain), 5.54 KB, created by Neil Horman on 2012-11-28 19:13:17 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Neil Horman

Created: 2012-11-28 19:13:17 UTC

Size: 5.54 KB

patch

obsolete

>From 27d3d65be8c665b75d8c1bd1481b01f76838e0a8 Mon Sep 17 00:00:00 2001
>From: Neil Horman <nhorman@redhat.com>
>Date: Tue, 27 Nov 2012 13:14:31 -0500
>Subject: [RHEL 6.4 PATCH] libfc: extend ex_lock to protect all of fc_seq_send
>
>This warning was reported recently:
>
>WARNING: at drivers/scsi/libfc/fc_exch.c:478 fc_seq_send+0x14f/0x160 [libfc]()
>(Not tainted)
>Hardware name: ProLiant DL120 G7
>Modules linked in: tcm_fc target_core_iblock target_core_file target_core_pscsi
>target_core_mod configfs dm_round_robin dm_multipath 8021q garp stp llc bnx2fc
>cnic uio fcoe libfcoe libfc scsi_transport_fc scsi_tgt autofs4 sunrpc
>pcc_cpufreq ipv6 hpilo hpwdt e1000e microcode iTCO_wdt iTCO_vendor_support
>serio_raw shpchp ixgbe dca mdio sg ext4 mbcache jbd2 sd_mod crc_t10dif pata_acpi
>ata_generic ata_piix hpsa dm_mirror dm_region_hash dm_log dm_mod [last unloaded:
>scsi_wait_scan]
>Pid: 5464, comm: target_completi Not tainted 2.6.32-272.el6.x86_64 #1
>Call Trace:
> [<ffffffff8106b747>] ? warn_slowpath_common+0x87/0xc0
> [<ffffffff8106b79a>] ? warn_slowpath_null+0x1a/0x20
> [<ffffffffa025f7df>] ? fc_seq_send+0x14f/0x160 [libfc]
> [<ffffffffa035cbce>] ? ft_queue_status+0x16e/0x210 [tcm_fc]
> [<ffffffffa030a660>] ? target_complete_ok_work+0x0/0x4b0 [target_core_mod]
> [<ffffffffa030a766>] ? target_complete_ok_work+0x106/0x4b0 [target_core_mod]
> [<ffffffffa030a660>] ? target_complete_ok_work+0x0/0x4b0 [target_core_mod]
> [<ffffffff8108c760>] ? worker_thread+0x170/0x2a0
> [<ffffffff810920d0>] ? autoremove_wake_function+0x0/0x40
> [<ffffffff8108c5f0>] ? worker_thread+0x0/0x2a0
> [<ffffffff81091d66>] ? kthread+0x96/0xa0
> [<ffffffff8100c14a>] ? child_rip+0xa/0x20
> [<ffffffff81091cd0>] ? kthread+0x0/0xa0
> [<ffffffff8100c140>] ? child_rip+0x0/0x20
>
>It occurs because fc_seq_send can have multiple contexts executing within it at
>the same time, and fc_seq_send doesn't consistently use the ep->ex_lock that
>protects this structure.  Because of that, its possible for one context to clear
>the INIT bit in the ep->esb_state field while another checks it, leading to the
>above stack trace generated by the WARN_ON in the function.
>
>We should probably undertake the effort to convert access to the fc_exch
>structures to use rcu, but that a larger work item.  To just fix this specific
>issue, we can just extend the ex_lock protection through the entire fc_seq_send
>path
>
>Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>---
> drivers/scsi/libfc/fc_exch.c | 39 +++++++++++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/scsi/libfc/fc_exch.c b/drivers/scsi/libfc/fc_exch.c
>index 4a5a47e..e1dc21a 100644
>--- a/drivers/scsi/libfc/fc_exch.c
>+++ b/drivers/scsi/libfc/fc_exch.c
>@@ -459,13 +459,7 @@ static void fc_exch_timer_set(struct fc_exch *ep, unsigned int timer_msec)
> 	spin_unlock_bh(&ep->ex_lock);
> }
> 
>-/**
>- * fc_seq_send() - Send a frame using existing sequence/exchange pair
>- * @lport: The local port that the exchange will be sent on
>- * @sp:	   The sequence to be sent
>- * @fp:	   The frame to be sent on the exchange
>- */
>-static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp,
>+static int fc_seq_send_locked(struct fc_lport *lport, struct fc_seq *sp,
> 		       struct fc_frame *fp)
> {
> 	struct fc_exch *ep;
>@@ -475,7 +469,7 @@ static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp,
> 	u8 fh_type = fh->fh_type;
> 
> 	ep = fc_seq_exch(sp);
>-	WARN_ON((ep->esb_stat & ESB_ST_SEQ_INIT) != ESB_ST_SEQ_INIT);
>+	WARN_ON(!(ep->esb_stat & ESB_ST_SEQ_INIT));
> 
> 	f_ctl = ntoh24(fh->fh_f_ctl);
> 	fc_exch_setup_hdr(ep, fp, f_ctl);
>@@ -498,18 +492,35 @@ static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp,
> 	error = lport->tt.frame_send(lport, fp);
> 
> 	if (fh_type == FC_TYPE_BLS)
>-		return error;
>+		goto out;
> 
> 	/*
> 	 * Update the exchange and sequence flags,
> 	 * assuming all frames for the sequence have been sent.
> 	 * We can only be called to send once for each sequence.
> 	 */
>-	spin_lock_bh(&ep->ex_lock);
> 	ep->f_ctl = f_ctl & ~FC_FC_FIRST_SEQ;	/* not first seq */
> 	if (f_ctl & FC_FC_SEQ_INIT)
> 		ep->esb_stat &= ~ESB_ST_SEQ_INIT;
>-	spin_unlock_bh(&ep->ex_lock);
>+out:
>+	return error;
>+}
>+
>+/**
>+ * fc_seq_send() - Send a frame using existing sequence/exchange pair
>+ * @lport: The local port that the exchange will be sent on
>+ * @sp:	   The sequence to be sent
>+ * @fp:	   The frame to be sent on the exchange
>+ */
>+static int fc_seq_send(struct fc_lport *lport, struct fc_seq *sp,
>+		       struct fc_frame *fp)
>+{
>+	struct fc_exch *ep;
>+	int error;
>+	ep = fc_seq_exch(sp);
>+	spin_lock_bh(&ep->ex_lock);
>+	error = fc_seq_send_locked(lport, sp, fp);
>+	spin_unlock(&ep->ex_lock);
> 	return error;
> }
> 
>@@ -625,7 +636,7 @@ static int fc_exch_abort_locked(struct fc_exch *ep,
> 	if (fp) {
> 		fc_fill_fc_hdr(fp, FC_RCTL_BA_ABTS, ep->did, ep->sid,
> 			       FC_TYPE_BLS, FC_FC_END_SEQ | FC_FC_SEQ_INIT, 0);
>-		error = fc_seq_send(ep->lp, sp, fp);
>+		error = fc_seq_send_locked(ep->lp, sp, fp);
> 	} else
> 		error = -ENOBUFS;
> 	return error;
>@@ -1128,7 +1139,7 @@ static void fc_seq_send_last(struct fc_seq *sp, struct fc_frame *fp,
> 	f_ctl = FC_FC_LAST_SEQ | FC_FC_END_SEQ | FC_FC_SEQ_INIT;
> 	f_ctl |= ep->f_ctl;
> 	fc_fill_fc_hdr(fp, rctl, ep->did, ep->sid, fh_type, f_ctl, 0);
>-	fc_seq_send(ep->lp, sp, fp);
>+	fc_seq_send_locked(ep->lp, sp, fp);
> }
> 
> /**
>@@ -1303,8 +1314,8 @@ static void fc_exch_recv_abts(struct fc_exch *ep, struct fc_frame *rx_fp)
> 		ap->ba_low_seq_cnt = htons(sp->cnt);
> 	}
> 	sp = fc_seq_start_next_locked(sp);
>-	spin_unlock_bh(&ep->ex_lock);
> 	fc_seq_send_last(sp, fp, FC_RCTL_BA_ACC, FC_TYPE_BLS);
>+	spin_unlock_bh(&ep->ex_lock);
> 	fc_frame_free(rx_fp);
> 	return;
> 
>-- 
>1.7.11.7
>

Actions: View | Diff

Attachments on bug 822088: 653689 | 741991