Message-ID: <570167F4.9060901@iogearbox.net>
Date:	Sun, 03 Apr 2016 20:59:00 +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/02/2016 02:04 AM, Daniel Borkmann wrote:
> 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.

Fwiw, here, the current behavior looks good to me (e.g. when called with offset
zero, you really want to remain at mac header start).

Powered by blists - more mailing lists