libfc, libfcoe and fcoe updates for scsi-misc

Here are a few fixes to libfc, libfcoe and fcoe. No new features, just bug fixes.

---

Bhanu Prakash Gollapudi (1):
libfc: Do not let disc work cancel itself

Kiran Patil (1):
libfcoe: VN2VN connection setup causing stack memory corruption.

Mike Christie (1):
libfc: fix setting of rport dev loss

Robert Love (1):
fcoe: Fix broken NPIV with correction to MAC validation

Vasu Dev (4):
libfc: use DID_TRANSPORT_DISRUPTED while lport not ready
libfc: adds flogi retry in case DID is zero in RJT
fcoe: set default FIP mode as FIP_MODE_FABRIC
libfc: possible race could panic system due to NULL fsp->cmd

 drivers/scsi/fcoe/fcoe.c      |   18 +++++++-----------
 drivers/scsi/fcoe/libfcoe.c   |    2 +-
 drivers/scsi/libfc/fc_disc.c  |    5 ++---
 drivers/scsi/libfc/fc_fcp.c   |   24 ++++++++----------------
 drivers/scsi/libfc/fc_lport.c |   12 ++++--------
 drivers/scsi/libfc/fc_rport.c |    4 ++--
 include/scsi/libfc.h          |    2 +-
 7 files changed, 25 insertions(+), 42 deletions(-)

Thanks, //Rob

From: Mike Christie <[email protected]>

There does not seem to be a reason why libfc adds a 5 second delay to the user requested value for the dev loss tmo. There also does not seem to be a reason to allow setting it to 0 (or really close).

This patch removes the extra 5 sec delay, and for 0 it sets it to 1 like other fc drivers. We should actually be able to set it to 0 since the queue_delayed_work API will just call queue_work, but other drivers set it to 1 in that case.

---
 drivers/scsi/libfc/fc_rport.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index b9f2286..a84ef13 100644
@@ -196,9 +196,9 @@ static const char *fc_rport_state(struct fc_rport_priv *rdata)
 void fc_set_rport_loss_tmo(struct fc_rport *rport, u32 timeout)
 {
 	if (timeout)
-		rport->dev_loss_tmo = timeout + 5;
+		rport->dev_loss_tmo = timeout;
 	else
-		rport->dev_loss_tmo = 30;
+		rport->dev_loss_tmo = 1;
 }
 EXPORT_SYMBOL(fc_set_rport_loss_tmo);
 

From: Vasu Dev <[email protected]>

This is per Mile Christie feedback since in this case IO could get retried for tape devices and therefore DID_REQUEUE cannot be used, more details in this thread.

http://marc.info/?l=linux-scsi&m=127970522630136&w=2

---
 drivers/scsi/libfc/fc_fcp.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

index c797f6b..43866e6 100644
@@ -1971,10 +1971,8 @@ static void fc_io_compl(struct fc_fcp_pkt *fsp)
 		break;
 	}
 
-	if (lport->state != LPORT_ST_READY && fsp->status_code != FC_COMPLETE) {
-		sc_cmd->result = (DID_REQUEUE << 16);
-		FC_FCP_DBG(fsp, "Returning DID_REQUEUE to scsi-ml\n");
-	}
+	if (lport->state != LPORT_ST_READY && fsp->status_code != FC_COMPLETE)
+		sc_cmd->result = (DID_TRANSPORT_DISRUPTED << 16);
 
 	spin_lock_irqsave(&si->scsi_queue_lock, flags);
 	list_del(&fsp->list);

From: Vasu Dev <[email protected]>

Sometimes switch in NPV mode rejects flogi request with DID zero and in that case flogi is not tried again and port remains offline, so this patch validates DID for non zero along with only ACC response to allow flogi retry for RJT with DID=0 also succeed FLOGI in next try.

---
 drivers/scsi/libfc/fc_lport.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

index d9b6e11..9be63ed 100644
@@ -1447,13 +1447,7 @@ void fc_lport_flogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 	}
 
 	did = fc_frame_did(fp);
-
-	if (!did) {
-		FC_LPORT_DBG(lport, "Bad FLOGI response\n");
-		goto out;
-	}
-
-	if (fc_frame_payload_op(fp) == ELS_LS_ACC) {
+	if (fc_frame_payload_op(fp) == ELS_LS_ACC && did) {
 		flp = fc_frame_payload_get(fp, sizeof(*flp));
 		if (flp) {
 			mfs = ntohs(flp->fl_csp.sp_bb_data) &

@@ -1492,8 +1486,10 @@ void fc_lport_flogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 				fc_lport_enter_dns(lport);
 			}
 		}
-	} else
+	} else {
+		FC_LPORT_DBG(lport, "FLOGI RJT or bad response\n");
 		fc_lport_error(lport, fp);
+	}
 
 out:
 	fc_frame_free(fp);

From: Vasu Dev <[email protected]>

Since sometimes current FIP_MODE_AUTO mode falls back to non-FIP mode while DCB link still getting ready in fabric mode with its peer switch, it falls back after few libfc flogi retries and that is not we want while working with FIP enabled switches in FABRIC mode, therefore sets default as FIP_MODE_FABRIC as discussed and agreed before in this mail thread
http://www.open-fcoe.org/pipermail/devel/2010-August/010511.html

---
 drivers/scsi/fcoe/fcoe.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 844d618..8225b82 100644
@@ -117,7 +117,7 @@ static void fcoe_recv_frame(struct sk_buff *skb);
 
 static void fcoe_get_lesb(struct fc_lport *, struct fc_els_lesb *);
 
-module_param_call(create, fcoe_create, NULL, (void *)FIP_MODE_AUTO, S_IWUSR);
+module_param_call(create, fcoe_create, NULL, (void *)FIP_MODE_FABRIC, S_IWUSR);
 __MODULE_PARM_TYPE(create, "string");
 MODULE_PARM_DESC(create, " Creates fcoe instance on a ethernet interface");
 module_param_call(create_vn2vn, fcoe_create, NULL,

From: Vasu Dev <[email protected]>

It is unlikely but in case if it hits then it would cause panic due to null cmd ptr, so far only one instance seen recently with ESX though this was introduced long ago with this commit:-

commit c1ecb90a66c5afc7cc5c9349f9c3714eef4a5cfb

Author: Chris Leech <[email protected]>
Date:   Thu Dec 10 09:59:26 2009 -0800

[SCSI] libfc: reduce hold time on SCSI host lock

Currently fsp->cmd is set to NULL w/o scsi_queue_lock before dequeuing from scsi_pkt_queue and that could cause NULL fsp->cmd in fc_fcp_cleanup_each_cmd for cmd completing with fsp->cmd = NULL after fc_fcp_cleanup_each_cmd taken reference. No need to set fsp->cmd to NULL as this is also protected by fc_fcp_lock_pkt(), for above race the fc_fcp_lock_pkt() in fc_fcp_cleanup_each_cmd() will fail as that cmd is already done.

Mike mentioned same issue at
http://www.open-fcoe.org/pipermail/devel/2010-September/010533.html

Similarly moved sc_cmd->SCp.ptr = NULL under scsi_queue_lock so that scsi abort error handler won't abort on completed cmds.

---
 drivers/scsi/libfc/fc_fcp.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

index 43866e6..e340373 100644
@@ -58,8 +58,7 @@ struct kmem_cache *scsi_pkt_cachep;
 #define FC_SRB_WRITE		(1 << 0)
 
 /*
- * The SCp.ptr should be tested and set under the host lock. NULL indicates
- * that the command has been retruned to the scsi layer.
+ * The SCp.ptr should be tested and set under the scsi_pkt_queue lock
  */
 #define CMD_SP(Cmnd)		    ((struct fc_fcp_pkt *)(Cmnd)->SCp.ptr)
 #define CMD_ENTRY_STATUS(Cmnd)	    ((Cmnd)->SCp.have_data_in)

@@ -1880,8 +1879,6 @@ static void fc_io_compl(struct fc_fcp_pkt *fsp)
 
 	lport = fsp->lp;
 	si = fc_get_scsi_internal(lport);
-	if (!fsp->cmd)
-		return;
 
 	/*
 	 * if can_queue ramp down is done then try can_queue ramp up

@@ -1891,11 +1888,6 @@ static void fc_io_compl(struct fc_fcp_pkt *fsp)
 		fc_fcp_can_queue_ramp_up(lport);
 
 	sc_cmd = fsp->cmd;
-	fsp->cmd = NULL;
-
-	if (!sc_cmd->SCp.ptr)
-		return;
-
 	CMD_SCSI_STATUS(sc_cmd) = fsp->cdb_status;
 	switch (fsp->status_code) {
 	case FC_COMPLETE:

@@ -1976,8 +1968,8 @@ static void fc_io_compl(struct fc_fcp_pkt *fsp)
 
 	spin_lock_irqsave(&si->scsi_queue_lock, flags);
 	list_del(&fsp->list);
-	spin_unlock_irqrestore(&si->scsi_queue_lock, flags);
 	sc_cmd->SCp.ptr = NULL;
+	spin_unlock_irqrestore(&si->scsi_queue_lock, flags);
 	sc_cmd->scsi_done(sc_cmd);
 
 	/* release ref from initial allocation in queue command */

@@ -1995,6 +1987,7 @@ int fc_eh_abort(struct scsi_cmnd *sc_cmd)
 {
 	struct fc_fcp_pkt *fsp;
 	struct fc_lport *lport;
+	struct fc_fcp_internal *si;
 	int rc = FAILED;
 	unsigned long flags;
 

@@ -2004,7 +1997,8 @@ int fc_eh_abort(struct scsi_cmnd *sc_cmd)
 	else if (!lport->link_up)
 		return rc;
 
-	spin_lock_irqsave(lport->host->host_lock, flags);
+	si = fc_get_scsi_internal(lport);
+	spin_lock_irqsave(&si->scsi_queue_lock, flags);
 	fsp = CMD_SP(sc_cmd);
 	if (!fsp) {
 		/* command completed while scsi eh was setting up */

@@ -2013,7 +2007,7 @@ int fc_eh_abort(struct scsi_cmnd *sc_cmd)
 	}
 	/* grab a ref so the fsp and sc_cmd cannot be relased from under us */
 	fc_fcp_pkt_hold(fsp);
-	spin_unlock_irqrestore(lport->host->host_lock, flags);
+	spin_unlock_irqrestore(&si->scsi_queue_lock, flags);
 
 	if (fc_fcp_lock_pkt(fsp)) {
 		/* completed while we were waiting for timer to be deleted */

From: Bhanu Prakash Gollapudi <[email protected]>

When number of NPIV ports created are greater than the xids allocated per pool -- for eg., creating 255 NPIV ports on a system with nr_cpu_ids of 32, with each pool containing 128 xids -- and then generating a link event - for eg., shutdown/no shutdown -- on the switch port causes the hang with the following stack trace.

Call Trace:
schedule_timeout+0x19d/0x230
wait_for_common+0xc0/0x170
__cancel_work_timer+0xcf/0x1b0
fc_disc_stop+0x16/0x30 [libfc]
fc_lport_reset_locked+0x47/0x90 [libfc]
fc_lport_enter_reset+0x67/0xe0 [libfc]
fc_lport_disc_callback+0xbc/0xe0 [libfc]
fc_disc_done+0xa8/0xf0 [libfc]
fc_disc_timeout+0x29/0x40 [libfc]
run_workqueue+0xb8/0x140
worker_thread+0x96/0x110
kthread+0x96/0xa0
child_rip+0xa/0x20

Fix is to not cancel the disc_work if discovery is already stopped, thus allowing lport state machine to restart and try discovery again.

---
 drivers/scsi/libfc/fc_disc.c |    5 ++---
 include/scsi/libfc.h         |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

index 32f67c4..911b273 100644
@@ -684,10 +684,9 @@ void fc_disc_stop(struct fc_lport *lport)
 {
 	struct fc_disc *disc = &lport->disc;
 
-	if (disc) {
+	if (disc->pending)
 		cancel_delayed_work_sync(&disc->disc_work);
-		fc_disc_stop_rports(disc);
-	}
+	fc_disc_stop_rports(disc);
 }
 
 /**

index 14be49b..f986ab7 100644
@@ -721,7 +721,7 @@ struct libfc_function_template {
  * struct fc_disc - Discovery context
  * @retry_count:   Number of retries
  * @pending:       1 if discovery is pending, 0 if not
- * @requesting:    1 if discovery has been requested, 0 if not
+ * @requested:     1 if discovery has been requested, 0 if not
  * @seq_count:     Number of sequences used for discovery
  * @buf_len:       Length of the discovery buffer
  * @disc_id:       Discovery ID

From: Kiran Patil <[email protected]>

Fix: When FIP frame is received, function fcoe_ctlr_vn_recv calls function fcoe_ctlr_vn_parse which does memset for addr (&buf.rdata) which leads to memory corruption. Code was trying to treat "buf" as struct but it was defined as union. Fix is to change from union to struct for "buf" in function fcoe_ctlr_vn_recv.

Technical Details: N/A

---
 drivers/scsi/fcoe/libfcoe.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index aa503d8..bc17c71 100644
@@ -2296,7 +2296,7 @@ static int fcoe_ctlr_vn_recv(struct fcoe_ctlr *fip, struct sk_buff *skb)
 {
 	struct fip_header *fiph;
 	enum fip_vn2vn_subcode sub;
-	union {
+	struct {
 		struct fc_rport_priv rdata;
 		struct fcoe_rport frport;
 	} buf;

A previous patch attempted to validate the destination MAC address of a FCoE frame by checking that MAC address against the received port's MAC address. The implementation seems fine on the surface, but any VN_Ports added using the NPIV feature will have their own MAC addresses and these MACs were not being checked, which prevented any NPIV VN_Ports from receiving frames.

In other words, the following patch has broken NPIV.

519e5135e2537c9dbc1cbcc0891b0a936ff5dcd2
[SCSI] fcoe: adds src and dest mac address
checking for fcoe frames

Part of the offending patch is correct, but the part that broke NPIV was attempting to satisfy FC-BB-5 section D.5, 2.1-

(discard frames that) "contain a destination MAC address/destination N_Port_ID pair that was not assigned by an FCF to one of the VN_Ports on the ENode"

The language does _not_ say to compare the destination FC-MAP/destination N_Port_ID, but instead to compare the destination MAC address/destination N_Port_ID.

"A properly formed FPMA is one in which the 24 most significant bits equal the Fabric’s FC-MAP value and the least significant 24 bits equal the N_Port_ID assigned to the VN_Port by the FCF."

This means that we need to compare the FC Frame's destination FCID against the embedded FCID in the destination MAC address. This patch checks the lower 24 bits of the destination MAC address against destination FCID in the Fibre Channel frame.

For MAC validation the first line of defense is the hardware MAC filtering. Each VN_Port will have a unicast MAC addresses added to the hardware's filtering table. The Ethernet driver should drop any MACs not destined for a programmed MAC. This patch adds a second line of defense that very specfically compares an element in the FC frame against an element in the Ethernet header, which is appropriate for the FCoE layer.

Many alternative approaches were considered, including a LLD callback from libfc. The second most reasonable approach seemed to be walking the list of NPIV ports and check each of their MAC addresses against the destination MAC address of the received frame. The problem with this approach was that it is likely that performance would suffer with the more NPIV ports added to the system since every received frame would need to walk this list, comparing each entry's MAC.

---
 drivers/scsi/fcoe/fcoe.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

index 8225b82..d23a538 100644
@@ -1243,7 +1243,6 @@ int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 	struct fcoe_interface *fcoe;
 	struct fc_frame_header *fh;
 	struct fcoe_percpu_s *fps;
-	struct fcoe_port *port;
 	struct ethhdr *eh;
 	unsigned int cpu;
 

@@ -1262,16 +1261,7 @@ int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 			skb_tail_pointer(skb), skb_end_pointer(skb),
 			skb->csum, skb->dev ? skb->dev->name : "<NULL>");
 
-	/* check for mac addresses */
 	eh = eth_hdr(skb);
-	port = lport_priv(lport);
-	if (compare_ether_addr(eh->h_dest, port->data_src_addr) &&
-	    compare_ether_addr(eh->h_dest, fcoe->ctlr.ctl_src_addr) &&
-	    compare_ether_addr(eh->h_dest, (u8[6])FC_FCOE_FLOGI_MAC)) {
-		FCOE_NETDEV_DBG(netdev, "wrong destination mac address:%pM\n",
-				eh->h_dest);
-		goto err;
-	}
 
 	if (is_fip_mode(&fcoe->ctlr) &&
 	    compare_ether_addr(eh->h_source, fcoe->ctlr.dest_addr)) {

@@ -1291,6 +1281,12 @@ int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 	skb_set_transport_header(skb, sizeof(struct fcoe_hdr));
 	fh = (struct fc_frame_header *) skb_transport_header(skb);
 
+	if (ntoh24(&eh->h_dest[3]) != ntoh24(fh->fh_d_id)) {
+		FCOE_NETDEV_DBG(netdev, "FC frame d_id mismatch with MAC:%pM\n",
+				eh->h_dest);
+		goto err;
+	}
+
 	fr = fcoe_dev_from_skb(skb);
 	fr->fr_dev = lport;
 	fr->ptype = ptype;

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html