bonding: prevent outgoing packets on inactive slaves
Subsystems (4)
RFC PATCH 1/1 bonding: prevent outgoing packets on inactive slaves
Applications exist that broadcast/multicast packets on all devices in the system (e.g. avahi-mdns).
When a device is an inactive slave of a bond in active-backup mode, its MAC address may be set identical to other divecies in the bond. The broadcast/multicast packets may then confuse switches to direct packets to the inactive slave, rather than the active one.
This patch makes sure the TX queues on inactive slaves are deactivated.
drivers/net/bonding/bonding.h +10 -0
@@ -20,6 +20,7 @@#include <linux/if_bonding.h> #include <linux/kobject.h> #include <linux/in6.h> +#include <net/sch_generic.h> #include "bond_3ad.h" #include "bond_alb.h"
@@ -291,12 +292,21 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave) slave->dev->priv_flags |=IFF_SLAVE_INACTIVE ;if (slave_do_arp_validate (bond, slave)) slave->dev->priv_flags |=IFF_SLAVE_NEEDARP ; + +/* prevent outgoing frames on inactive slaves from confusing switches */ +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) +dev_deactivate_tx (slave->dev); }static inline void bond_set_slave_active_flags (struct slave *slave) { +struct bonding *bond =netdev_priv (slave->dev->master); + slave->state =BOND_STATE_ACTIVE ; slave->dev->priv_flags &= ~(IFF_SLAVE_INACTIVE |IFF_SLAVE_NEEDARP ); + +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) +dev_activate_tx (slave->dev); }static inline void bond_set_master_3ad_flags (struct bonding *bond)
include/net/sch_generic.h +2 -0
@@ -296,6 +296,8 @@ extern void dev_init_scheduler(struct net_device *dev);extern void dev_shutdown (struct net_device *dev);extern void dev_activate (struct net_device *dev);extern void dev_deactivate (struct net_device *dev); +extern void dev_activate_tx (struct net_device *dev); +extern void dev_deactivate_tx (struct net_device *dev);extern void qdisc_reset (struct Qdisc *qdisc);extern void qdisc_destroy (struct Qdisc *qdisc);extern void qdisc_tree_decrease_qlen (struct Qdisc *qdisc,unsigned int n);
net/sched/sch_generic.c +16 -4
@@ -628,7 +628,7 @@ static void transition_one_qdisc(struct net_device *dev, } } -void dev_activate (struct net_device *dev) +void dev_activate_tx (struct net_device *dev) {int need_watchdog;
@@ -647,13 +647,19 @@ void dev_activate(struct net_device *dev) need_watchdog =0 ;netdev_for_each_tx_queue (dev, transition_one_qdisc, &need_watchdog); -transition_one_qdisc (dev, &dev->rx_queue,NULL );if (need_watchdog) { dev->trans_start = jiffies;dev_watchdog_up (dev); } } +EXPORT_SYMBOL (dev_activate_tx); + +void dev_activate (struct net_device *dev) +{ +dev_activate_tx (dev); +transition_one_qdisc (dev, &dev->rx_queue,NULL ); +} static void dev_deactivate_queue(struct net_device *dev, struct netdev_queue *dev_queue,
@@ -703,12 +709,18 @@ static bool some_qdisc_is_busy(struct net_device *dev)return false; } -void dev_deactivate (struct net_device *dev) +void dev_deactivate_tx (struct net_device *dev) {netdev_for_each_tx_queue (dev, dev_deactivate_queue, &noop_qdisc); -dev_deactivate_queue (dev, &dev->rx_queue, &noop_qdisc);dev_watchdog_down (dev); +} +EXPORT_SYMBOL (dev_deactivate_tx); + +void dev_deactivate (struct net_device *dev) +{ +dev_deactivate_tx (dev); +dev_deactivate_queue (dev, &dev->rx_queue, &noop_qdisc);/* Wait for outstanding qdisc-less dev_queue_xmit calls. */ synchronize_rcu ();
Signature · 2 lines
Jiri Bohac <[email protected]> SUSE Labs, SUSE CZ
Quoted 12 lines · Jiri Bohac
Applications exist that broadcast/multicast packets on all devices in the system (e.g. avahi-mdns).
When a device is an inactive slave of a bond in active-backup mode, its MAC address may be set identical to other divecies in the bond. The broadcast/multicast packets may then confuse switches to direct packets to the inactive slave, rather than the active one.
This patch makes sure the TX queues on inactive slaves are deactivated.
Wont this break ARP link monitoring ?
Quoted 105 lines
Signed-off-by Jiri Bohac <[email protected]>drivers/net/bonding/bonding.h +10
-0@@ -20,6 +20,7 @@#include <linux/if_bonding.h> #include <linux/kobject.h> #include <linux/in6.h> +#include <net/sch_generic.h> #include "bond_3ad.h" #include "bond_alb.h" @@ -291,12 +292,21 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave) slave->dev->priv_flags |=IFF_SLAVE_INACTIVE ;if (slave_do_arp_validate (bond, slave)) slave->dev->priv_flags |=IFF_SLAVE_NEEDARP ; + +/* prevent outgoing frames on inactive slaves from confusing switches */ +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) +dev_deactivate_tx (slave->dev); }static inline void bond_set_slave_active_flags (struct slave *slave) { +struct bonding *bond =netdev_priv (slave->dev->master); + slave->state =BOND_STATE_ACTIVE ; slave->dev->priv_flags &= ~(IFF_SLAVE_INACTIVE |IFF_SLAVE_NEEDARP ); + +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) +dev_activate_tx (slave->dev); }static inline void bond_set_master_3ad_flags (struct bonding *bond)include/net/sch_generic.h +2
-0@@ -296,6 +296,8 @@ extern void dev_init_scheduler(struct net_device *dev);extern void dev_shutdown (struct net_device *dev);extern void dev_activate (struct net_device *dev);extern void dev_deactivate (struct net_device *dev); +extern void dev_activate_tx (struct net_device *dev); +extern void dev_deactivate_tx (struct net_device *dev);extern void qdisc_reset (struct Qdisc *qdisc);extern void qdisc_destroy (struct Qdisc *qdisc);extern void qdisc_tree_decrease_qlen (struct Qdisc *qdisc,unsigned int n);net/sched/sch_generic.c +16
-4@@ -628,7 +628,7 @@ static void transition_one_qdisc(struct net_device *dev, } } -void dev_activate (struct net_device *dev) +void dev_activate_tx (struct net_device *dev) {int need_watchdog;@@ -647,13 +647,19 @@ void dev_activate(struct net_device *dev) need_watchdog =0 ;netdev_for_each_tx_queue (dev, transition_one_qdisc, &need_watchdog); -transition_one_qdisc (dev, &dev->rx_queue,NULL );if (need_watchdog) { dev->trans_start = jiffies;dev_watchdog_up (dev); } } +EXPORT_SYMBOL (dev_activate_tx); + +void dev_activate (struct net_device *dev) +{ +dev_activate_tx (dev); +transition_one_qdisc (dev, &dev->rx_queue,NULL ); +} static void dev_deactivate_queue(struct net_device *dev, struct netdev_queue *dev_queue,@@ -703,12 +709,18 @@ static bool some_qdisc_is_busy(struct net_device *dev)return false; } -void dev_deactivate (struct net_device *dev) +void dev_deactivate_tx (struct net_device *dev) {netdev_for_each_tx_queue (dev, dev_deactivate_queue, &noop_qdisc); -dev_deactivate_queue (dev, &dev->rx_queue, &noop_qdisc);dev_watchdog_down (dev); +} +EXPORT_SYMBOL (dev_deactivate_tx); + +void dev_deactivate (struct net_device *dev) +{ +dev_deactivate_tx (dev); +dev_deactivate_queue (dev, &dev->rx_queue, &noop_qdisc);/* Wait for outstanding qdisc-less dev_queue_xmit calls. */ synchronize_rcu ();
Quoted 6 lines · Eric Dumazet
Quoted 3 lines · Jiri Bohac
This patch makes sure the TX queues on inactive slaves are deactivated.
Wont this break ARP link monitoring ?
No, ARP monitoring sends the ARP requests from the active slave.
Signature · 2 lines
Jiri Bohac <[email protected]> SUSE Labs, SUSE CZ
Quoted 9 lines · Jiri Bohac
Quoted 5 lines · Eric Dumazet
Quoted 3 lines · Jiri Bohac
This patch makes sure the TX queues on inactive slaves are deactivated.
Wont this break ARP link monitoring ?
No, ARP monitoring sends the ARP requests from the active slave.
Yes, this makes sense now you say it :)
Thanks
Quoted 9 lines · Jiri Bohac
Applications exist that broadcast/multicast packets on all devices in the system (e.g. avahi-mdns).
When a device is an inactive slave of a bond in active-backup mode, its MAC address may be set identical to other divecies in the bond. The broadcast/multicast packets may then confuse switches to direct packets to the inactive slave, rather than the active one.
Quoted 4 lines
This patch makes sure the TX queues on inactive slaves are deactivated.
Signed-off-by Jiri Bohac <[email protected]>
I'd love to include this patch; many times I've tracked down
"bonding" problems to some errant dingus confusing the switch, but I think this patch will break some things, and therefore has to be a NAK.
Specifically, I suspect this will break users of some protocols
that intentionally (and legitimately) bind directly to the slave underneath bonding, LLDP for one. I'm fairly sure there are such users, because the inactive slave rx path was changed last year to permit explicit binds to the inactive slaves to receive packets that normally would be dropped:
commit 0d7a3681232f545c6a59f77e60f7667673ef0e93
Author: Joe Eykholt <[email protected]> Date: Wed Jul 2 18:22:01 2008 -0700
net/core: Allow certain receives on inactive slave.
Allow a packet_type that specifies the exact device to receive even on an inactive bonding slave devices. This is important for some L2 protocols such as LLDP and FCoE. This can eventually be used for the bonding special cases as well.
The fact that they're receiving on the inactive slave suggests
that there may be transmits on the same slave, and a quick read of the LLDP spec seems to agree. I'm also unsure of exactly how FCoE operates in this regard (whether it does anything that will break due to this patch).
Anybody have better information about LLDP or FCoE? -J
---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]
Quoted 19 lines · Jay Vosburgh
Quoted 2 lines · Jiri Bohac
This patch makes sure the TX queues on inactive slaves are deactivated.
I'd love to include this patch; many times I've tracked down
"bonding" problems to some errant dingus confusing the switch, but I think this patch will break some things, and therefore has to be a NAK.Specifically, I suspect this will break users of some protocols
that intentionally (and legitimately) bind directly to the slave underneath bonding, LLDP for one. I'm fairly sure there are such users, because the inactive slave rx path was changed last year to permit explicit binds to the inactive slaves to receive packets that normally would be dropped:commit
0d7a3681232f545c6a59f77e60f7667673ef0e93Author: Joe Eykholt <[email protected]> Date: Wed Jul 2 18:22:01 2008 -0700
Quoted 6 lines
net/core: Allow certain receives on inactive slave.
Allow a
packet_typethat specifies the exact device to receive even on an inactive bonding slave devices. This is important for some L2 protocols such as LLDP and FCoE. This can eventually be used for the bonding special cases as well.
OK. I checked FCoE and it really seems to bind to a specific interface. I checked openlldp and it does bind to individual interfaces specifically, so checking the ptype really seems like it should work. How about the following patch, then?
I think it even is cleaner than the original, because bond_set_slave_active_flags() really only sets flags instead of calling non-trivial functions while holding locks. If some protocol does not work with the ptype heuristics, it can easilly be "whitelisted" in skb_bond_should_drop_tx():
drivers/net/bonding/bond_main.c +2 -2
@@ -1955,7 +1955,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev) slave_dev->priv_flags &= ~(IFF_MASTER_8023AD |IFF_MASTER_ALB |IFF_SLAVE_INACTIVE |IFF_BONDING | -IFF_SLAVE_NEEDARP ); +IFF_SLAVE_NEEDARP |IFF_SLAVE_FILTER_TX );kfree (slave);
@@ -2081,7 +2081,7 @@ static int bond_release_all(struct net_device *bond_dev) } slave_dev->priv_flags &= ~(IFF_MASTER_8023AD |IFF_MASTER_ALB | -IFF_SLAVE_INACTIVE ); +IFF_SLAVE_INACTIVE |IFF_SLAVE_FILTER_TX );kfree (slave);
drivers/net/bonding/bonding.h +4 -1
@@ -291,12 +291,15 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave) slave->dev->priv_flags |=IFF_SLAVE_INACTIVE ;if (slave_do_arp_validate (bond, slave)) slave->dev->priv_flags |=IFF_SLAVE_NEEDARP ; +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) + slave->dev->priv_flags |=IFF_SLAVE_FILTER_TX ; }static inline void bond_set_slave_active_flags (struct slave *slave) { slave->state =BOND_STATE_ACTIVE ; - slave->dev->priv_flags &= ~(IFF_SLAVE_INACTIVE |IFF_SLAVE_NEEDARP ); + slave->dev->priv_flags &= ~(IFF_SLAVE_INACTIVE |IFF_SLAVE_NEEDARP | +IFF_SLAVE_FILTER_TX ); }static inline void bond_set_master_3ad_flags (struct bonding *bond)
include/linux/if.h +1 -0
@@ -70,6 +70,7 @@#define IFF_XMIT_DST_RELEASE 0x400/* dev_hard_start_xmit() is allowed to * release skb->dst */ +#define IFF_SLAVE_FILTER_TX 0x800/* filter tx on bonding slaves */ #define IF_GET_IFACE 0x0001/* for querying only */ #define IF_GET_PROTO 0x0002
net/core/dev.c +25 -0
@@ -1786,6 +1786,25 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,return netdev_get_tx_queue (dev, queue_index); } +/* In active-backup mode, on bonding slaves other than the currently active slave, +* suppress outgoing packets, except for special L2 protocols. +*/ +static inline int skb_bond_should_drop_tx (struct sk_buff *skb) +{ +struct packet_type *ptype; +__be16 type; + +/* allow protocols specifically bound to this interface */ + type = skb->protocol; +list_for_each_entry_rcu (ptype, + &ptype_base[ntohs (type) &PTYPE_HASH_MASK ], list) { +if (ptype->type == type && ptype->dev == skb->dev) +return 0 ; + } + +return 1 ; +} +/** * dev_queue_xmit - transmit a buffer * @skb: buffer to transmit
@@ -1818,6 +1837,12 @@ int dev_queue_xmit(struct sk_buff *skb)struct Qdisc *q;int rc = -ENOMEM ; +if ((dev->priv_flags &IFF_SLAVE_FILTER_TX ) && +skb_bond_should_drop_tx (skb)) { + rc =NET_XMIT_DROP ; + goto out_kfree_skb; + } +/* GSO will handle the following emulations directly. */ if (netif_needs_gso (dev, skb)) goto gso;
Signature · 2 lines
Jiri Bohac <[email protected]> SUSE Labs, SUSE CZ
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi,
after a small off-list (my fault, sorry) discussion with Jay, I am re-sending the patch with a minor modification. See below.
On Tue, Aug 25, 2009 at 10:36:22AM -0700, Jay Vosburgh wrote (off-list):
Quoted 12 lines
Review
Quoted 6 lines · Jiri Bohac
(quoted diff) +6
-0+ if (unlikely(dev->priv_flags & IFF_SLAVE_FILTER_TX) && + skb_bond_should_drop_tx(skb)) { + rc = NET_XMIT_DROP; + goto out_kfree_skb; + } +The
priv_flagstest is hidden inside theskb_bond_should_drop
that already exists, I see no reason to do this differently. The function is an inline, so in terms of the generated code, it should be about the same.
OK, fixed in the new version below.
Quoted 10 lines
Also, your patch won't prevent a VLAN configured directly above
the slave from transmitting. I mention this because I've occasionally seen configurations of the form:bond0 -> eth0.555 -> eth0 I.e., where the bonding slave is the VLAN interface, not theactual interface. The other bonding slave was on a different VLAN, if memory serves. I don't know if this is really an issue or not for your purpose.
Right, the patch won't prevent transmission from the real device on which a VLAN is configured. After some thinking, however, I am now convinced this is the right thing to do:
1) the problem can be prevented when setting up the bond. VLAN interfaces can have their MAC address changed, independently from the real device the VLAN is configured on (and from any other VLAN interfaces). The problem occurs when a VLAN interface, with a MAC address identical to the real device (or other VLAN interfaces) is added as the first slave to the bond, making the bond inherit this address and force it to subsequently enslaved devices. If a slave, other than the first VLAN, is then made the active slave, switches could be confused. The VLAN's MAC address can, however, easily be changed prior to enslaving the VLAN interface and the problem will then never occur.
2) filtering outgoing frames from the VLAN's real device could break legitimate traffic. If the network topology ensures that non-tagged (or tagged with a different VLAN id) frames going out from the VLAN interface never get on the same L2 network as the frames from the other bonding slaves, the setup can work well and filtering the frames will break that.
drivers/net/bonding/bond_main.c +2 -2
@@ -1955,7 +1955,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev) slave_dev->priv_flags &= ~(IFF_MASTER_8023AD |IFF_MASTER_ALB |IFF_SLAVE_INACTIVE |IFF_BONDING | -IFF_SLAVE_NEEDARP ); +IFF_SLAVE_NEEDARP |IFF_SLAVE_FILTER_TX );kfree (slave);
@@ -2081,7 +2081,7 @@ static int bond_release_all(struct net_device *bond_dev) } slave_dev->priv_flags &= ~(IFF_MASTER_8023AD |IFF_MASTER_ALB | -IFF_SLAVE_INACTIVE ); +IFF_SLAVE_INACTIVE |IFF_SLAVE_FILTER_TX );kfree (slave);
drivers/net/bonding/bonding.h +4 -1
@@ -291,12 +291,15 @@ static inline void bond_set_slave_inactive_flags(struct slave *slave) slave->dev->priv_flags |=IFF_SLAVE_INACTIVE ;if (slave_do_arp_validate (bond, slave)) slave->dev->priv_flags |=IFF_SLAVE_NEEDARP ; +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) + slave->dev->priv_flags |=IFF_SLAVE_FILTER_TX ; }static inline void bond_set_slave_active_flags (struct slave *slave) { slave->state =BOND_STATE_ACTIVE ; - slave->dev->priv_flags &= ~(IFF_SLAVE_INACTIVE |IFF_SLAVE_NEEDARP ); + slave->dev->priv_flags &= ~(IFF_SLAVE_INACTIVE |IFF_SLAVE_NEEDARP | +IFF_SLAVE_FILTER_TX ); }static inline void bond_set_master_3ad_flags (struct bonding *bond)
include/linux/if.h +1 -0
@@ -70,6 +70,7 @@#define IFF_XMIT_DST_RELEASE 0x400/* dev_hard_start_xmit() is allowed to * release skb->dst */ +#define IFF_SLAVE_FILTER_TX 0x800/* filter tx on bonding slaves */ #define IF_GET_IFACE 0x0001/* for querying only */ #define IF_GET_PROTO 0x0002
net/core/dev.c +27 -0
@@ -1786,6 +1786,28 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,return netdev_get_tx_queue (dev, queue_index); } +/* In active-backup mode, on bonding slaves other than the currently active slave, +* suppress outgoing packets, except for special L2 protocols. +*/ +static inline int skb_bond_should_drop_tx (struct sk_buff *skb) +{ +struct packet_type *ptype; +__be16 type; + +if (likely (!(skb->dev->priv_flags &IFF_SLAVE_FILTER_TX ))) +return 0 ; + +/* allow protocols specifically bound to this interface */ + type = skb->protocol; +list_for_each_entry_rcu (ptype, + &ptype_base[ntohs (type) &PTYPE_HASH_MASK ], list) { +if (ptype->type == type && ptype->dev == skb->dev) +return 0 ; + } + +return 1 ; +} +/** * dev_queue_xmit - transmit a buffer * @skb: buffer to transmit
@@ -1818,6 +1840,11 @@ int dev_queue_xmit(struct sk_buff *skb)struct Qdisc *q;int rc = -ENOMEM ; +if (skb_bond_should_drop_tx (skb)) { + rc =NET_XMIT_DROP ; + goto out_kfree_skb; + } +/* GSO will handle the following emulations directly. */ if (netif_needs_gso (dev, skb)) goto gso;
Signature · 2 lines
Jiri Bohac <[email protected]> SUSE Labs, SUSE CZ