Patchwork [Open-FCoE] fcoe: Use correct API to set vlan tag for FCoE Ethertype skbs

login
register
mail settings
Submitter Robert Love
Date June 13, 2013, 8:54 p.m.
Message ID <20130613205405.4950.23064.stgit@fritz>
Download mbox | patch
Permalink /patch/64/
State Superseded
Headers show

Comments

Robert Love - June 13, 2013, 8:54 p.m.
fcoe_xmit was coded such that it would skip the vlan net device/layer
and instead set some vlan flags and transmit on the real net device.
The real net device has code that would add the vlan tag for fcoe skbs.
This avoids some extra processing for data frames and provides a small
performance improvement.

Since fcoe_xmit was not using the vlan net device, __vlan_put_tag
within the real net device's xmit routine was ultimately being
called to set the vlan tag.

With the below change the behavior of __vlan_put_tag changed slightly,
it now sets the skb->protocol = vlan_proto. vlan_proto was not a field
being set by fcoe_xmit, so the skb->protocol is now not being set to
ETH_P_8021Q, as it should be.

This patch converts fcoe_xmit to use the vlan_put_tag routine which
will tag the skb and fcoe will continue to transmit fcoe skbs on the
real net device.

For reference, the below change was the one that altered the
__vlan_put_tag behavior.

  commit 86a9bad3ab6b6f858fd4443b48738cabbb6d094c
  Author: Patrick McHardy <kaber@trash.net>
  Date:   Fri Apr 19 02:04:30 2013 +0000

      net: vlan: add protocol argument to packet tagging functions

      Add a protocol argument to the VLAN packet tagging functions. In case of HW
      tagging, we need that protocol available in the ndo_start_xmit functions,
      so it is stored in a new field in the skb. The new field fits into a hole
      (on 64 bit) and doesn't increase the sks's size.

Signed-off-by: Robert Love <robert.w.love@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)
John Fastabend - June 17, 2013, 10:48 p.m.
On 6/13/2013 1:54 PM, Robert Love wrote:
> fcoe_xmit was coded such that it would skip the vlan net device/layer
> and instead set some vlan flags and transmit on the real net device.
> The real net device has code that would add the vlan tag for fcoe skbs.
> This avoids some extra processing for data frames and provides a small
> performance improvement.
>
> Since fcoe_xmit was not using the vlan net device, __vlan_put_tag
> within the real net device's xmit routine was ultimately being
> called to set the vlan tag.
>
> With the below change the behavior of __vlan_put_tag changed slightly,
> it now sets the skb->protocol = vlan_proto. vlan_proto was not a field
> being set by fcoe_xmit, so the skb->protocol is now not being set to
> ETH_P_8021Q, as it should be.
>
> This patch converts fcoe_xmit to use the vlan_put_tag routine which
> will tag the skb and fcoe will continue to transmit fcoe skbs on the
> real net device.
>
> For reference, the below change was the one that altered the
> __vlan_put_tag behavior.
>
>    commit 86a9bad3ab6b6f858fd4443b48738cabbb6d094c
>    Author: Patrick McHardy <kaber@trash.net>
>    Date:   Fri Apr 19 02:04:30 2013 +0000
>
>        net: vlan: add protocol argument to packet tagging functions
>
>        Add a protocol argument to the VLAN packet tagging functions. In case of HW
>        tagging, we need that protocol available in the ndo_start_xmit functions,
>        so it is stored in a new field in the skb. The new field fits into a hole
>        (on 64 bit) and doesn't increase the sks's size.
>
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> Tested-by: Ross Brattain <ross.b.brattain@intel.com>
> ---
>   drivers/scsi/fcoe/fcoe.c |   12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index fc7bb1f..cb7ac9c 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1658,13 +1658,11 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
>   	skb->protocol = htons(ETH_P_FCOE);
>   	skb->priority = fcoe->priority;
>
> -	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;
> -	} else
> -		skb->dev = fcoe->netdev;
> +	/* must set skb->dev before calling vlan_put_tag */
> +	skb->dev = fcoe->realdev;

Should there be a check to see if a vlan tag is needed? I'm not sure
but is there ever a case where we really want to run over the real
device without any tag?

With this new block a tag will always be inserted perhaps a zero id tag.

> +	skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_dev_vlan_id(fcoe->netdev));

Also and likely more importantly I think this may do a bad pointer
dereference if fcoe->netdev is not a vlan dev. Just take a look at
vlan_dev_vlan_id in ./net/8021q/vlan_core.c

> +	if (!skb)
> +		return -ENOMEM;
>
>   	/* fill up mac and fcoe headers */
>   	eh = eth_hdr(skb);
>
> _______________________________________________
> fcoe-devel mailing list
> fcoe-devel@open-fcoe.org
> http://lists.open-fcoe.org/mailman/listinfo/fcoe-devel
>
Robert Love - June 18, 2013, 5:55 p.m.
On Mon 17 Jun 2013 03:48:37 PM PDT, John Fastabend wrote:
>
> On 6/13/2013 1:54 PM, Robert Love wrote:
>>
>> fcoe_xmit was coded such that it would skip the vlan net device/layer
>> and instead set some vlan flags and transmit on the real net device.
>> The real net device has code that would add the vlan tag for fcoe skbs.
>> This avoids some extra processing for data frames and provides a small
>> performance improvement.
>>
>> Since fcoe_xmit was not using the vlan net device, __vlan_put_tag
>> within the real net device's xmit routine was ultimately being
>> called to set the vlan tag.
>>
>> With the below change the behavior of __vlan_put_tag changed slightly,
>> it now sets the skb->protocol = vlan_proto. vlan_proto was not a field
>> being set by fcoe_xmit, so the skb->protocol is now not being set to
>> ETH_P_8021Q, as it should be.
>>
>> This patch converts fcoe_xmit to use the vlan_put_tag routine which
>> will tag the skb and fcoe will continue to transmit fcoe skbs on the
>> real net device.
>>
>> For reference, the below change was the one that altered the
>> __vlan_put_tag behavior.
>>
>> commit 86a9bad3ab6b6f858fd4443b48738cabbb6d094c
>> Author: Patrick McHardy <kaber@trash.net>
>> Date: Fri Apr 19 02:04:30 2013 +0000
>>
>> net: vlan: add protocol argument to packet tagging functions
>>
>> Add a protocol argument to the VLAN packet tagging functions.
>> In case of HW
>> tagging, we need that protocol available in the ndo_start_xmit
>> functions,
>> so it is stored in a new field in the skb. The new field fits
>> into a hole
>> (on 64 bit) and doesn't increase the sks's size.
>>
>> Signed-off-by: Robert Love <robert.w.love@intel.com>
>> Acked-by: Neil Horman <nhorman@tuxdriver.com>
>> Tested-by: Ross Brattain <ross.b.brattain@intel.com>
>> ---
>> drivers/scsi/fcoe/fcoe.c | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
>> index fc7bb1f..cb7ac9c 100644
>> --- a/drivers/scsi/fcoe/fcoe.c
>> +++ b/drivers/scsi/fcoe/fcoe.c
>> @@ -1658,13 +1658,11 @@ static int fcoe_xmit(struct fc_lport *lport,
>> struct fc_frame *fp)
>> skb->protocol = htons(ETH_P_FCOE);
>> skb->priority = fcoe->priority;
>>
>> - 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;
>> - } else
>> - skb->dev = fcoe->netdev;
>> + /* must set skb->dev before calling vlan_put_tag */
>> + skb->dev = fcoe->realdev;
>
>
> Should there be a check to see if a vlan tag is needed? I'm not sure
> but is there ever a case where we really want to run over the real
> device without any tag?
>
> With this new block a tag will always be inserted perhaps a zero id tag.
>
Good point. I don't want to change the behavior with this patch only 
adjust for the networking vlan changes.

>>
>> + skb = vlan_put_tag(skb, htons(ETH_P_8021Q),
>> vlan_dev_vlan_id(fcoe->netdev));
>
>
> Also and likely more importantly I think this may do a bad pointer
> dereference if fcoe->netdev is not a vlan dev. Just take a look at
> vlan_dev_vlan_id in ./net/8021q/vlan_core.c
>
Another good point.

I've re-spun the patch and I've addressed both of your concerns. I'll 
send it out now. I've basically gone back to the previous if/else 
statement. The change now is that when it's a VLAN device I use 
vlan_put_tag instead of setting vlan_tci directly.

Thanks, //Rob

Patch

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index fc7bb1f..cb7ac9c 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1658,13 +1658,11 @@  static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
 	skb->protocol = htons(ETH_P_FCOE);
 	skb->priority = fcoe->priority;
 
-	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;
-	} else
-		skb->dev = fcoe->netdev;
+	/* must set skb->dev before calling vlan_put_tag */
+	skb->dev = fcoe->realdev;
+	skb = vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_dev_vlan_id(fcoe->netdev));
+	if (!skb)
+		return -ENOMEM;
 
 	/* fill up mac and fcoe headers */
 	eh = eth_hdr(skb);