Re: [PATCH] bonding: fix bond 6 mode change MAC of arp reply from vif to cause Domu's network unreachable intermittently

[Jay Vosburgh <fubar@us.ibm.com>]

Zheng Li <zheng.x.li@oracle.com> wrote:

>This is a fix for a bug in bond_alb.c
>Rate of reproduced:100%
>Scenario: set Dom0 to bond 6 mode, Domu communicate with Dom0 through vif which
>is in bridge mode. The Dom0's bridge of xenbr0 contains vif and bond0, bond0
>contains eth0 and eth1. You can just need to ping a host which is in same LAN on
>Domu, some of packets will be lost intermittently.
>Analyse: When Dom0 set bond mode to 6, the bond alb will change MAC of every arp
>reply in rlb_arp_xmit function to affect receive packets, it is ok for normal
>NIC, but it's wrong to Domu, when Domu send arp reply through vif of Dom0, bond
>of alb replace Domu's MAC in arp reply with NIC's MAC address, that will cause
>remote host send packets to Domu using real NIC's MAC instead of Domu's MAC. Domu
>can't receive the packets whose dst MAC is not Domu's MAC.

	If I understand correctly, the issue really isn't about Dom0 /
Domu specifically, but rather that ARP traffic passing through a bridge
and out via the bond (when the bond is a port of the bridge) should not
have its source MAC address adjusted by the receive load balance code in
rlb_arp_xmit.

	Is my understanding correct?

	If so, then the patch in principle seems reasonable (although it
could be coded without the sentinel; more on that in a minute), but the
subject and description aren't really an accurate description of the
root problem (just one manifestation of it).  A better subject might be
something like "rlb mode should not alter ARP replies originating via
bridge."

>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Andy Gospodarek <andy@greyhouse.net>
>Cc: "David S. Miller" <davem@davemloft.net>
>Signed-off-by: Zheng Li <zheng.x.li@oracle.com>
>
>---
> drivers/net/bonding/bond_alb.c |   13 ++++++++++++-
> 1 files changed, 12 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index e15cc11..d6b134a 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -700,7 +700,18 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, struct bonding *bond)
> 		*/
> 		tx_slave = rlb_choose_channel(skb, bond);
> 		if (tx_slave) {
>-			memcpy(arp->mac_src,tx_slave->dev->dev_addr, ETH_ALEN);
>+			struct slave *tmp_slave = NULL;
>+			int i = 0, found_mac = 0;
>+			bond_for_each_slave(bond, tmp_slave, i) {
>+				if (ether_addr_equal_64bits(arp->mac_src,
>+						tmp_slave->dev->dev_addr)) {
>+					found_mac = 1;
>+					break;
>+				}
>+			}
>+			if (found_mac)
>+				memcpy(arp->mac_src, tx_slave->dev->dev_addr,
>+					ETH_ALEN);

	I think this could be coded as:

			bond_for_each_slave(bond, tmp_slave, i) {
				if (ether_addr_equal_64bits(arp->mac_src,
						tmp_slave->dev->dev_addr)) {
					memcpy(arp->mac_src,
					       tx_slave->dev->dev_addr,
					       ETH_ALEN);
					break;
				}
			}

	or even create a "bond_slave_has_mac" helper, since this loop is
something done several times in the code, e.g.,

struct slave *bond_slave_has_mac(struct bonding *bond, const u8 *mac)
{
	int i = 0;
	struct slave *tmp;

	bond_for_each_slave(bond, tmp, i)
		if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
			return tmp;

	return NULL;
}

	then your code would be

		if (tx_slave) {
			struct slave *tmp;

			/* Only modify ARP's MAC if it originates locally;
			 * don't change ARPs arriving via a bridge.
			 */
			tmp = bond_slave_has_mac(bond, arp->mac_src);
			if (tmp)
				memcpy(arp->mac_src, tx_slave->dev->dev_addr,
				       ETH_ALEN);

	... and so on.  I haven't tested any of the above, I'm just
scribbling it out into email, but it seems reasonable.

> 		}
> 		pr_debug("Server sent ARP Reply packet\n");
> 	} else if (arp->op_code == htons(ARPOP_REQUEST)) {
>-- 
>1.7.6.5

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com