| 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
| ||
|
Message-ID: <ZJfuo6eyNbj29BcV@Laptop-X1>
Date: Sun, 25 Jun 2023 15:37:07 +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,
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