netdev - Re: [PATCH] vlan: Fix the ingress VLAN_FLAG_REORDER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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