[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BANLkTikKZy09vsMZgoQy-cKS_XX53gqjkw@mail.gmail.com>
Date: Fri, 3 Jun 2011 07:18:10 +0800
From: Changli Gao <xiaosuo@...il.com>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: David Miller <davem@...emloft.net>,
shemminger@...ux-foundation.org, greearb@...delatech.com,
nicolas.2p.debian@...il.com, jpirko@...hat.com,
netdev@...r.kernel.org, kaber@...sh.net, fubar@...ibm.com,
eric.dumazet@...il.com, andy@...yhouse.net, jesse@...ira.com
Subject: Re: [PATCH] vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check v2
On Thu, Jun 2, 2011 at 11:26 PM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> Changli Gao <xiaosuo@...il.com> writes:
>
>> On Thu, Jun 2, 2011 at 9:03 PM, Eric W. Biederman <ebiederm@...ssion.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.
--
Regards,
Changli Gao(xiaosuo@...il.com)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists