[PATCH 0/2] libfc/fcoe Fixes

[PATCH 0/2] libfc/fcoe Fixes

[Chad Dupuis]

Hi James, Martin

During testing we fixed a deadlock we found in libfc while logging off from the
fabric if an ELS command was still outstanding.  Also we wanted to make clear
virtual link handling a little more robust when the fcoe_ctlr is not logged
into the fabric.

Please apply at your earliest convenience.

Thanks,
Chad

Chad Dupuis (2):
  fcoe: Harden CVL handling when we have not logged into the fabric.
  libfc: Do not take rdata->rp_mutex when processing a -FC_EX_CLOSED ELS
    response.

 drivers/scsi/fcoe/fcoe_ctlr.c | 35 ++++++++++++++++++++++++++++++++---
 drivers/scsi/libfc/fc_rport.c | 32 ++++++++++++++++++++++++--------
 2 files changed, 56 insertions(+), 11 deletions(-)

-- 
1.8.5.6

[PATCH 1/2] fcoe: Harden CVL handling when we have not logged into the fabric.

[Chad Dupuis]

If we haven't logged into the fabric yet we want to be a little more nuanced
with our CVL handling than what we've been:

- If the FCF has been selected, check the source MAC to make sure the frame is
from the FCF we've selected.
- If a FCF is selected and the CVL is from the FCF but we have not logged in
yet, then reset everything and go back to solicitation.

Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
---
 drivers/scsi/fcoe/fcoe_ctlr.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index dcf3653..aed8a52 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -1316,7 +1316,7 @@ drop:
  * The overall length has already been checked.
  */
 static void fcoe_ctlr_recv_clr_vlink(struct fcoe_ctlr *fip,
-				     struct fip_header *fh)
+				     struct sk_buff *skb)
 {
 	struct fip_desc *desc;
 	struct fip_mac_desc *mp;
@@ -1331,14 +1331,18 @@ static void fcoe_ctlr_recv_clr_vlink(struct fcoe_ctlr *fip,
 	int num_vlink_desc;
 	int reset_phys_port = 0;
 	struct fip_vn_desc **vlink_desc_arr = NULL;
+	struct fip_header *fh = (struct fip_header *)skb->data;
+	struct ethhdr *eh = eth_hdr(skb);
 
 	LIBFCOE_FIP_DBG(fip, "Clear Virtual Link received\n");
 
-	if (!fcf || !lport->port_id) {
+	if (!fcf) {
 		/*
 		 * We are yet to select best FCF, but we got CVL in the
 		 * meantime. reset the ctlr and let it rediscover the FCF
 		 */
+		LIBFCOE_FIP_DBG(fip, "Resetting fcoe_ctlr as FCF has not been "
+		    "selected yet\n");
 		mutex_lock(&fip->ctlr_mutex);
 		fcoe_ctlr_reset(fip);
 		mutex_unlock(&fip->ctlr_mutex);
@@ -1346,6 +1350,31 @@ static void fcoe_ctlr_recv_clr_vlink(struct fcoe_ctlr *fip,
 	}
 
 	/*
+	 * If we've selected an FCF check that the CVL is from there to avoid
+	 * processing CVLs from an unexpected source.  If it is from an
+	 * unexpected source drop it on the floor.
+	 */
+	if (!ether_addr_equal(eh->h_source, fcf->fcf_mac)) {
+		LIBFCOE_FIP_DBG(fip, "Dropping CVL due to source address "
+		    "mismatch with FCF src=%pM\n", eh->h_source);
+		return;
+	}
+
+	/*
+	 * If we haven't logged into the fabric but receive a CVL we should
+	 * reset everything and go back to solicitation.
+	 */
+	if (!lport->port_id) {
+		LIBFCOE_FIP_DBG(fip, "lport not logged in, resoliciting\n");
+		mutex_lock(&fip->ctlr_mutex);
+		fcoe_ctlr_reset(fip);
+		mutex_unlock(&fip->ctlr_mutex);
+		fc_lport_reset(fip->lp);
+		fcoe_ctlr_solicit(fip, NULL);
+		return;
+	}
+
+	/*
 	 * mask of required descriptors.  Validating each one clears its bit.
 	 */
 	desc_mask = BIT(FIP_DT_MAC) | BIT(FIP_DT_NAME);
@@ -1576,7 +1605,7 @@ static int fcoe_ctlr_recv_handler(struct fcoe_ctlr *fip, struct sk_buff *skb)
 	if (op == FIP_OP_DISC && sub == FIP_SC_ADV)
 		fcoe_ctlr_recv_adv(fip, skb);
 	else if (op == FIP_OP_CTRL && sub == FIP_SC_CLR_VLINK)
-		fcoe_ctlr_recv_clr_vlink(fip, fiph);
+		fcoe_ctlr_recv_clr_vlink(fip, skb);
 	kfree_skb(skb);
 	return 0;
 drop:
-- 
1.8.5.6

[PATCH 2/2] libfc: Do not take rdata->rp_mutex when processing a -FC_EX_CLOSED ELS response.

[Chad Dupuis]

When an ELS response handler receives a -FC_EX_CLOSED, the rdata->rp_mutex is
already held which can lead to a deadlock condition like the following stack trace:

[<ffffffffa04d8f18>] fc_rport_plogi_resp+0x28/0x200 [libfc]
[<ffffffffa04cfa1a>] fc_invoke_resp+0x6a/0xe0 [libfc]
[<ffffffffa04d0c08>] fc_exch_mgr_reset+0x1b8/0x280 [libfc]
[<ffffffffa04d87b3>] fc_rport_logoff+0x43/0xd0 [libfc]
[<ffffffffa04ce73d>] fc_disc_stop+0x6d/0xf0 [libfc]
[<ffffffffa04ce7ce>] fc_disc_stop_final+0xe/0x20 [libfc]
[<ffffffffa04d55f7>] fc_fabric_logoff+0x17/0x70 [libfc]

The other ELS handlers need to follow the FLOGI response handler and simply do
a kref_put against the fc_rport_priv struct and exit when receving a
-FC_EX_CLOSED response.

Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
---
 drivers/scsi/libfc/fc_rport.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 93f5961..6f19c12 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -884,10 +884,13 @@ static void fc_rport_plogi_resp(struct fc_seq *sp, struct fc_frame *fp,
 	u16 cssp_seq;
 	u8 op;
 
-	mutex_lock(&rdata->rp_mutex);
-
 	FC_RPORT_DBG(rdata, "Received a PLOGI %s\n", fc_els_resp_type(fp));
 
+	if (fp == ERR_PTR(-FC_EX_CLOSED))
+		goto put;
+
+	mutex_lock(&rdata->rp_mutex);
+
 	if (rdata->rp_state != RPORT_ST_PLOGI) {
 		FC_RPORT_DBG(rdata, "Received a PLOGI response, but in state "
 			     "%s\n", fc_rport_state(rdata));
@@ -926,6 +929,7 @@ out:
 	fc_frame_free(fp);
 err:
 	mutex_unlock(&rdata->rp_mutex);
+put:
 	kref_put(&rdata->kref, lport->tt.rport_destroy);
 }
 
@@ -1008,10 +1012,13 @@ static void fc_rport_prli_resp(struct fc_seq *sp, struct fc_frame *fp,
 	u8 op;
 	u8 resp_code = 0;
 
-	mutex_lock(&rdata->rp_mutex);
-
 	FC_RPORT_DBG(rdata, "Received a PRLI %s\n", fc_els_resp_type(fp));
 
+	if (fp == ERR_PTR(-FC_EX_CLOSED))
+		goto put;
+
+	mutex_lock(&rdata->rp_mutex);
+
 	if (rdata->rp_state != RPORT_ST_PRLI) {
 		FC_RPORT_DBG(rdata, "Received a PRLI response, but in state "
 			     "%s\n", fc_rport_state(rdata));
@@ -1079,6 +1086,7 @@ out:
 	fc_frame_free(fp);
 err:
 	mutex_unlock(&rdata->rp_mutex);
+put:
 	kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
 }
 
@@ -1156,10 +1164,13 @@ static void fc_rport_rtv_resp(struct fc_seq *sp, struct fc_frame *fp,
 	struct fc_rport_priv *rdata = rdata_arg;
 	u8 op;
 
-	mutex_lock(&rdata->rp_mutex);
-
 	FC_RPORT_DBG(rdata, "Received a RTV %s\n", fc_els_resp_type(fp));
 
+	if (fp == ERR_PTR(-FC_EX_CLOSED))
+		goto put;
+
+	mutex_lock(&rdata->rp_mutex);
+
 	if (rdata->rp_state != RPORT_ST_RTV) {
 		FC_RPORT_DBG(rdata, "Received a RTV response, but in state "
 			     "%s\n", fc_rport_state(rdata));
@@ -1201,6 +1212,7 @@ out:
 	fc_frame_free(fp);
 err:
 	mutex_unlock(&rdata->rp_mutex);
+put:
 	kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
 }
 
@@ -1292,10 +1304,13 @@ static void fc_rport_adisc_resp(struct fc_seq *sp, struct fc_frame *fp,
 	struct fc_els_adisc *adisc;
 	u8 op;
 
-	mutex_lock(&rdata->rp_mutex);
-
 	FC_RPORT_DBG(rdata, "Received a ADISC response\n");
 
+	if (fp == ERR_PTR(-FC_EX_CLOSED))
+		goto put;
+
+	mutex_lock(&rdata->rp_mutex);
+
 	if (rdata->rp_state != RPORT_ST_ADISC) {
 		FC_RPORT_DBG(rdata, "Received a ADISC resp but in state %s\n",
 			     fc_rport_state(rdata));
@@ -1330,6 +1345,7 @@ out:
 	fc_frame_free(fp);
 err:
 	mutex_unlock(&rdata->rp_mutex);
+put:
 	kref_put(&rdata->kref, rdata->local_port->tt.rport_destroy);
 }
 
-- 
1.8.5.6

Re: [PATCH 0/2] libfc/fcoe Fixes

[Hannes Reinecke]

Hi Chad,

On 09/28/2016 07:30 PM, Chad Dupuis wrote:
> 
> During testing we fixed a deadlock we found in libfc while logging off from the
> fabric if an ELS command was still outstanding.  Also we wanted to make clear
> virtual link handling a little more robust when the fcoe_ctlr is not logged
> into the fabric.
> 
> Please apply at your earliest convenience.
> 
> Thanks,
> Chad
> 
> Chad Dupuis (2):
>   fcoe: Harden CVL handling when we have not logged into the fabric.
>   libfc: Do not take rdata->rp_mutex when processing a -FC_EX_CLOSED ELS
>     response.
> 
>  drivers/scsi/fcoe/fcoe_ctlr.c | 35 ++++++++++++++++++++++++++++++++---
>  drivers/scsi/libfc/fc_rport.c | 32 ++++++++++++++++++++++++--------
>  2 files changed, 56 insertions(+), 11 deletions(-)
> 
I'm currently preparing a larger FCoE patchset (for my VN2VN fixes),
which also includes the second patch.
Should I take the first one, too, and submit it in one bunch or do you
prefer to keep both separate?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 0/2] libfc/fcoe Fixes

[Chad Dupuis]

On Thu, 29 Sep 2016, 8:06am -0000, Hannes Reinecke wrote:

> Hi Chad,
> 
> On 09/28/2016 07:30 PM, Chad Dupuis wrote:
> > 
> > During testing we fixed a deadlock we found in libfc while logging off from the
> > fabric if an ELS command was still outstanding.  Also we wanted to make clear
> > virtual link handling a little more robust when the fcoe_ctlr is not logged
> > into the fabric.
> > 
> > Please apply at your earliest convenience.
> > 
> > Thanks,
> > Chad
> > 
> > Chad Dupuis (2):
> >   fcoe: Harden CVL handling when we have not logged into the fabric.
> >   libfc: Do not take rdata->rp_mutex when processing a -FC_EX_CLOSED ELS
> >     response.
> > 
> >  drivers/scsi/fcoe/fcoe_ctlr.c | 35 ++++++++++++++++++++++++++++++++---
> >  drivers/scsi/libfc/fc_rport.c | 32 ++++++++++++++++++++++++--------
> >  2 files changed, 56 insertions(+), 11 deletions(-)
> > 
> I'm currently preparing a larger FCoE patchset (for my VN2VN fixes),
> which also includes the second patch.
> Should I take the first one, too, and submit it in one bunch or do you
> prefer to keep both separate?
> 
> Cheers,
> 
> Hannes
> 

I'm fine either way to be honest...whatever is easier for Martin.

Re: [PATCH 0/2] libfc/fcoe Fixes

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> I'm currently preparing a larger FCoE patchset (for my VN2VN
Hannes> fixes), which also includes the second patch.  Should I take the
Hannes> first one, too, and submit it in one bunch or do you prefer to
Hannes> keep both separate?

Well, 4.9 is right around the corner so I'm guessing your patchset will
be aimed at 4.10. But assuming we get some quick reviews for Chad's
patches we could squeeze those in.

-- 
Martin K. Petersen	Oracle Linux Engineering