Re: [PATCH] vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check v2

[ebiederm@xmission.com (Eric W. Biederman)]

Changli Gao <xiaosuo@gmail.com> writes:

> On Thu, Jun 2, 2011 at 9:03 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> -static struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
>> +static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
>>  {
>> -       if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) {
>> -               if (skb_cow(skb, skb_headroom(skb)) < 0)
>> -                       skb = NULL;
>> -               if (skb) {
>> -                       /* Lifted from Gleb's VLAN code... */
>> -                       memmove(skb->data - ETH_HLEN,
>> -                               skb->data - VLAN_ETH_HLEN, 12);
>> -                       skb->mac_header += VLAN_HLEN;
>> -               }
>> +       if (skb_cow(skb, skb_headroom(skb)) < 0)
>> +               skb = NULL;
>> +       if (skb) {
>
> I think an else branch maybe more readable here.

Probably most readable would be simply returning NULL immediately on
error.  In this patch I just removed the if statement, and I would like
to avoid mixing different bug fixes in the same patch if possible.

>> +               /* Lifted from Gleb's VLAN code... */
>> +               memmove(skb->data - ETH_HLEN,
>> +                       skb->data - VLAN_ETH_HLEN, 12);
>> +               skb->mac_header += VLAN_HLEN;
>
> skb->mac_len should be adjusted too.

Given how vlan_untag is called at the moment it does appear
that the skb->mac_len = skb->network_header - skb->mac_header
in __netif_receive_skb that used to handle this for is no longer
doing this for us.

My feel is that either we need to do all of the header resets and the
vlan_untagging together.  So we either need this all together before or
after the another_round label:

So the proper fix is probably something like this.

diff --git a/net/core/dev.c b/net/core/dev.c
index bcb05cb..8fe50d4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3102,9 +3102,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 		skb->skb_iif = skb->dev->ifindex;
 	orig_dev = skb->dev;
 
-	skb_reset_network_header(skb);
-	skb_reset_transport_header(skb);
-	skb->mac_len = skb->network_header - skb->mac_header;
 
 	pt_prev = NULL;
 
@@ -3119,6 +3116,9 @@ another_round:
 		if (unlikely(!skb))
 			goto out;
 	}
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
+	skb->mac_len = skb->network_header - skb->mac_header;
 
 #ifdef CONFIG_NET_CLS_ACT
 	if (skb->tc_verd & TC_NCLS) {