Patchwork [Open-FCoE] fcoe: Fix a vlan tagging problem

login
register
mail settings
Submitter Neil Horman
Date June 25, 2013, 7:09 p.m.
Message ID <1372187361-24334-1-git-send-email-nhorman@tuxdriver.com>
Download mbox | patch
Permalink /patch/67/
State Superseded
Headers show

Comments

Neil Horman - June 25, 2013, 7:09 p.m.
I was testing upstream FCoE and noticed that I was no longer able to find any
luns, nor were any of my fc_host devices comming out of Offline state.  I took
some tcpdumps of the problem and noted that my FLOGI requests were getting
accepted but what should have been my PLOGI requests, were getting interpreted
by wireshark as uncategorized FCP data.  There was a vlan header, but the
ethertype was all zeros, causing it, and all subsequent data to get
misinterpreted.  Looking further, I noted that in fcoe_xmit, the path that
handles PLOGI requests does some manual vlan tag addition, rather than using
vlan_put_tag as it should.  I'm honestly not sure how it ever worked, as I don't
see that the manual tag addition ever set the h_vlan_proto field to
htons(ETH_P_8021Q) as it should have.  I assume that just worked by luck, and
some recent change in the vlan code caused it to stop functioning.  At any rate,
I've tested out the below fix, and this properly tags frames with a vlan in both
the non-accelerated and hw accelerated case.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Robert Love <robert.w.love@intel.com>
CC: stable@vger.kernel.org
---
 drivers/scsi/fcoe/fcoe.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Robert Love - June 25, 2013, 7:13 p.m.
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Tuesday, June 25, 2013 12:09 PM
> To: fcoe-devel@open-fcoe.org
> Cc: Neil Horman; Love, Robert W; stable@vger.kernel.org
> Subject: [PATCH] fcoe: Fix a vlan tagging problem
> 
> I was testing upstream FCoE and noticed that I was no longer able to find any
> luns, nor were any of my fc_host devices comming out of Offline state.  I
> took some tcpdumps of the problem and noted that my FLOGI requests
> were getting accepted but what should have been my PLOGI requests, were
> getting interpreted by wireshark as uncategorized FCP data.  There was a
> vlan header, but the ethertype was all zeros, causing it, and all subsequent
> data to get misinterpreted.  Looking further, I noted that in fcoe_xmit, the
> path that handles PLOGI requests does some manual vlan tag addition,
> rather than using vlan_put_tag as it should.  I'm honestly not sure how it ever
> worked, as I don't see that the manual tag addition ever set the
> h_vlan_proto field to
> htons(ETH_P_8021Q) as it should have.  I assume that just worked by luck,
> and some recent change in the vlan code caused it to stop functioning.  At
> any rate, I've tested out the below fix, and this properly tags frames with a
> vlan in both the non-accelerated and hw accelerated case.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Robert Love <robert.w.love@intel.com>
> CC: stable@vger.kernel.org
> ---
>  drivers/scsi/fcoe/fcoe.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c index
> fc7bb1f..731c471 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1660,9 +1660,8 @@ static int fcoe_xmit(struct fc_lport *lport, struct
> fc_frame *fp)
> 
>  	if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN &&
>  	    fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
> -		skb->vlan_tci = VLAN_TAG_PRESENT |
> -				vlan_dev_vlan_id(fcoe->netdev);
>  		skb->dev = fcoe->realdev;
> +		vlan_put_tag(skb, htons(ETH_P_8021Q),
> +vlan_dev_vlan_id(fcoe->netdev));
>  	} else
>  		skb->dev = fcoe->netdev;
> 
> --
> 1.8.1.4

Hey Neil, this is the same change as recently discussed on list here:

http://patchwork.open-fcoe.org/patch/65/

The only difference between yours and my patch is that I check the return value from vlan_put_tag.

I'm going to send my patch up today, unless you have an objection.

//Rob
Neil Horman - June 25, 2013, 7:26 p.m.
On Tue, Jun 25, 2013 at 07:13:51PM +0000, Love, Robert W wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Tuesday, June 25, 2013 12:09 PM
> > To: fcoe-devel@open-fcoe.org
> > Cc: Neil Horman; Love, Robert W; stable@vger.kernel.org
> > Subject: [PATCH] fcoe: Fix a vlan tagging problem
> > 
> > I was testing upstream FCoE and noticed that I was no longer able to find any
> > luns, nor were any of my fc_host devices comming out of Offline state.  I
> > took some tcpdumps of the problem and noted that my FLOGI requests
> > were getting accepted but what should have been my PLOGI requests, were
> > getting interpreted by wireshark as uncategorized FCP data.  There was a
> > vlan header, but the ethertype was all zeros, causing it, and all subsequent
> > data to get misinterpreted.  Looking further, I noted that in fcoe_xmit, the
> > path that handles PLOGI requests does some manual vlan tag addition,
> > rather than using vlan_put_tag as it should.  I'm honestly not sure how it ever
> > worked, as I don't see that the manual tag addition ever set the
> > h_vlan_proto field to
> > htons(ETH_P_8021Q) as it should have.  I assume that just worked by luck,
> > and some recent change in the vlan code caused it to stop functioning.  At
> > any rate, I've tested out the below fix, and this properly tags frames with a
> > vlan in both the non-accelerated and hw accelerated case.
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Robert Love <robert.w.love@intel.com>
> > CC: stable@vger.kernel.org
> > ---
> >  drivers/scsi/fcoe/fcoe.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c index
> > fc7bb1f..731c471 100644
> > --- a/drivers/scsi/fcoe/fcoe.c
> > +++ b/drivers/scsi/fcoe/fcoe.c
> > @@ -1660,9 +1660,8 @@ static int fcoe_xmit(struct fc_lport *lport, struct
> > fc_frame *fp)
> > 
> >  	if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN &&
> >  	    fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
> > -		skb->vlan_tci = VLAN_TAG_PRESENT |
> > -				vlan_dev_vlan_id(fcoe->netdev);
> >  		skb->dev = fcoe->realdev;
> > +		vlan_put_tag(skb, htons(ETH_P_8021Q),
> > +vlan_dev_vlan_id(fcoe->netdev));
> >  	} else
> >  		skb->dev = fcoe->netdev;
> > 
> > --
> > 1.8.1.4
> 
> Hey Neil, this is the same change as recently discussed on list here:
> 
> http://patchwork.open-fcoe.org/patch/65/
> 
> The only difference between yours and my patch is that I check the return value from vlan_put_tag.
> 
> I'm going to send my patch up today, unless you have an objection.
> 
ACK, works for me, please send it to stable as well if you could.

Thanks!
Neil

> //Rob
> 
>
Robert Love - June 25, 2013, 7:29 p.m.
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Tuesday, June 25, 2013 12:26 PM
> To: Love, Robert W
> Cc: fcoe-devel@open-fcoe.org; stable@vger.kernel.org
> Subject: Re: [PATCH] fcoe: Fix a vlan tagging problem
> 
> On Tue, Jun 25, 2013 at 07:13:51PM +0000, Love, Robert W wrote:
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > Sent: Tuesday, June 25, 2013 12:09 PM
> > > To: fcoe-devel@open-fcoe.org
> > > Cc: Neil Horman; Love, Robert W; stable@vger.kernel.org
> > > Subject: [PATCH] fcoe: Fix a vlan tagging problem
> > >
> > > I was testing upstream FCoE and noticed that I was no longer able to
> > > find any luns, nor were any of my fc_host devices comming out of
> > > Offline state.  I took some tcpdumps of the problem and noted that
> > > my FLOGI requests were getting accepted but what should have been my
> > > PLOGI requests, were getting interpreted by wireshark as
> > > uncategorized FCP data.  There was a vlan header, but the ethertype
> > > was all zeros, causing it, and all subsequent data to get
> > > misinterpreted.  Looking further, I noted that in fcoe_xmit, the
> > > path that handles PLOGI requests does some manual vlan tag addition,
> > > rather than using vlan_put_tag as it should.  I'm honestly not sure
> > > how it ever worked, as I don't see that the manual tag addition ever
> > > set the h_vlan_proto field to
> > > htons(ETH_P_8021Q) as it should have.  I assume that just worked by
> > > luck, and some recent change in the vlan code caused it to stop
> > > functioning.  At any rate, I've tested out the below fix, and this
> > > properly tags frames with a vlan in both the non-accelerated and hw
> accelerated case.
> > >
> > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > > CC: Robert Love <robert.w.love@intel.com>
> > > CC: stable@vger.kernel.org
> > > ---
> > >  drivers/scsi/fcoe/fcoe.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> > > index
> > > fc7bb1f..731c471 100644
> > > --- a/drivers/scsi/fcoe/fcoe.c
> > > +++ b/drivers/scsi/fcoe/fcoe.c
> > > @@ -1660,9 +1660,8 @@ static int fcoe_xmit(struct fc_lport *lport,
> > > struct fc_frame *fp)
> > >
> > >  	if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN &&
> > >  	    fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
> > > -		skb->vlan_tci = VLAN_TAG_PRESENT |
> > > -				vlan_dev_vlan_id(fcoe->netdev);
> > >  		skb->dev = fcoe->realdev;
> > > +		vlan_put_tag(skb, htons(ETH_P_8021Q),
> > > +vlan_dev_vlan_id(fcoe->netdev));
> > >  	} else
> > >  		skb->dev = fcoe->netdev;
> > >
> > > --
> > > 1.8.1.4
> >
> > Hey Neil, this is the same change as recently discussed on list here:
> >
> > http://patchwork.open-fcoe.org/patch/65/
> >
> > The only difference between yours and my patch is that I check the return
> value from vlan_put_tag.
> >
> > I'm going to send my patch up today, unless you have an objection.
> >
> ACK, works for me, please send it to stable as well if you could.
> 

OK, I missed one of John's comments, so I just sent an updated version.... I'm testing it now, but I'm going to be delayed by an upcoming block of meetings.

This problem was introduced in the 3.9 merge window. I don't think it needs to go to stable unless it misses 3.9-rc, which I think is possible. I'm on top of it.

//Rob

Patch

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index fc7bb1f..731c471 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1660,9 +1660,8 @@  static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
 
 	if (fcoe->netdev->priv_flags & IFF_802_1Q_VLAN &&
 	    fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
-		skb->vlan_tci = VLAN_TAG_PRESENT |
-				vlan_dev_vlan_id(fcoe->netdev);
 		skb->dev = fcoe->realdev;
+		vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_dev_vlan_id(fcoe->netdev));
 	} else
 		skb->dev = fcoe->netdev;