Re: [PATCH net 1/3] bonding: fix macvlan over alb bond support

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>The commit 14af9963ba1e ("bonding: Support macvlans on top of tlb/rlb mode
>bonds") aims to enable the use of macvlans on top of rlb bond mode. However,
>the current rlb bond mode only handles ARP packets to update remote neighbor
>entries. This causes an issue when a macvlan is on top of the bond, and
>remote devices send packets to the macvlan using the bond's MAC address
>as the destination. After delivering the packets to the macvlan, the macvlan
>will rejects them as the MAC address is incorrect. Consequently, this commit
>makes macvlan over bond non-functional.
>
>To address this problem, one potential solution is to check for the presence
>of a macvlan port on the bond device using netif_is_macvlan_port(bond->dev)
>and return NULL in the rlb_arp_xmit() function. However, this approach
>doesn't fully resolve the situation when a VLAN exists between the bond and
>macvlan.
>
>So let's just do a partial revert for commit 14af9963ba1e in rlb_arp_xmit().
>As the comment said, Don't modify or load balance ARPs that do not originate
>locally.
>
>Fixes: 14af9963ba1e ("bonding: Support macvlans on top of tlb/rlb mode bonds")
>Reported-by: susan.zheng@veritas.com
>Closes: https://bugzilla.redhat.com/show_bug.cgi?id=2117816
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/bonding/bond_alb.c |  4 ++--
> include/net/bonding.h          | 11 +----------
> 2 files changed, 3 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index b9dbad3a8af8..7765616107e5 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -661,9 +661,9 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, struct bonding *bond)
> 	arp = (struct arp_pkt *)skb_network_header(skb);
> 
> 	/* Don't modify or load balance ARPs that do not originate locally
>-	 * (e.g.,arrive via a bridge).
>+	 * (e.g.,arrive via a bridge or macvlan).
> 	 */

	I agree that this is best way to handle this case.  This might
be nitpicking, but I think the comment would be clearer if it didn't
give examples of what is excluded, but instead lists only the things
that are included.  Perhaps something like:

 	/* Don't modify or load balance ARPs that do not originate
	 * from the bond itself or a VLAN directly above the bond.
	 */

	-J

>-	if (!bond_slave_has_mac_rx(bond, arp->mac_src))
>+	if (!bond_slave_has_mac_rcu(bond, arp->mac_src))
> 		return NULL;
> 
> 	dev = ip_dev_find(dev_net(bond->dev), arp->ip_src);
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index 30ac427cf0c6..5b8b1b644a2d 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -722,23 +722,14 @@ static inline struct slave *bond_slave_has_mac(struct bonding *bond,
> }
> 
> /* Caller must hold rcu_read_lock() for read */
>-static inline bool bond_slave_has_mac_rx(struct bonding *bond, const u8 *mac)
>+static inline bool bond_slave_has_mac_rcu(struct bonding *bond, const u8 *mac)
> {
> 	struct list_head *iter;
> 	struct slave *tmp;
>-	struct netdev_hw_addr *ha;
> 
> 	bond_for_each_slave_rcu(bond, tmp, iter)
> 		if (ether_addr_equal_64bits(mac, tmp->dev->dev_addr))
> 			return true;
>-
>-	if (netdev_uc_empty(bond->dev))
>-		return false;
>-
>-	netdev_for_each_uc_addr(ha, bond->dev)
>-		if (ether_addr_equal_64bits(mac, ha->addr))
>-			return true;
>-
> 	return false;
> }
> 
>-- 
>2.41.0
>
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com