Re: [Discuss] IPv4 packets lost with macvlan over bond alb

[Hangbin Liu <liuhangbin@gmail.com>]

On Fri, Jul 14, 2023 at 05:18:48PM +0800, Hangbin Liu wrote:
> Hi Jay,
> 
> Any thoughts for my last reply?

Ping

> 
> Thanks
> Hangbin
> On Sun, Jun 25, 2023 at 03:37:10PM +0800, Hangbin Liu wrote:
> > Hi Jay,
> > On Wed, Jun 21, 2023 at 06:42:10PM -0700, Jay Vosburgh wrote:
> > > 	By default, yes, VLANs use the same MAC, but may use a different
> > > MAC than the device the VLAN is configured above.  However, changing the
> > > VLAN's MAC address in a similar test case (VLAN above balance-alb bond)
> > > still works, because VLAN packets are delivered by matching the VLAN ID
> > > (via vlan_do_receive() -> vlan_find_dev()), not via the MAC address.
> > > 
> > > 	So, the RLB MAC edits done by rlb_arp_xmit() work in the sense
> > > that traffic flows, even though peers see a MAC address from the bond
> > > for the VLAN IP, not the VLAN's actual MAC address.
> > > 
> > > 	A bridge can also use a MAC address that differs from the bond,
> > > but rlb_arp_xmit() has a special case for bridge, and doesn't alter the
> > > ARP if the relevant IP address is on a bridge (so, no balancing).
> > > 
> > > 	Changing rlb_arp_xmit() to add macvlan to the bridge check makes
> > > the test case pass, e.g.,
> > > 
> > > diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> > > index b9dbad3a8af8..f720c419dfb7 100644
> > > --- a/drivers/net/bonding/bond_alb.c
> > > +++ b/drivers/net/bonding/bond_alb.c
> > > @@ -668,7 +668,7 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, struct bonding *bond)
> > >  
> > >  	dev = ip_dev_find(dev_net(bond->dev), arp->ip_src);
> > >  	if (dev) {
> > > -		if (netif_is_bridge_master(dev)) {
> > > +		if (netif_is_bridge_master(dev) || netif_is_macvlan(dev)) {
> > 
> > This is not enough. As a common usecase is to attach the macvlan to another
> > namespace(Apologise that my reproducer only has a comment, but no set the macvlan
> > to a separate namespace). So ip_dev_find() could not find the macvlan dev.
> > 
> > Using netif_is_macvlan_port() could make sure the bonding is under macvlan
> > devices. But what if there are middle devices between bond and macvlan? Maybe
> > we need to check each upper device?
> > 
> > If we want to skip arp xmit for macvlan, it looks like a partial revert of
> > 14af9963ba1e ("bonding: Support macvlans on top of tlb/rlb mode bonds"), which
> > means we can keep the TLB part and revert the RLB modification.
> > 
> > 
> > So my draft patch is:
> > 
> > diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> > index b9dbad3a8af8..c27f1a78a94b 100644
> > --- a/drivers/net/bonding/bond_alb.c
> > +++ b/drivers/net/bonding/bond_alb.c
> > @@ -663,7 +663,7 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, struct bonding *bond)
> >  	/* Don't modify or load balance ARPs that do not originate locally
> >  	 * (e.g.,arrive via a bridge).
> >  	 */
> > -	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);
> > @@ -960,7 +960,6 @@ static int alb_upper_dev_walk(struct net_device *upper,
> >  			      struct netdev_nested_priv *priv)
> >  {
> >  	struct alb_walk_data *data = (struct alb_walk_data *)priv->data;
> > -	bool strict_match = data->strict_match;
> >  	const u8 *mac_addr = data->mac_addr;
> >  	struct bonding *bond = data->bond;
> >  	struct slave *slave = data->slave;
> > @@ -979,10 +978,7 @@ static int alb_upper_dev_walk(struct net_device *upper,
> >  		}
> >  	}
> >  
> > -	/* If this is a macvlan device, then only send updates
> > -	 * when strict_match is turned off.
> > -	 */
> > -	if (netif_is_macvlan(upper) && !strict_match) {
> > +	if (netif_is_macvlan(upper)) {
> >  		tags = bond_verify_device_path(bond->dev, upper, 0);
> >  		if (IS_ERR_OR_NULL(tags))
> >  			BUG();
> > diff --git a/include/net/bonding.h b/include/net/bonding.h
> > index 59955ac33157..6e4e406d8cd2 100644
> > --- a/include/net/bonding.h
> > +++ b/include/net/bonding.h
> > @@ -724,23 +724,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;
> >  }
> >  
> > 
> > If we want to keep the macvlan support for ALB, as I said, the second way
> > is restore the macvlan mac address when receive message for macvlan port, e.g.:
> > I didn't test which way affect the performance more.
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index edbaa1444f8e..379d4c139b23 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -1585,6 +1585,36 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> >  				  bond->dev->addr_len);
> >  	}
> >  
> > +	if (BOND_MODE(bond) == BOND_MODE_ALB &&
> > +	    netif_is_macvlan_port(bond->dev) &&
> > +	    skb->pkt_type == PACKET_HOST) {
> > +		struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> > +		struct rlb_client_info *client_info;
> > +		u32 hash_index;
> > +
> > +		spin_lock_bh(&bond->mode_lock);
> > +
> > +		hash_index = bond_info->rx_hashtbl_used_head;
> > +		for (; hash_index != RLB_NULL_INDEX;
> > +		     hash_index = client_info->used_next) {
> > +
> > +			client_info = &(bond_info->rx_hashtbl[hash_index]);
> > +
> > +			if (ip_hdr(skb)->saddr == client_info->ip_dst &&
> > +			    ip_hdr(skb)->daddr == client_info->ip_src) {
> > +
> > +				if (unlikely(skb_cow_head(skb, skb->data - skb_mac_header(skb)))) {
> > +					kfree_skb(skb);
> > +					return RX_HANDLER_CONSUMED;
> > +				}
> > +				bond_hw_addr_copy(eth_hdr(skb)->h_dest,
> > +						  client_info->mac_src, ETH_ALEN);
> > +			}
> > +		}
> > +
> > +		spin_unlock_bh(&bond->mode_lock);
> > +	}
> > +
> >  	return ret;
> >  }
> >  
> > 
> > Thanks
> > Hangbin