[PATCH 0/8] libfc, libfcoe and fcoe updates for scsi-misc

[PATCH 0/8] libfc, libfcoe and fcoe updates for scsi-misc

[Robert Love]

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

[PATCH 1/8] libfc: fix setting of rport dev loss

[Robert Love]

From: Mike Christie <michaelc@cs.wisc.edu>

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.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_rport.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index b9f2286..a84ef13 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -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);
 

[PATCH 2/8] libfc: use DID_TRANSPORT_DISRUPTED while lport not ready

[Robert Love]

From: Vasu Dev <vasu.dev@intel.com>

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

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_fcp.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index c797f6b..43866e6 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -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);

[PATCH 3/8] libfc: adds flogi retry in case DID is zero in RJT

[Robert Love]

From: Vasu Dev <vasu.dev@intel.com>

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.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_lport.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c
index d9b6e11..9be63ed 100644
--- a/drivers/scsi/libfc/fc_lport.c
+++ b/drivers/scsi/libfc/fc_lport.c
@@ -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);

[PATCH 4/8] fcoe: set default FIP mode as FIP_MODE_FABRIC

[Robert Love]

From: Vasu Dev <vasu.dev@intel.com>

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

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 844d618..8225b82 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -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,

[PATCH 5/8] libfc: possible race could panic system due to NULL fsp->cmd

[Robert Love]

From: Vasu Dev <vasu.dev@intel.com>

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 <christopher.leech@intel.com>
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.

Signed-off-by: Vasu Dev <vasu.dev@intel.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_fcp.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/libfc/fc_fcp.c b/drivers/scsi/libfc/fc_fcp.c
index 43866e6..e340373 100644
--- a/drivers/scsi/libfc/fc_fcp.c
+++ b/drivers/scsi/libfc/fc_fcp.c
@@ -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 */

[PATCH 6/8] libfc: Do not let disc work cancel itself

[Robert Love]

From: Bhanu Prakash Gollapudi <bprakash@broadcom.com>

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.

Signed-off-by: Bhanu Prakash Gollapudi <bprakash@broadcom.com>
Acked-by: Joe Eykholt <jeykholt@cisco.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/libfc/fc_disc.c |    5 ++---
 include/scsi/libfc.h         |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libfc/fc_disc.c b/drivers/scsi/libfc/fc_disc.c
index 32f67c4..911b273 100644
--- a/drivers/scsi/libfc/fc_disc.c
+++ b/drivers/scsi/libfc/fc_disc.c
@@ -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);
 }
 
 /**
diff --git a/include/scsi/libfc.h b/include/scsi/libfc.h
index 14be49b..f986ab7 100644
--- a/include/scsi/libfc.h
+++ b/include/scsi/libfc.h
@@ -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

[PATCH 7/8] libfcoe: VN2VN connection setup causing stack memory corruption.

[Robert Love]

From: Kiran Patil <kiran.patil@intel.com>

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

Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Acked-by: Joe Eykholt <jeykholt@cisco.com>
Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/libfcoe.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/fcoe/libfcoe.c b/drivers/scsi/fcoe/libfcoe.c
index aa503d8..bc17c71 100644
--- a/drivers/scsi/fcoe/libfcoe.c
+++ b/drivers/scsi/fcoe/libfcoe.c
@@ -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;

[PATCH 8/8] fcoe: Fix broken NPIV with correction to MAC validation

[Robert Love]

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.

>From the FC-BB-5 specification,

"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.

Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 8225b82..d23a538 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html