[ovs-dev] [PATCH 06/14] tunneling: Fix header parsing for ECN decapsulation.

Jesse Gross jesse at nicira.com
Fri Dec 3 02:54:22 UTC 2010


On Thu, Dec 2, 2010 at 6:19 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Thu, Dec 2, 2010 at 5:08 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Thu, Dec 2, 2010 at 3:45 PM, Ben Pfaff <blp at nicira.com> wrote:
>>> On Thu, Dec 02, 2010 at 12:36:55PM -0800, Jesse Gross wrote:
>>>> When copying ECN bits from the tunnel header to the inner packet,
>>>> the network header was not being properly located when there was
>>>> a vlan tag.  This simplies and fixes the header parsing to prevent
>>>> this problem.  It also removes a call to eth_type_trans() which
>>>> does several comparisions of MAC addresses which are not needed here
>>>> because we do not need to set skb->pkt_type.
>>>
>>> The eth_type_trans() elimination looks good to me, as long as we don't
>>> need the special case for IPX (I sure hope we don't).
>>
>> Yes, I didn't really want to put in the IPX hack.  I don't think it
>> actually matter though because nothing inside of OVS cares about the
>> difference between ETH_P_802_2 and ETH_P_802_3.  If we egress through
>> an internal device to the IP stack we actually will call
>> eth_type_trans().  A driver could see the difference if the packet
>> goes out through a netdev but I highly doubt any of them care.
>>
>>>
>>> In ecn_decapsulate(), we access the IP header (ip_hdr(skb)->tos) before
>>> we check that we can get at it with pskb_may_pull().  That doesn't look
>>> right to me, especially given that we are careful to check that we can
>>> pull an extra 4 bytes when there's an 802.1Q header.
>>
>> Thanks, you're right.  However, I also realized that I forgot what I
>> was trying to parse and completely screwed up this patch (for one
>> thing it can never do anything useful because it's actually looking at
>> the inner IP header).  I'm going to drop it for now and send out a
>> revised version later.
>
> OK.
>
> Or you could just drop the ecn_decapsulate() half and check in the
> eth_type_trans() half.  I don't think that these two parts are logically
> or technically connected.

Yeah, I'll break them apart when I fix it up.




More information about the dev mailing list