| Message ID | 20230819090109.14467-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bonding: fix macvlan over alb bond support | expand |
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
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). */ - 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; }
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(-)