[PATCH] net:bonding:support balance-alb interface with vlan to bridge

[PATCH] net:bonding:support balance-alb interface with vlan to bridge

[Sun Shouxin]

In my test, balance-alb bonding with two slaves eth0 and eth1,
and then Bond0.150 is created with vlan id attached bond0.
After adding bond0.150 into one linux bridge, I noted that Bond0,
bond0.150 and  bridge were assigned to the same MAC as eth0.
Once bond0.150 receives a packet whose dest IP is bridge's
and dest MAC is eth1's, the linux bridge cannot process it as expected.
The patch fix the issue, and diagram as below:

eth1(mac:eth1_mac)--bond0(balance-alb,mac:eth0_mac)--eth0(mac:eth0_mac)
      		      |
      		   bond0.150(mac:eth0_mac)
      		      |
      	           bridge(ip:br_ip, mac:eth0_mac)--other port

Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
---
 drivers/net/bonding/bond_main.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e75acb14d066..6210a9c7ca76 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1537,9 +1537,11 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
 	struct sk_buff *skb = *pskb;
 	struct slave *slave;
 	struct bonding *bond;
+	struct net_device *vlan;
 	int (*recv_probe)(const struct sk_buff *, struct bonding *,
 			  struct slave *);
 	int ret = RX_HANDLER_ANOTHER;
+	unsigned int headroom;
 
 	skb = skb_share_check(skb, GFP_ATOMIC);
 	if (unlikely(!skb))
@@ -1591,6 +1593,24 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
 				  bond->dev->addr_len);
 	}
 
+	if (skb_vlan_tag_present(skb)) {
+		if (BOND_MODE(bond) == BOND_MODE_ALB && skb->pkt_type == PACKET_HOST) {
+			vlan = __vlan_find_dev_deep_rcu(bond->dev, skb->vlan_proto,
+							skb_vlan_tag_get(skb) & VLAN_VID_MASK);
+			if (vlan) {
+				if (vlan->priv_flags & IFF_BRIDGE_PORT) {
+					headroom = skb->data - skb_mac_header(skb);
+					if (unlikely(skb_cow_head(skb, headroom))) {
+						kfree_skb(skb);
+						return RX_HANDLER_CONSUMED;
+					}
+					bond_hw_addr_copy(eth_hdr(skb)->h_dest, vlan->dev_addr,
+							  vlan->addr_len);
+				}
+			}
+		}
+	}
+
 	return ret;
 }
 
-- 
2.27.0

Re: [PATCH] net:bonding:support balance-alb interface with vlan to bridge

[Jay Vosburgh]

Sun Shouxin <sunshouxin@chinatelecom.cn> wrote:

>In my test, balance-alb bonding with two slaves eth0 and eth1,
>and then Bond0.150 is created with vlan id attached bond0.
>After adding bond0.150 into one linux bridge, I noted that Bond0,
>bond0.150 and  bridge were assigned to the same MAC as eth0.
>Once bond0.150 receives a packet whose dest IP is bridge's
>and dest MAC is eth1's, the linux bridge cannot process it as expected.
>The patch fix the issue, and diagram as below:
>
>eth1(mac:eth1_mac)--bond0(balance-alb,mac:eth0_mac)--eth0(mac:eth0_mac)
>      		      |
>      		   bond0.150(mac:eth0_mac)
>      		      |
>      	           bridge(ip:br_ip, mac:eth0_mac)--other port

	In principle, since 567b871e5033, the bond alb mode shouldn't be
load balancing incoming traffic for an IP address arriving via a bridge
configured above the bond.

	Looking at it, there's logic in rlb_arp_xmit to exclude the
bridge-over-bond case, but it relies on the MAC of traffic arriving via
the bridge being different from the bond's MAC.  I suspect this is
because 567b871e5033 was intended to manage traffic originating from
other bridge ports, and didn't consider the case of the bridge itself
when the bridge MAC equals the bond MAC.

	The bridge MAC will equal the bond MAC if the bond is the first
port added to the bridge, because the bridge will normally adopt the MAC
of the first port added (unless manually set to something else).

	I think the correct fix here is to update the test in
rlb_arp_xmit to properly exclude all bridge traffic (i.e., handle the
bridge MAC == bond MAC case), not to alter the destination MAC address
in incoming traffic.

	-J

>Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
>Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
>---
> drivers/net/bonding/bond_main.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e75acb14d066..6210a9c7ca76 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1537,9 +1537,11 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> 	struct sk_buff *skb = *pskb;
> 	struct slave *slave;
> 	struct bonding *bond;
>+	struct net_device *vlan;
> 	int (*recv_probe)(const struct sk_buff *, struct bonding *,
> 			  struct slave *);
> 	int ret = RX_HANDLER_ANOTHER;
>+	unsigned int headroom;
> 
> 	skb = skb_share_check(skb, GFP_ATOMIC);
> 	if (unlikely(!skb))
>@@ -1591,6 +1593,24 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
> 				  bond->dev->addr_len);
> 	}
> 
>+	if (skb_vlan_tag_present(skb)) {
>+		if (BOND_MODE(bond) == BOND_MODE_ALB && skb->pkt_type == PACKET_HOST) {
>+			vlan = __vlan_find_dev_deep_rcu(bond->dev, skb->vlan_proto,
>+							skb_vlan_tag_get(skb) & VLAN_VID_MASK);
>+			if (vlan) {
>+				if (vlan->priv_flags & IFF_BRIDGE_PORT) {
>+					headroom = skb->data - skb_mac_header(skb);
>+					if (unlikely(skb_cow_head(skb, headroom))) {
>+						kfree_skb(skb);
>+						return RX_HANDLER_CONSUMED;
>+					}
>+					bond_hw_addr_copy(eth_hdr(skb)->h_dest, vlan->dev_addr,
>+							  vlan->addr_len);
>+				}
>+			}
>+		}
>+	}
>+
> 	return ret;
> }
> 
>-- 
>2.27.0
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH] net:bonding:support balance-alb interface with vlan to bridge

[sunshouxin]

在 2022/8/6 3:18, Jay Vosburgh 写道:
> Sun Shouxin <sunshouxin@chinatelecom.cn> wrote:
>
>> In my test, balance-alb bonding with two slaves eth0 and eth1,
>> and then Bond0.150 is created with vlan id attached bond0.
>> After adding bond0.150 into one linux bridge, I noted that Bond0,
>> bond0.150 and  bridge were assigned to the same MAC as eth0.
>> Once bond0.150 receives a packet whose dest IP is bridge's
>> and dest MAC is eth1's, the linux bridge cannot process it as expected.
>> The patch fix the issue, and diagram as below:
>>
>> eth1(mac:eth1_mac)--bond0(balance-alb,mac:eth0_mac)--eth0(mac:eth0_mac)
>>       		      |
>>       		   bond0.150(mac:eth0_mac)
>>       		      |
>>       	           bridge(ip:br_ip, mac:eth0_mac)--other port
> 	In principle, since 567b871e5033, the bond alb mode shouldn't be
> load balancing incoming traffic for an IP address arriving via a bridge
> configured above the bond.
>
> 	Looking at it, there's logic in rlb_arp_xmit to exclude the
> bridge-over-bond case, but it relies on the MAC of traffic arriving via
> the bridge being different from the bond's MAC.  I suspect this is
> because 567b871e5033 was intended to manage traffic originating from
> other bridge ports, and didn't consider the case of the bridge itself
> when the bridge MAC equals the bond MAC.
>
> 	The bridge MAC will equal the bond MAC if the bond is the first
> port added to the bridge, because the bridge will normally adopt the MAC
> of the first port added (unless manually set to something else).
>
> 	I think the correct fix here is to update the test in
> rlb_arp_xmit to properly exclude all bridge traffic (i.e., handle the
> bridge MAC == bond MAC case), not to alter the destination MAC address
> in incoming traffic.
>
> 	-J


Thanks your warm instruction, I'll resend patch as your suggestion.

   -Sun


>
>> Suggested-by: Hu Yadi <huyd12@chinatelecom.cn>
>> Signed-off-by: Sun Shouxin <sunshouxin@chinatelecom.cn>
>> ---
>> drivers/net/bonding/bond_main.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e75acb14d066..6210a9c7ca76 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1537,9 +1537,11 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
>> 	struct sk_buff *skb = *pskb;
>> 	struct slave *slave;
>> 	struct bonding *bond;
>> +	struct net_device *vlan;
>> 	int (*recv_probe)(const struct sk_buff *, struct bonding *,
>> 			  struct slave *);
>> 	int ret = RX_HANDLER_ANOTHER;
>> +	unsigned int headroom;
>>
>> 	skb = skb_share_check(skb, GFP_ATOMIC);
>> 	if (unlikely(!skb))
>> @@ -1591,6 +1593,24 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb)
>> 				  bond->dev->addr_len);
>> 	}
>>
>> +	if (skb_vlan_tag_present(skb)) {
>> +		if (BOND_MODE(bond) == BOND_MODE_ALB && skb->pkt_type == PACKET_HOST) {
>> +			vlan = __vlan_find_dev_deep_rcu(bond->dev, skb->vlan_proto,
>> +							skb_vlan_tag_get(skb) & VLAN_VID_MASK);
>> +			if (vlan) {
>> +				if (vlan->priv_flags & IFF_BRIDGE_PORT) {
>> +					headroom = skb->data - skb_mac_header(skb);
>> +					if (unlikely(skb_cow_head(skb, headroom))) {
>> +						kfree_skb(skb);
>> +						return RX_HANDLER_CONSUMED;
>> +					}
>> +					bond_hw_addr_copy(eth_hdr(skb)->h_dest, vlan->dev_addr,
>> +							  vlan->addr_len);
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> 	return ret;
>> }
>>
>> -- 
>> 2.27.0
>>
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com