[PATCH net] vlan: pull on __vlan_insert_tag error path and fix csum correction

[PATCH net] vlan: pull on __vlan_insert_tag error path and fix csum correction

[Daniel Borkmann]

When __vlan_insert_tag() fails from skb_vlan_push() path due to the
skb_cow_head(), we need to undo the __skb_push() in the error path
as well that was done earlier to move skb->data pointer to mac header.

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.

Fixes: 93515d53b133 ("net: move vlan pop/push functions into common code")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 net/core/skbuff.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d04c2d1..e561f9f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4502,13 +4502,16 @@ int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
 		__skb_push(skb, offset);
 		err = __vlan_insert_tag(skb, skb->vlan_proto,
 					skb_vlan_tag_get(skb));
-		if (err)
+		if (err) {
+			__skb_pull(skb, offset);
 			return err;
+		}
+
 		skb->protocol = skb->vlan_proto;
 		skb->mac_len += VLAN_HLEN;
-		__skb_pull(skb, offset);
 
 		skb_postpush_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
+		__skb_pull(skb, offset);
 	}
 	__vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
 	return 0;
-- 
1.9.3

Re: [PATCH net] vlan: pull on __vlan_insert_tag error path and fix csum correction

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.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.

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...

Re: [PATCH net] vlan: pull on __vlan_insert_tag error path and fix csum correction

[Daniel Borkmann]

On 04/01/2016 09:00 PM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.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?

Re: [PATCH net] vlan: pull on __vlan_insert_tag error path and fix csum correction

[Daniel Borkmann]

On 04/01/2016 11:28 PM, Daniel Borkmann wrote:
> On 04/01/2016 09:00 PM, David Miller wrote:
>> From: Daniel Borkmann <daniel@iogearbox.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

Re: [PATCH net] vlan: pull on __vlan_insert_tag error path and fix csum correction

[Daniel Borkmann]

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@iogearbox.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).