Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jay Vosburgh <jay.vosburgh@canonical.com>
To:     Sun Shouxin <sunshouxin@chinatelecom.cn>
cc:     vfalico@gmail.com, andy@greyhouse.net, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        ast@kernel.org, daniel@iogearbox.net, hawk@kernel.org,
        john.fastabend@gmail.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
        huyd12@chinatelecom.cn
Subject: Re: [PATCH] net:bonding:support balance-alb interface with vlan to bridge
In-reply-to: <20220805074556.70297-1-sunshouxin@chinatelecom.cn>
References: <20220805074556.70297-1-sunshouxin@chinatelecom.cn>
Comments: In-reply-to Sun Shouxin <sunshouxin@chinatelecom.cn>
   message dated "Fri, 05 Aug 2022 00:45:56 -0700."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3916.1659727127.1@famine>
Content-Transfer-Encoding: quoted-printable
Date:   Fri, 05 Aug 2022 12:18:47 -0700
Message-ID: <3917.1659727127@famine>
Precedence: bulk

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 =3D=3D 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_m=
ain.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(struc=
t sk_buff **pskb)
> 	struct sk_buff *skb =3D *pskb;
> 	struct slave *slave;
> 	struct bonding *bond;
>+	struct net_device *vlan;
> 	int (*recv_probe)(const struct sk_buff *, struct bonding *,
> 			  struct slave *);
> 	int ret =3D RX_HANDLER_ANOTHER;
>+	unsigned int headroom;
> =

> 	skb =3D skb_share_check(skb, GFP_ATOMIC);
> 	if (unlikely(!skb))
>@@ -1591,6 +1593,24 @@ static rx_handler_result_t bond_handle_frame(struc=
t sk_buff **pskb)
> 				  bond->dev->addr_len);
> 	}
> =

>+	if (skb_vlan_tag_present(skb)) {
>+		if (BOND_MODE(bond) =3D=3D BOND_MODE_ALB && skb->pkt_type =3D=3D PACKE=
T_HOST) {
>+			vlan =3D __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 =3D 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