Message-ID: <56FF0C84.5030300@iogearbox.net>
Date:	Sat, 02 Apr 2016 02:04:20 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	David Miller <davem@...emloft.net>
CC:	jiri@...nulli.us, alexei.starovoitov@...il.com, jesse@...nel.org,
	tom@...bertland.com, netdev@...r.kernel.org
Subject: Re: [PATCH net] vlan: pull on __vlan_insert_tag error path and fix
 csum correction

On 04/01/2016 11:28 PM, Daniel Borkmann wrote:
> On 04/01/2016 09:00 PM, David Miller wrote:
>> From: Daniel Borkmann <daniel@...earbox.net>
>> Date: Fri,  1 Apr 2016 11:41:03 +0200
>>
>>> Moreover, I noticed that when in the non-error path the __skb_pull()
>>> is done and the original offset to mac header was non-zero, we fixup
>>> from a wrong skb->data offset in the checksum complete processing.
>>>
>>> So the skb_postpush_rcsum() really needs to be done before __skb_pull()
>>> where skb->data still points to the mac header start.
>>
>> Ugh, what a mess, are you sure any of this is right even after your
>> change?  What happens (outside of the csum part) is this:
>>
>>     __skb_push(offset);
>>     __vlan_insert_tag(); {
>>         skb_push(VLAN_HLEN);
>>     ...
>>         memmove(skb->data, skb->data + VLAN_HLEN, 2 * ETH_ALEN);
>>     }
>>     __skb_pull(offset);
>>
>> If I understand this correctly, the last pull will therefore put
>> skb->data pointing at vlan_ethhdr->h_vlan_TCI of the new VLAN header
>> pushed by __vlan_insert_tag().
>>
>> That is assuming skb->data began right after the original ethernet
>> header.
>
> Yes, this is correct. Now, continuing this train of thought: you have
> skb->data pointing _currently_ at vlan_ethhdr->h_vlan_TCI.
>
> And then you call:
>
>    skb_postpush_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>
> So, we point from the ->vlan_TCI + (2 * ETH_ALEN) as start offset, and
> with VLAN_HLEN (= 4 bytes that we added) as length for the csum
> correction as input. So, we point way beyond what we actually wanted
> to fixup wrt csum, no?
>
> But what we actually want to sum is [h_vlan_proto + h_vlan_TCI], which
> is where above skb_postpush_rcsum() call points to _before_ the last
> __skb_pull() happens. In other words, still at that time, we have the
> same expectations as in __vlan_insert_tag().
>
>> To me, that postpull csum currently is absolutely in the correct spot,
>> because it's acting upon the pull done by __vlan_insert_tag(), not the
>> one done here by skb_vlan_push().
>>
>> Right?
>>
>> Can you tell me how you tested this?  Just curious...
>
> I noticed both while reviewing the code, the error path fixup is not
> critical for ovs or act_vlan as the skb is dropped afterwards, but not
> necessarily in eBPF case, so there it matters as eBPF doesn't know at
> this point, what the program is going to do with it (similar fixup is
> done in __skb_vlan_pop() error path, btw). For the csum, I did a hexdump
> to compare what we write and what is being passed in for the csum correction.
>
> Anyway ...
>
> Aside from all this and based on your comment, I'm investigating whether
> for the vlan push and also pop case the __skb_pull(skb, offset) in success
> case is actually enough and whether it needs to take VLAN_HLEN into account
> as well. But, I need to do more test for that one first. At least the skb_vlan_push()
> comment says "__vlan_insert_tag expect skb->data pointing to mac header.
> So change skb->data before calling it and change back to original position
> later", Jiri?

For this part, what is meant with "original" position (relative to the start
of the ethernet header [currently the case], or relative to some data, e.g.
before/after the call, I still expect skb->data position to point to my IP header)?

Thanks,
Daniel

Powered by blists - more mailing lists