| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 14 Jul 2023 17:18:44 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Jay Vosburgh <jay.vosburgh@...onical.com>
Cc: netdev@...r.kernel.org
Subject: Re: [Discuss] IPv4 packets lost with macvlan over bond alb
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
Powered by blists - more mailing lists