[PATCH net-next-2.6 0/3] bonding fixes

[PATCH net-next-2.6 0/3] bonding fixes

[Ben Hutchings]

I've been investigating a bonding bug that results in skb_under_panic()
on older kernels.  The bug is still present but with less disastrous
results.  In the course of my investigation I found two regressions
without actually looking very hard, which is somewhat worrying.

The fixes are:

1. Remove redundant VLAN tag insertion logic. Fixes a regression due to
VLAN handling changes in 2.6.37, which corrupts *all* outgoing traffic
on a VLAN over a bond when the slave does not implement VLAN tag
insertion.

2. Change active slave quietly when bond is down. Fixes a crash
introduced by 5a37e8ca8536c47871d46c82211f399adf06fd44 (bonding: rejoin
multicast groups on VLANs) and also cleans up existing code.

3. Fix mangled NAs on slaves without VLAN tag insertion. This bug
appears to have been present ever since gratuitous NAs were implemented
in 2.6.29. It doesn't look possible to fix without relying on the recent
VLAN handling changes. For stable/longterm and backports, I think the
safest thing to do is not to send a gratuitous NA if it would end up
mangled.

Ben.

Ben Hutchings (3):
  bonding/vlan: Remove redundant VLAN tag insertion logic
  bonding: Change active slave quietly when bond is down
  bonding/vlan: Fix mangled NAs on slaves without VLAN tag insertion

 drivers/net/bonding/bond_ipv6.c |    7 +++++-
 drivers/net/bonding/bond_main.c |   42 +++++++++-----------------------------
 2 files changed, 16 insertions(+), 33 deletions(-)

-- 
1.7.3.2


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6 0/3] bonding fixes

[Ben Hutchings]

These should of course go to net-2.6, not net-next-2.6.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6 0/3] bonding fixes

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 13 Dec 2010 18:18:03 +0000

> These should of course go to net-2.6, not net-next-2.6.

I'll apply these, thanks a lot Ben.

[PATCH net-2.6 1/3] bonding/vlan: Remove redundant VLAN tag insertion logic

[Ben Hutchings]

A bond may have a mixture of slave devices with and without hardware
VLAN tag insertion capability.  Therefore it always claims this
capability and performs software VLAN tag insertion if the slave does
not.

Since commit 7b9c60903714bf0a19d746b228864bad3497284e, this has
also been done by dev_hard_start_xmit().  The result is that VLAN-
tagged skbs are now double-tagged when transmitted through slave
devices without hardware VLAN tag insertion!

Remove the now-redundant logic from bond_dev_queue_xmit().

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/bonding/bond_main.c |   27 +--------------------------
 1 files changed, 1 insertions(+), 26 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d0ea760..ef370c9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -418,36 +418,11 @@ struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr)
  * @bond: bond device that got this skb for tx.
  * @skb: hw accel VLAN tagged skb to transmit
  * @slave_dev: slave that is supposed to xmit this skbuff
- *
- * When the bond gets an skb to transmit that is
- * already hardware accelerated VLAN tagged, and it
- * needs to relay this skb to a slave that is not
- * hw accel capable, the skb needs to be "unaccelerated",
- * i.e. strip the hwaccel tag and re-insert it as part
- * of the payload.
  */
 int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
 			struct net_device *slave_dev)
 {
-	unsigned short uninitialized_var(vlan_id);
-
-	/* Test vlan_list not vlgrp to catch and handle 802.1p tags */
-	if (!list_empty(&bond->vlan_list) &&
-	    !(slave_dev->features & NETIF_F_HW_VLAN_TX) &&
-	    vlan_get_tag(skb, &vlan_id) == 0) {
-		skb->dev = slave_dev;
-		skb = vlan_put_tag(skb, vlan_id);
-		if (!skb) {
-			/* vlan_put_tag() frees the skb in case of error,
-			 * so return success here so the calling functions
-			 * won't attempt to free is again.
-			 */
-			return 0;
-		}
-	} else {
-		skb->dev = slave_dev;
-	}
-
+	skb->dev = slave_dev;
 	skb->priority = 1;
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	if (unlikely(bond->dev->priv_flags & IFF_IN_NETPOLL)) {
-- 
1.7.3.2



-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-2.6 1/3] bonding/vlan: Remove redundant VLAN tag insertion logic

[Jay Vosburgh]

Ben Hutchings <bhutchings@solarflare.com> wrote:

>A bond may have a mixture of slave devices with and without hardware
>VLAN tag insertion capability.  Therefore it always claims this
>capability and performs software VLAN tag insertion if the slave does
>not.
>
>Since commit 7b9c60903714bf0a19d746b228864bad3497284e, this has
>also been done by dev_hard_start_xmit().  The result is that VLAN-
>tagged skbs are now double-tagged when transmitted through slave
>devices without hardware VLAN tag insertion!
>
>Remove the now-redundant logic from bond_dev_queue_xmit().
>
>Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

>---
> drivers/net/bonding/bond_main.c |   27 +--------------------------
> 1 files changed, 1 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index d0ea760..ef370c9 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -418,36 +418,11 @@ struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr)
>  * @bond: bond device that got this skb for tx.
>  * @skb: hw accel VLAN tagged skb to transmit
>  * @slave_dev: slave that is supposed to xmit this skbuff
>- *
>- * When the bond gets an skb to transmit that is
>- * already hardware accelerated VLAN tagged, and it
>- * needs to relay this skb to a slave that is not
>- * hw accel capable, the skb needs to be "unaccelerated",
>- * i.e. strip the hwaccel tag and re-insert it as part
>- * of the payload.
>  */
> int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb,
> 			struct net_device *slave_dev)
> {
>-	unsigned short uninitialized_var(vlan_id);
>-
>-	/* Test vlan_list not vlgrp to catch and handle 802.1p tags */
>-	if (!list_empty(&bond->vlan_list) &&
>-	    !(slave_dev->features & NETIF_F_HW_VLAN_TX) &&
>-	    vlan_get_tag(skb, &vlan_id) == 0) {
>-		skb->dev = slave_dev;
>-		skb = vlan_put_tag(skb, vlan_id);
>-		if (!skb) {
>-			/* vlan_put_tag() frees the skb in case of error,
>-			 * so return success here so the calling functions
>-			 * won't attempt to free is again.
>-			 */
>-			return 0;
>-		}
>-	} else {
>-		skb->dev = slave_dev;
>-	}
>-
>+	skb->dev = slave_dev;
> 	skb->priority = 1;
> #ifdef CONFIG_NET_POLL_CONTROLLER
> 	if (unlikely(bond->dev->priv_flags & IFF_IN_NETPOLL)) {
>-- 
>1.7.3.2
>
>
>
>-- 
>Ben Hutchings, Senior Software Engineer, Solarflare Communications
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-2.6 1/3] bonding/vlan: Remove redundant VLAN tag insertion logic

[Jesse Gross]

On Mon, Dec 13, 2010 at 11:58 AM, Jay Vosburgh <fubar@us.ibm.com> wrote:
> Ben Hutchings <bhutchings@solarflare.com> wrote:
>
>>A bond may have a mixture of slave devices with and without hardware
>>VLAN tag insertion capability.  Therefore it always claims this
>>capability and performs software VLAN tag insertion if the slave does
>>not.
>>
>>Since commit 7b9c60903714bf0a19d746b228864bad3497284e, this has
>>also been done by dev_hard_start_xmit().  The result is that VLAN-
>>tagged skbs are now double-tagged when transmitted through slave
>>devices without hardware VLAN tag insertion!
>>
>>Remove the now-redundant logic from bond_dev_queue_xmit().
>>
>>Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

Thanks for catching this.  I had this change but it was grouped in
with some other bonding/vlan simplifications that were dependent on
the rest of the drivers being converted.

Reviewed-by: Jesse Gross <jesse@nicira.com>

[PATCH net-2.6 2/3] bonding: Change active slave quietly when bond is down

[Ben Hutchings]

bond_change_active_slave() may be called when a slave is added, even
if the bond has not been brought up yet.  It may then attempt to send
packets, and further it may use mcast_work which is uninitialised
before the bond is brought up.  Add the necessary checks for
netif_running(bond->dev).

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/bonding/bond_main.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index ef370c9..3b16c34 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1178,11 +1178,13 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 				bond_do_fail_over_mac(bond, new_active,
 						      old_active);
 
-			bond->send_grat_arp = bond->params.num_grat_arp;
-			bond_send_gratuitous_arp(bond);
+			if (netif_running(bond->dev)) {
+				bond->send_grat_arp = bond->params.num_grat_arp;
+				bond_send_gratuitous_arp(bond);
 
-			bond->send_unsol_na = bond->params.num_unsol_na;
-			bond_send_unsolicited_na(bond);
+				bond->send_unsol_na = bond->params.num_unsol_na;
+				bond_send_unsolicited_na(bond);
+			}
 
 			write_unlock_bh(&bond->curr_slave_lock);
 			read_unlock(&bond->lock);
@@ -1196,8 +1198,9 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 
 	/* resend IGMP joins since active slave has changed or
 	 * all were sent on curr_active_slave */
-	if ((USES_PRIMARY(bond->params.mode) && new_active) ||
-	    bond->params.mode == BOND_MODE_ROUNDROBIN) {
+	if (((USES_PRIMARY(bond->params.mode) && new_active) ||
+	     bond->params.mode == BOND_MODE_ROUNDROBIN) &&
+	    netif_running(bond->dev)) {
 		bond->igmp_retrans = bond->params.resend_igmp;
 		queue_delayed_work(bond->wq, &bond->mcast_work, 0);
 	}
-- 
1.7.3.2



-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-2.6 2/3] bonding: Change active slave quietly when bond is down

[Jay Vosburgh]

Ben Hutchings <bhutchings@solarflare.com> wrote:

>bond_change_active_slave() may be called when a slave is added, even
>if the bond has not been brought up yet.  It may then attempt to send
>packets, and further it may use mcast_work which is uninitialised
>before the bond is brought up.  Add the necessary checks for
>netif_running(bond->dev).

	I wondered if it would be better to instead arrange for
bond_change_active_slave (and bond_select_active_slave, which calls it)
to not be called when the bond is down, but that would require quite a
bit of change to bond_enslave and bond_open.

	-J

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>


>Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
>---
> drivers/net/bonding/bond_main.c |   15 +++++++++------
> 1 files changed, 9 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index ef370c9..3b16c34 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1178,11 +1178,13 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> 				bond_do_fail_over_mac(bond, new_active,
> 						      old_active);
>
>-			bond->send_grat_arp = bond->params.num_grat_arp;
>-			bond_send_gratuitous_arp(bond);
>+			if (netif_running(bond->dev)) {
>+				bond->send_grat_arp = bond->params.num_grat_arp;
>+				bond_send_gratuitous_arp(bond);
>
>-			bond->send_unsol_na = bond->params.num_unsol_na;
>-			bond_send_unsolicited_na(bond);
>+				bond->send_unsol_na = bond->params.num_unsol_na;
>+				bond_send_unsolicited_na(bond);
>+			}
>
> 			write_unlock_bh(&bond->curr_slave_lock);
> 			read_unlock(&bond->lock);
>@@ -1196,8 +1198,9 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
>
> 	/* resend IGMP joins since active slave has changed or
> 	 * all were sent on curr_active_slave */
>-	if ((USES_PRIMARY(bond->params.mode) && new_active) ||
>-	    bond->params.mode == BOND_MODE_ROUNDROBIN) {
>+	if (((USES_PRIMARY(bond->params.mode) && new_active) ||
>+	     bond->params.mode == BOND_MODE_ROUNDROBIN) &&
>+	    netif_running(bond->dev)) {
> 		bond->igmp_retrans = bond->params.resend_igmp;
> 		queue_delayed_work(bond->wq, &bond->mcast_work, 0);
> 	}
>-- 
>1.7.3.2
>
>
>
>-- 
>Ben Hutchings, Senior Software Engineer, Solarflare Communications
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-2.6 2/3] bonding: Change active slave quietly when bond is down

[Ben Hutchings]

On Mon, 2010-12-13 at 12:41 -0800, Jay Vosburgh wrote:
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> >bond_change_active_slave() may be called when a slave is added, even
> >if the bond has not been brought up yet.  It may then attempt to send
> >packets, and further it may use mcast_work which is uninitialised
> >before the bond is brought up.  Add the necessary checks for
> >netif_running(bond->dev).
> 
> 	I wondered if it would be better to instead arrange for
> bond_change_active_slave (and bond_select_active_slave, which calls it)
> to not be called when the bond is down, but that would require quite a
> bit of change to bond_enslave and bond_open.
[...]

Yes, it did seem to me that it would be better to defer all these
actions until bond_open, but I thought that would be too risky at this
stage in the release cycle.  Please do clean this up further in
net-next-2.6.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-2.6 2/3] bonding: Change active slave quietly when bond is down

[Flavio Leitner]

On Mon, Dec 13, 2010 at 06:19:56PM +0000, Ben Hutchings wrote:
> bond_change_active_slave() may be called when a slave is added, even
> if the bond has not been brought up yet.  It may then attempt to send
> packets, and further it may use mcast_work which is uninitialised
> before the bond is brought up.  Add the necessary checks for
> netif_running(bond->dev).
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/bonding/bond_main.c |   15 +++++++++------
>  1 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index ef370c9..3b16c34 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1178,11 +1178,13 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
>  				bond_do_fail_over_mac(bond, new_active,
>  						      old_active);
>  
> -			bond->send_grat_arp = bond->params.num_grat_arp;
> -			bond_send_gratuitous_arp(bond);
> +			if (netif_running(bond->dev)) {
> +				bond->send_grat_arp = bond->params.num_grat_arp;
> +				bond_send_gratuitous_arp(bond);
>  
> -			bond->send_unsol_na = bond->params.num_unsol_na;
> -			bond_send_unsolicited_na(bond);
> +				bond->send_unsol_na = bond->params.num_unsol_na;
> +				bond_send_unsolicited_na(bond);
> +			}
>  
>  			write_unlock_bh(&bond->curr_slave_lock);
>  			read_unlock(&bond->lock);
> @@ -1196,8 +1198,9 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
>  
>  	/* resend IGMP joins since active slave has changed or
>  	 * all were sent on curr_active_slave */
> -	if ((USES_PRIMARY(bond->params.mode) && new_active) ||
> -	    bond->params.mode == BOND_MODE_ROUNDROBIN) {
> +	if (((USES_PRIMARY(bond->params.mode) && new_active) ||
> +	     bond->params.mode == BOND_MODE_ROUNDROBIN) &&
> +	    netif_running(bond->dev)) {
>  		bond->igmp_retrans = bond->params.resend_igmp;
>  		queue_delayed_work(bond->wq, &bond->mcast_work, 0);
>  	}
> -- 
> 1.7.3.2

I had a script randomly adding/removing slaves and setting
the bonding device up and down to test that commit, so
I'm surprised that it didn't catch this combination before.

Acked-by: Flavio Leitner <fleitner@redhat.com>

Thanks,
-- 
Flavio

[PATCH net-2.6 3/3] bonding/vlan: Fix mangled NAs on slaves without VLAN tag insertion

[Ben Hutchings]

bond_na_send() attempts to insert a VLAN tag in between building and
sending packets of the respective formats.  If the slave does not
implement hardware VLAN tag insertion then vlan_put_tag() will mangle
the network-layer header because the Ethernet header is not present at
this point (unlike in bond_arp_send()).

Fix this by adding the tag out-of-line and relying on
dev_hard_start_xmit() to insert it inline if necessary.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/bonding/bond_ipv6.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bonding/bond_ipv6.c b/drivers/net/bonding/bond_ipv6.c
index 121b073..84fbd4e 100644
--- a/drivers/net/bonding/bond_ipv6.c
+++ b/drivers/net/bonding/bond_ipv6.c
@@ -88,7 +88,12 @@ static void bond_na_send(struct net_device *slave_dev,
 	}
 
 	if (vlan_id) {
-		skb = vlan_put_tag(skb, vlan_id);
+		/* The Ethernet header is not present yet, so it is
+		 * too early to insert a VLAN tag.  Force use of an
+		 * out-of-line tag here and let dev_hard_start_xmit()
+		 * insert it if the slave hardware can't.
+		 */
+		skb = __vlan_hwaccel_put_tag(skb, vlan_id);
 		if (!skb) {
 			pr_err("failed to insert VLAN tag\n");
 			return;
-- 
1.7.3.2


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-2.6 3/3] bonding/vlan: Fix mangled NAs on slaves without VLAN tag insertion

[Jay Vosburgh]

Ben Hutchings <bhutchings@solarflare.com> wrote:

>bond_na_send() attempts to insert a VLAN tag in between building and
>sending packets of the respective formats.  If the slave does not
>implement hardware VLAN tag insertion then vlan_put_tag() will mangle
>the network-layer header because the Ethernet header is not present at
>this point (unlike in bond_arp_send()).
>
>Fix this by adding the tag out-of-line and relying on
>dev_hard_start_xmit() to insert it inline if necessary.
>
>Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

>---
> drivers/net/bonding/bond_ipv6.c |    7 ++++++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_ipv6.c b/drivers/net/bonding/bond_ipv6.c
>index 121b073..84fbd4e 100644
>--- a/drivers/net/bonding/bond_ipv6.c
>+++ b/drivers/net/bonding/bond_ipv6.c
>@@ -88,7 +88,12 @@ static void bond_na_send(struct net_device *slave_dev,
> 	}
>
> 	if (vlan_id) {
>-		skb = vlan_put_tag(skb, vlan_id);
>+		/* The Ethernet header is not present yet, so it is
>+		 * too early to insert a VLAN tag.  Force use of an
>+		 * out-of-line tag here and let dev_hard_start_xmit()
>+		 * insert it if the slave hardware can't.
>+		 */
>+		skb = __vlan_hwaccel_put_tag(skb, vlan_id);
> 		if (!skb) {
> 			pr_err("failed to insert VLAN tag\n");
> 			return;
>-- 
>1.7.3.2
>
>
>-- 
>Ben Hutchings, Senior Software Engineer, Solarflare Communications
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.
>

Re: [PATCH net-2.6 3/3] bonding/vlan: Fix mangled NAs on slaves without VLAN tag insertion

[Jesse Gross]

On Mon, Dec 13, 2010 at 10:20 AM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> bond_na_send() attempts to insert a VLAN tag in between building and
> sending packets of the respective formats.  If the slave does not
> implement hardware VLAN tag insertion then vlan_put_tag() will mangle
> the network-layer header because the Ethernet header is not present at
> this point (unlike in bond_arp_send()).
>
> Fix this by adding the tag out-of-line and relying on
> dev_hard_start_xmit() to insert it inline if necessary.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/bonding/bond_ipv6.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_ipv6.c b/drivers/net/bonding/bond_ipv6.c
> index 121b073..84fbd4e 100644
> --- a/drivers/net/bonding/bond_ipv6.c
> +++ b/drivers/net/bonding/bond_ipv6.c
> @@ -88,7 +88,12 @@ static void bond_na_send(struct net_device *slave_dev,
>        }
>
>        if (vlan_id) {
> -               skb = vlan_put_tag(skb, vlan_id);
> +               /* The Ethernet header is not present yet, so it is
> +                * too early to insert a VLAN tag.  Force use of an
> +                * out-of-line tag here and let dev_hard_start_xmit()
> +                * insert it if the slave hardware can't.
> +                */
> +               skb = __vlan_hwaccel_put_tag(skb, vlan_id);
>                if (!skb) {
>                        pr_err("failed to insert VLAN tag\n");
>                        return;

You could drop the check for !skb, since __vlan_hwaccel_put_tag() can
never fail.

Actually, my plan is to remove all current users of vlan_put_tag() and
replace them with __vlan_hwaccel_put_tag (and then rename that to
vlan_put_tag()).  There's really no reason why the upper layers should
have to care what the underlying device is capable of, now that
dev_hard_start_xmit() can handle it.

Reviewed-by: Jesse Gross <jesse@nicira.com>