Patchwork vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check v2

login
register
mail settings
Submitter Eric W. Biederman
Date June 2, 2011, 3:26 p.m.
Message ID <m1ipsob6f8.fsf@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/98418/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - June 2, 2011, 3:26 p.m.
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.

--
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
Changli Gao - June 2, 2011, 11:18 p.m.
On Thu, Jun 2, 2011 at 11:26 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> 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.
>

OK. It is minor.

>>> +               /* 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.
>

OK, it is right. Thanks.
Jiri Pirko - June 6, 2011, 2:48 p.m.
Thu, Jun 02, 2011 at 05:26:51PM CEST, ebiederm@xmission.com wrote:
>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) {


This looks good to me. This does not need to be done before vlan_untag.

--
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

Patch

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) {