[PATCH net] bonding: don't set oif to bond dev when getting NS target destination

[PATCH net] bonding: don't set oif to bond dev when getting NS target destination

[Hangbin Liu]

Unlike IPv4, IPv6 routing strictly requires the source address to be valid
on the outgoing interface. If the NS target is set to a remote VLAN interface,
and the source address is also configured on a VLAN over a bond interface,
setting the oif to the bond device will fail to retrieve the correct
destination route.

Fix this by not setting the oif to the bond device when retrieving the NS
target destination. This allows the correct destination device (the VLAN
interface) to be determined, so that bond_verify_device_path can return the
proper VLAN tags for sending NS messages.

Reported-by: David Wilder <wilder@us.ibm.com>
Closes: https://lore.kernel.org/netdev/aGOKggdfjv0cApTO@fedora/
Suggested-by: Jay Vosburgh <jv@jvosburgh.net>
Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c               |  1 -
 .../drivers/net/bonding/bond_options.sh       | 59 +++++++++++++++++++
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 257333c88710..30cf97f4e814 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3355,7 +3355,6 @@ static void bond_ns_send_all(struct bonding *bond, struct slave *slave)
 		/* Find out through which dev should the packet go */
 		memset(&fl6, 0, sizeof(struct flowi6));
 		fl6.daddr = targets[i];
-		fl6.flowi6_oif = bond->dev->ifindex;
 
 		dst = ip6_route_output(dev_net(bond->dev), NULL, &fl6);
 		if (dst->error) {
diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
index 7bc148889ca7..b3eb8a919c71 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
@@ -7,6 +7,7 @@ ALL_TESTS="
 	prio
 	arp_validate
 	num_grat_arp
+	vlan_over_bond
 "
 
 lib_dir=$(dirname "$0")
@@ -376,6 +377,64 @@ num_grat_arp()
 	done
 }
 
+vlan_over_bond_arp()
+{
+	local mode="$1"
+	RET=0
+
+	bond_reset "mode $mode arp_interval 100 arp_ip_target 192.0.3.10"
+	ip -n "${s_ns}" link add bond0.3 link bond0 type vlan id 3
+	ip -n "${s_ns}" link set bond0.3 up
+	ip -n "${s_ns}" addr add 192.0.3.1/24 dev bond0.3
+	ip -n "${s_ns}" addr add 2001:db8::3:1/64 dev bond0.3
+
+	slowwait_for_counter 5 5 tc_rule_handle_stats_get \
+		"dev eth0.3 ingress" 101 ".packets" "-n ${c_ns}" || RET=1
+	log_test "vlan over bond arp" "$mode"
+}
+
+vlan_over_bond_ns()
+{
+	local mode="$1"
+	RET=0
+
+	if skip_ns; then
+		log_test_skip "vlan_over_bond ns" "$mode"
+		return 0
+	fi
+
+	bond_reset "mode $mode arp_interval 100 ns_ip6_target 2001:db8::3:10"
+	ip -n "${s_ns}" link add bond0.3 link bond0 type vlan id 3
+	ip -n "${s_ns}" link set bond0.3 up
+	ip -n "${s_ns}" addr add 192.0.3.1/24 dev bond0.3
+	ip -n "${s_ns}" addr add 2001:db8::3:1/64 dev bond0.3
+
+	slowwait_for_counter 5 5 tc_rule_handle_stats_get \
+		"dev eth0.3 ingress" 102 ".packets" "-n ${c_ns}" || RET=1
+	log_test "vlan over bond ns" "$mode"
+}
+
+vlan_over_bond()
+{
+	# add vlan 3 for client
+	ip -n "${c_ns}" link add eth0.3 link eth0 type vlan id 3
+	ip -n "${c_ns}" link set eth0.3 up
+	ip -n "${c_ns}" addr add 192.0.3.10/24 dev eth0.3
+	ip -n "${c_ns}" addr add 2001:db8::3:10/64 dev eth0.3
+
+	# Add tc rule to check the vlan pkts
+	tc -n "${c_ns}" qdisc add dev eth0.3 clsact
+	tc -n "${c_ns}" filter add dev eth0.3 ingress protocol arp \
+		handle 101 flower skip_hw arp_op request \
+		arp_sip 192.0.3.1 arp_tip 192.0.3.10 action pass
+	tc -n "${c_ns}" filter add dev eth0.3 ingress protocol ipv6 \
+		handle 102 flower skip_hw ip_proto icmpv6 \
+		type 135 src_ip 2001:db8::3:1 action pass
+
+	vlan_over_bond_arp "active-backup"
+	vlan_over_bond_ns "active-backup"
+}
+
 trap cleanup EXIT
 
 setup_prepare
-- 
2.50.1

Re: [PATCH net] bonding: don't set oif to bond dev when getting NS target destination

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Unlike IPv4, IPv6 routing strictly requires the source address to be valid
>on the outgoing interface. If the NS target is set to a remote VLAN interface,
>and the source address is also configured on a VLAN over a bond interface,
>setting the oif to the bond device will fail to retrieve the correct
>destination route.
>
>Fix this by not setting the oif to the bond device when retrieving the NS
>target destination. This allows the correct destination device (the VLAN
>interface) to be determined, so that bond_verify_device_path can return the
>proper VLAN tags for sending NS messages.
>
>Reported-by: David Wilder <wilder@us.ibm.com>
>Closes: https://lore.kernel.org/netdev/aGOKggdfjv0cApTO@fedora/
>Suggested-by: Jay Vosburgh <jv@jvosburgh.net>
>Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")

	Does the test update part of this patch apply all the way back
to the oldest longterm kernel after 4e24be018eb9?  I think that's 6.1
right now.

	Generically, I'm wondering if test updates should be separate
patches from the functional changes as a general policy.

	-J

>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/bonding/bond_main.c               |  1 -
> .../drivers/net/bonding/bond_options.sh       | 59 +++++++++++++++++++
> 2 files changed, 59 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 257333c88710..30cf97f4e814 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3355,7 +3355,6 @@ static void bond_ns_send_all(struct bonding *bond, struct slave *slave)
> 		/* Find out through which dev should the packet go */
> 		memset(&fl6, 0, sizeof(struct flowi6));
> 		fl6.daddr = targets[i];
>-		fl6.flowi6_oif = bond->dev->ifindex;
> 
> 		dst = ip6_route_output(dev_net(bond->dev), NULL, &fl6);
> 		if (dst->error) {
>diff --git a/tools/testing/selftests/drivers/net/bonding/bond_options.sh b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
>index 7bc148889ca7..b3eb8a919c71 100755
>--- a/tools/testing/selftests/drivers/net/bonding/bond_options.sh
>+++ b/tools/testing/selftests/drivers/net/bonding/bond_options.sh
>@@ -7,6 +7,7 @@ ALL_TESTS="
> 	prio
> 	arp_validate
> 	num_grat_arp
>+	vlan_over_bond
> "
> 
> lib_dir=$(dirname "$0")
>@@ -376,6 +377,64 @@ num_grat_arp()
> 	done
> }
> 
>+vlan_over_bond_arp()
>+{
>+	local mode="$1"
>+	RET=0
>+
>+	bond_reset "mode $mode arp_interval 100 arp_ip_target 192.0.3.10"
>+	ip -n "${s_ns}" link add bond0.3 link bond0 type vlan id 3
>+	ip -n "${s_ns}" link set bond0.3 up
>+	ip -n "${s_ns}" addr add 192.0.3.1/24 dev bond0.3
>+	ip -n "${s_ns}" addr add 2001:db8::3:1/64 dev bond0.3
>+
>+	slowwait_for_counter 5 5 tc_rule_handle_stats_get \
>+		"dev eth0.3 ingress" 101 ".packets" "-n ${c_ns}" || RET=1
>+	log_test "vlan over bond arp" "$mode"
>+}
>+
>+vlan_over_bond_ns()
>+{
>+	local mode="$1"
>+	RET=0
>+
>+	if skip_ns; then
>+		log_test_skip "vlan_over_bond ns" "$mode"
>+		return 0
>+	fi
>+
>+	bond_reset "mode $mode arp_interval 100 ns_ip6_target 2001:db8::3:10"
>+	ip -n "${s_ns}" link add bond0.3 link bond0 type vlan id 3
>+	ip -n "${s_ns}" link set bond0.3 up
>+	ip -n "${s_ns}" addr add 192.0.3.1/24 dev bond0.3
>+	ip -n "${s_ns}" addr add 2001:db8::3:1/64 dev bond0.3
>+
>+	slowwait_for_counter 5 5 tc_rule_handle_stats_get \
>+		"dev eth0.3 ingress" 102 ".packets" "-n ${c_ns}" || RET=1
>+	log_test "vlan over bond ns" "$mode"
>+}
>+
>+vlan_over_bond()
>+{
>+	# add vlan 3 for client
>+	ip -n "${c_ns}" link add eth0.3 link eth0 type vlan id 3
>+	ip -n "${c_ns}" link set eth0.3 up
>+	ip -n "${c_ns}" addr add 192.0.3.10/24 dev eth0.3
>+	ip -n "${c_ns}" addr add 2001:db8::3:10/64 dev eth0.3
>+
>+	# Add tc rule to check the vlan pkts
>+	tc -n "${c_ns}" qdisc add dev eth0.3 clsact
>+	tc -n "${c_ns}" filter add dev eth0.3 ingress protocol arp \
>+		handle 101 flower skip_hw arp_op request \
>+		arp_sip 192.0.3.1 arp_tip 192.0.3.10 action pass
>+	tc -n "${c_ns}" filter add dev eth0.3 ingress protocol ipv6 \
>+		handle 102 flower skip_hw ip_proto icmpv6 \
>+		type 135 src_ip 2001:db8::3:1 action pass
>+
>+	vlan_over_bond_arp "active-backup"
>+	vlan_over_bond_ns "active-backup"
>+}
>+
> trap cleanup EXIT
> 
> setup_prepare
>-- 
>2.50.1
>

---
	-Jay Vosburgh, jv@jvosburgh.net

Re: [PATCH net] bonding: don't set oif to bond dev when getting NS target destination

[Jakub Kicinski]

On Mon, 11 Aug 2025 07:27:19 -0700 Jay Vosburgh wrote:
> 	Generically, I'm wondering if test updates should be separate
> patches from the functional changes as a general policy.

Yes, not sure if we made it a hard requirement, but I think it's our
preference. It is the reason why we don't require cover letters for
submissions with 2 patches.

Hangbin, please update config for bonding tests, looks like vlans 
are not enabled there today.
-- 
pw-bot: cr

Re: [PATCH net] bonding: don't set oif to bond dev when getting NS target destination

[Hangbin Liu]

On Mon, Aug 11, 2025 at 09:33:28AM -0700, Jakub Kicinski wrote:
> On Mon, 11 Aug 2025 07:27:19 -0700 Jay Vosburgh wrote:
> > 	Generically, I'm wondering if test updates should be separate
> > patches from the functional changes as a general policy.
> 
> Yes, not sure if we made it a hard requirement, but I think it's our
> preference. It is the reason why we don't require cover letters for
> submissions with 2 patches.
> 
> Hangbin, please update config for bonding tests, looks like vlans 
> are not enabled there today.

Ah, yes, I forgot to update this part.

Thanks
Hangbin

Re: [PATCH net] bonding: don't set oif to bond dev when getting NS target destination

[Hangbin Liu]

On Mon, Aug 11, 2025 at 09:33:28AM -0700, Jakub Kicinski wrote:
> On Mon, 11 Aug 2025 07:27:19 -0700 Jay Vosburgh wrote:
> > 	Generically, I'm wondering if test updates should be separate
> > patches from the functional changes as a general policy.
> 
> Yes, not sure if we made it a hard requirement, but I think it's our
> preference. It is the reason why we don't require cover letters for
> submissions with 2 patches.
> 
> Hangbin, please update config for bonding tests, looks like vlans 
> are not enabled there today.

BTW, I'd like to change the bond config to modules. Because we can't unload
the modules with current config. It that OK for you?

diff --git a/tools/testing/selftests/drivers/net/bonding/config b/tools/testing/selftests/drivers/net/bonding/config
index dad4e5fda4db..858ba0f19348 100644
--- a/tools/testing/selftests/drivers/net/bonding/config
+++ b/tools/testing/selftests/drivers/net/bonding/config
@@ -1,11 +1,11 @@
-CONFIG_BONDING=y
-CONFIG_BRIDGE=y
-CONFIG_DUMMY=y
+CONFIG_BONDING=m
+CONFIG_BRIDGE=m
+CONFIG_DUMMY=m
 CONFIG_IPV6=y
-CONFIG_MACVLAN=y
-CONFIG_IPVLAN=y
-CONFIG_NET_ACT_GACT=y
-CONFIG_NET_CLS_FLOWER=y
-CONFIG_NET_SCH_INGRESS=y
-CONFIG_NLMON=y
-CONFIG_VETH=y
+CONFIG_MACVLAN=m
+CONFIG_IPVLAN=m
+CONFIG_NET_ACT_GACT=m
+CONFIG_NET_CLS_FLOWER=m
+CONFIG_NET_SCH_INGRESS=m
+CONFIG_NLMON=m
+CONFIG_VETH=m

Thanks
Hangbin

Re: [PATCH net] bonding: don't set oif to bond dev when getting NS target destination

[Jakub Kicinski]

On Tue, 12 Aug 2025 02:04:53 +0000 Hangbin Liu wrote:
> On Mon, Aug 11, 2025 at 09:33:28AM -0700, Jakub Kicinski wrote:
> > On Mon, 11 Aug 2025 07:27:19 -0700 Jay Vosburgh wrote:  
> > > 	Generically, I'm wondering if test updates should be separate
> > > patches from the functional changes as a general policy.  
> > 
> > Yes, not sure if we made it a hard requirement, but I think it's our
> > preference. It is the reason why we don't require cover letters for
> > submissions with 2 patches.
> > 
> > Hangbin, please update config for bonding tests, looks like vlans 
> > are not enabled there today.  
> 
> BTW, I'd like to change the bond config to modules. Because we can't unload
> the modules with current config. It that OK for you?

Yes. I'm not sure if any of the bonding tests need to load the modules
explicitly but we'll find out :) In general modules are fine for the CI.