| 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: Sat, 5 Aug 2023 16:25:09 +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
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
Powered by blists - more mailing lists