netdev - Re: [Lxc-users] Bad checksums and lost packets with macvlan on dummy

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:	Wed, 02 Mar 2011 19:33:36 +0100
From:	Daniel Lezcano <daniel.lezcano@...e.fr>
To:	Patrick McHardy <kaber@...sh.net>
CC:	Eric Dumazet <eric.dumazet@...il.com>,
	Andrian Nord <nightnord@...il.com>,
	lxc-users@...ts.sourceforge.net,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [Lxc-users] Bad checksums and lost packets with macvlan on dummy

On 03/02/2011 07:03 PM, Patrick McHardy wrote:
> Am 02.03.2011 17:03, schrieb Daniel Lezcano:
>> On 03/02/2011 12:03 PM, Patrick McHardy wrote:
>>> Am 01.03.2011 21:04, schrieb Daniel Lezcano:
>>>> On 03/01/2011 05:51 PM, Patrick McHardy wrote:
>>>>>> Patrick, do you have any suggestions to fix this ?
>>>>> Since the frames are only looped back locally, I suppose the easiest
>>>>> fix would be to mark them with CHECKSUM_UNNECESSARY. Alternatively
>>>>> we need to complete the checksum manually, similar to what
>>>>> dev_hard_start_xmit() does.
>>>> That sounds very simple to fix, maybe too much simple :)
>>>>
>>>> I did the following change:
>>>>
>>>> --- linux-next.orig/drivers/net/macvlan.c
>>>> +++ linux-next/drivers/net/macvlan.c
>>>> @@ -222,6 +222,7 @@ static int macvlan_queue_xmit(struct sk_
>>>>
>>>>           if (vlan->mode == MACVLAN_MODE_BRIDGE) {
>>>>                   const struct ethhdr *eth = (void *)skb->data;
>>>> +               skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>>
>>>>                   /* send to other bridge ports directly */
>>>>                   if (is_multicast_ether_addr(eth->h_dest)) {
>>>>
>>>>
>>>> and that fixed the problem. Do you think it is acceptable ?
>>> The only problem I see is if the packets are bridged to a
>>> different networking device (or redirected using the mirred
>>> action), in this case the checksum will not be completed.
>>> This would be a very strange setup though and probably wouldn't
>>> be using dummy as lower device, so I'm not sure we have to
>>> worry about this case.
>> I am not sure to get it, do you say the patch is correct ?
> Its correct with a short-coming that doesn't seem to matter.
>
>> If my understanding is correct, the packet will be flagged
>> CHECKSUM_UNNECESSARY only for the macvlan devices, right ?
> Only for packets bridged between macvlan devices. A setup like
> the following would cause problems:
>
>                          br0
>                           |
>                     .----------.
>                     |          |
> macvlan0	macvlan1    eth0
>     |               |
>      -------.-------
> 	 dummy0
>
> In this case packets sent from macvlan0 will show up on
> eth0 with incorrect setups. However this setup doesn't
> seem realistic to me, you would simply use eth0 instead
> of dummy0.

Ok, I understand. thanks for the clarification.

>> By the way, this problem occurs for any lower device with offloading
>> capabilities with a macvlan port in bridge mode.
> True. This doesn't affect outgoing packets since their checksum
> will be completed in dev_hard_start_xmit(), but it affects
> packets bridged between macvlans.

One last question. In the case of broadcast packets with maclvan in 
bridge mode.
We will have the packets going through each macvlan port and also to the 
lower-device, right ?
For the latter, don't we have a problem if the packet is flagged 
CHECKSUM_UNNECESSARY ?

Shouldn't we restore the ip_summed field before sending through 
dev_queue_xmit ?

---
  drivers/net/macvlan.c |    3 +++
  1 file changed, 3 insertions(+)

Index: linux-next/drivers/net/macvlan.c
===================================================================
--- linux-next.orig/drivers/net/macvlan.c
+++ linux-next/drivers/net/macvlan.c
@@ -222,10 +222,13 @@ static int macvlan_queue_xmit(struct sk_

         if (vlan->mode == MACVLAN_MODE_BRIDGE) {
                 const struct ethhdr *eth = (void *)skb->data;
+               __u8 ip_summed = skb->ip_summed;
+               skb->ip_summed; = CHECKSUM_UNNECESSARY;

                 /* send to other bridge ports directly */
                 if (is_multicast_ether_addr(eth->h_dest)) {
                         macvlan_broadcast(skb, port, dev, 
MACVLAN_MODE_BRIDGE);
+                       skb->ip_summed = ip_summed;
                         goto xmit_world;
                 }




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