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

[Hangbin Liu <liuhangbin@gmail.com>]

Hi Jay,

Any thoughts for my last reply?

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