| 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
| ||
|
Message-ID: <20090527084226.GB1652@psychotron.englab.brq.redhat.com>
Date: Wed, 27 May 2009 10:42:26 +0200
From: Jiri Pirko <jpirko@...hat.com>
To: Eric Dumazet <dada1@...mosbay.com>
Cc: netdev@...r.kernel.org, jgarzik@...ox.com, davem@...emloft.net,
shemminger@...ux-foundation.org, bridge@...ts.linux-foundation.org,
fubar@...ibm.com, bonding-devel@...ts.sourceforge.net,
kaber@...sh.net, mschmidt@...hat.com
Subject: Re: [PATCH net-next] bonding: allow bond in mode balance-alb to
work properly in bridge -try4.1
Tue, May 26, 2009 at 06:59:53PM CEST, dada1@...mosbay.com wrote:
>Jiri Pirko a écrit :
>> [PATCH net-next] bonding: allow bond in mode balance-alb to work properly in bridge -try4.1
>>
>> Hi all.
>>
>> The problem is described in following bugzilla:
>> https://bugzilla.redhat.com/show_bug.cgi?id=487763
>>
>> Basically here's what's going on. In every mode, bonding interface uses the same
>> mac address for all enslaved devices (except fail_over_mac). Only balance-alb
>> will simultaneously use multiple MAC addresses across different slaves. When you
>> put this kind of bond device into a bridge it will only add one of mac adresses
>> into a hash list of mac addresses, say X. This mac address is marked as local.
>> But this bonding interface also has mac address Y. Now then packet arrives with
>> destination address Y, this address is not marked as local and the packed looks
>> like it needs to be forwarded. This packet is then lost which is wrong.
>>
>> Notice that interfaces can be added and removed from bond while it is in bridge.
>>
>> ***
>> When the multiple addresses for bridge port approach failed to solve this issue
>> due to STP I started to think other way to solve this. I returned to previous
>> solution but tweaked one.
>>
>> This patch solves the situation in the bonding without touching bridge code.
>> For every incoming frame to bonding the destination address is compared to
>> current address of the slave device from which tha packet came. If these two
>> match destination address is replaced by mac address of the master. This address
>> is known by bridge so it is delivered properly.
>>
>> I experimentally tried that this works as good as searching through the slave
>> list (v4 of this patch).
>>
>> I was forced to create a new header because I need to use
>> compare_ether_addr_64bits() (defined in linux/etherdevice.h) in
>> linux/netdevice.h. I've hit some cross include issues. I think that it's good
>> to have skb_bond_should_drop() in a separate file anyway.
>>
>> Jirka
>>
>>
>> Signed-off-by: Jiri Pirko <jpirko@...hat.com>
>>
>> diff --git a/include/linux/bonding.h b/include/linux/bonding.h
>> new file mode 100644
>> index 0000000..3081ddb
>> --- /dev/null
>> +++ b/include/linux/bonding.h
>> @@ -0,0 +1,78 @@
>> +/*
>> + * include/linux/bonding.h
>> + *
>> + * Copyright (C) 2009 Jiri Pirko <jpirko@...hat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2
>> + * as published by the Free Software Foundation.
>> + *
>> + * Bonding device helpers.
>> + */
>> +
>> +#ifndef _LINUX_BONDING_H
>> +#define _LINUX_BONDING_H
>> +
>> +#ifdef __KERNEL__
>> +
>> +#include <linux/skbuff.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/if.h>
>> +#include <linux/etherdevice.h>
>> +#include <linux/if_ether.h>
>> +
>> +static inline void skb_bond_set_mac_by_master(struct sk_buff *skb,
>> + struct net_device *dev,
>> + struct net_device *master)
>> +{
>> + unsigned char *dest = eth_hdr(skb)->h_dest;
>> +
>> + if (compare_ether_addr_64bits(dest, master->dev_addr) &&
>> + !compare_ether_addr_64bits(dest, dev->dev_addr))
>> + memcpy(dest, master->dev_addr, ETH_ALEN);
>
>But couldnt we test skb->pkt_type == PACKET_HOST instead,
>Or eth_type_trans() not yet called at this point ?
>
>I would suggest :
>
>if (skb->pkt_type == PACKET_HOST)
> memcpy(dest, master->dev_addr, ETH_ALEN);
Yes Eric, you are right, good point.
eth_type_trans() is called in any driver before and
compare_ether_addr_64bits(dest, dev->dev_addr) is done there. So it's safe
to use this here (and save some ticks). I'm going to make a new patch and
test it.
Jirka
>
>> +}
>> +
>> +/* On bonding slaves other than the currently active slave, suppress
>> + * duplicates except for 802.3ad ETH_P_SLOW, alb non-mcast/bcast, and
>> + * ARP on active-backup slaves with arp_validate enabled.
>> + */
>> +static inline int skb_bond_should_drop(struct sk_buff *skb)
>> +{
>> + struct net_device *dev = skb->dev;
>> + struct net_device *master = dev->master;
>> +
>> + if (master) {
>> + if (master->priv_flags & IFF_MASTER_ARPMON)
>> + dev->last_rx = jiffies;
>> +
>> + if ((master->priv_flags & IFF_MASTER_ALB) && master->br_port) {
>> + /* Do address unmangle. The local destination address
>> + * will be always the one master has. Provides the right
>> + * functionality in a bridge.
>> + */
>> + skb_bond_set_mac_by_master(skb, dev, master);
>> + }
>> +
>> + if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
>> + if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
>> + skb->protocol == __cpu_to_be16(ETH_P_ARP))
>> + return 0;
>> +
>> + if (master->priv_flags & IFF_MASTER_ALB) {
>> + if (skb->pkt_type != PACKET_BROADCAST &&
>> + skb->pkt_type != PACKET_MULTICAST)
>> + return 0;
>> + }
>> + if (master->priv_flags & IFF_MASTER_8023AD &&
>> + skb->protocol == __cpu_to_be16(ETH_P_SLOW))
>> + return 0;
>> +
>> + return 1;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +#endif /* __KERNEL__ */
>> +
>> +#endif /* _LINUX_BONDING_H */
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index ae3c209..06e24ae 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1897,39 +1897,6 @@ static inline void netif_set_gso_max_size(struct net_device *dev,
>> dev->gso_max_size = size;
>> }
>>
>> -/* On bonding slaves other than the currently active slave, suppress
>> - * duplicates except for 802.3ad ETH_P_SLOW, alb non-mcast/bcast, and
>> - * ARP on active-backup slaves with arp_validate enabled.
>> - */
>> -static inline int skb_bond_should_drop(struct sk_buff *skb)
>> -{
>> - struct net_device *dev = skb->dev;
>> - struct net_device *master = dev->master;
>> -
>> - if (master) {
>> - if (master->priv_flags & IFF_MASTER_ARPMON)
>> - dev->last_rx = jiffies;
>> -
>> - if (dev->priv_flags & IFF_SLAVE_INACTIVE) {
>> - if ((dev->priv_flags & IFF_SLAVE_NEEDARP) &&
>> - skb->protocol == __cpu_to_be16(ETH_P_ARP))
>> - return 0;
>> -
>> - if (master->priv_flags & IFF_MASTER_ALB) {
>> - if (skb->pkt_type != PACKET_BROADCAST &&
>> - skb->pkt_type != PACKET_MULTICAST)
>> - return 0;
>> - }
>> - if (master->priv_flags & IFF_MASTER_8023AD &&
>> - skb->protocol == __cpu_to_be16(ETH_P_SLOW))
>> - return 0;
>> -
>> - return 1;
>> - }
>> - }
>> - return 0;
>> -}
>> -
>> extern struct pernet_operations __net_initdata loopback_net_ops;
>>
>> static inline int dev_ethtool_get_settings(struct net_device *dev,
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index 7f7de1a..c6eae40 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -2,6 +2,7 @@
>> #include <linux/netdevice.h>
>> #include <linux/if_vlan.h>
>> #include <linux/netpoll.h>
>> +#include <linux/bonding.h>
>> #include "vlan.h"
>>
>> /* VLAN rx hw acceleration helper. This acts like netif_{rx,receive_skb}(). */
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 241613f..221b43f 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -127,6 +127,7 @@
>> #include <linux/jhash.h>
>> #include <linux/random.h>
>> #include <trace/napi.h>
>> +#include <linux/bonding.h>
>>
>> #include "net-sysfs.h"
>>
>> --
>> 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
>>
>>
>
>
--
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
Powered by blists - more mailing lists