With a virtual machine behind a bridge on top of a bond, outgoing traffic
should retain the VM's source MAC. That works fine most of the time, until
doing a failover, and then the MAC gets rewritten to the bond slave's MAC,
and the return traffic gets dropped. If we don't rewrite the MAC there, we
don't lose any traffic.
Cc: Jay Vosburgh <[email protected]>
Cc: Veaceslav Falico <[email protected]>
Cc: Andy Gospodarek <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Thomas Davis <[email protected]>
Cc: [email protected]
Signed-off-by: Jarod Wilson <[email protected]>
---
drivers/net/bonding/bond_alb.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 3455f2cc13f2..ce8257c7cbea 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -1302,6 +1302,26 @@ void bond_alb_deinitialize(struct bonding *bond)
rlb_deinitialize(bond);
}
+static bool bond_alb_bridged_mac(struct bonding *bond, struct ethhdr *eth_data)
+{
+ struct list_head *iter;
+ struct slave *slave;
+
+ if (BOND_MODE(bond) != BOND_MODE_ALB)
+ return false;
+
+ /* Don't modify source MACs that do not originate locally
+ * (e.g.,arrive via a bridge).
+ */
+ if (!netif_is_bridge_port(bond->dev))
+ return false;
+
+ if (bond_slave_has_mac_rx(bond, eth_data->h_source))
+ return false;
+
+ return true;
+}
+
static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
struct slave *tx_slave)
{
@@ -1316,7 +1336,8 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
}
if (tx_slave && bond_slave_can_tx(tx_slave)) {
- if (tx_slave != rcu_access_pointer(bond->curr_active_slave)) {
+ if (tx_slave != rcu_access_pointer(bond->curr_active_slave) &&
+ !bond_alb_bridged_mac(bond, eth_data)) {
ether_addr_copy(eth_data->h_source,
tx_slave->dev->dev_addr);
}
--
2.30.2
Jarod Wilson <[email protected]> wrote:
>With a virtual machine behind a bridge on top of a bond, outgoing traffic
>should retain the VM's source MAC. That works fine most of the time, until
>doing a failover, and then the MAC gets rewritten to the bond slave's MAC,
>and the return traffic gets dropped. If we don't rewrite the MAC there, we
>don't lose any traffic.
Please have the log message here specify that this applies only
to balance-alb mode, and, the usual nomenclature for bonding patches is
"[PATCH] bonding:"; for this case, I'd suggest "balance-alb:" right
afterwards to be clear that it's only for alb mode. I didn't really
spot the "bond_alb" tag for what it was on first read, and it is the
only indication that this change is specific to alb mode other than the
patch itself.
>Cc: Jay Vosburgh <[email protected]>
>Cc: Veaceslav Falico <[email protected]>
>Cc: Andy Gospodarek <[email protected]>
>Cc: "David S. Miller" <[email protected]>
>Cc: Jakub Kicinski <[email protected]>
>Cc: Thomas Davis <[email protected]>
>Cc: [email protected]
>Signed-off-by: Jarod Wilson <[email protected]>
>---
> drivers/net/bonding/bond_alb.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index 3455f2cc13f2..ce8257c7cbea 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -1302,6 +1302,26 @@ void bond_alb_deinitialize(struct bonding *bond)
> rlb_deinitialize(bond);
> }
>
>+static bool bond_alb_bridged_mac(struct bonding *bond, struct ethhdr *eth_data)
>+{
>+ struct list_head *iter;
>+ struct slave *slave;
>+
>+ if (BOND_MODE(bond) != BOND_MODE_ALB)
>+ return false;
>+
>+ /* Don't modify source MACs that do not originate locally
>+ * (e.g.,arrive via a bridge).
>+ */
>+ if (!netif_is_bridge_port(bond->dev))
>+ return false;
I believe this logic will fail if the plumbing is, e.g., bond ->
vlan -> bridge, as netif_is_bridge_port() would not return true for the
bond in that case.
Making this reliable is tricky at best, and may be impossible to
be correct for all possible cases. As such, I think the comment above
should reflect the limited scope of what is actually being checked here
(i.e., the bond itself is directly a bridge port).
-J
>+
>+ if (bond_slave_has_mac_rx(bond, eth_data->h_source))
>+ return false;
>+
>+ return true;
>+}
>+
> static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> struct slave *tx_slave)
> {
>@@ -1316,7 +1336,8 @@ static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> }
>
> if (tx_slave && bond_slave_can_tx(tx_slave)) {
>- if (tx_slave != rcu_access_pointer(bond->curr_active_slave)) {
>+ if (tx_slave != rcu_access_pointer(bond->curr_active_slave) &&
>+ !bond_alb_bridged_mac(bond, eth_data)) {
> ether_addr_copy(eth_data->h_source,
> tx_slave->dev->dev_addr);
> }
>--
>2.30.2
>
---
-Jay Vosburgh, [email protected]