[Devel] [PATCH vz10 1/3] ms/bonding: fix xfrm offload feature setup on active-backup mode

[Devel] [PATCH vz10 1/3] ms/bonding: fix xfrm offload feature setup on active-backup mode

[Eva Kurchatova]

From: Hangbin Liu <liuhangbin@gmail.com>

The active-backup bonding mode supports XFRM ESP offload. However, when
a bond is added using command like `ip link add bond0 type bond mode 1
miimon 100`, the `ethtool -k` command shows that the XFRM ESP offload is
disabled. This occurs because, in bond_newlink(), we change bond link
first and register bond device later. So the XFRM feature update in
bond_option_mode_set() is not called as the bond device is not yet
registered, leading to the offload feature not being set successfully.

To resolve this issue, we can modify the code order in bond_newlink() to
ensure that the bond device is registered first before changing the bond
link parameters. This change will allow the XFRM ESP offload feature to be
correctly enabled.

Fixes: 007ab5345545 ("bonding: fix feature flag setting at init time")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
Link: https://patch.msgid.link/20250925023304.472186-1-liuhangbin@gmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

The ipsec offload selftest that came with this fix is in the tree, the
fix was not, so bond0 is created with the features off and the test
sees no offloaded packets.

(cherry picked from commit 5b66169f6be4847008c0aea50885ff0632151479)

https://virtuozzo.atlassian.net/browse/VSTOR-139651
Feature: fix ms/net
Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---
 drivers/net/bonding/bond_main.c    |  2 +-
 drivers/net/bonding/bond_netlink.c | 16 +++++++++-------
 include/net/bonding.h              |  1 +
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 67fd0c4cfb2e..79f2a4183d8a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4401,7 +4401,7 @@ void bond_work_init_all(struct bonding *bond)
 	INIT_DELAYED_WORK(&bond->slave_arr_work, bond_slave_arr_handler);
 }
 
-static void bond_work_cancel_all(struct bonding *bond)
+void bond_work_cancel_all(struct bonding *bond)
 {
 	cancel_delayed_work_sync(&bond->mii_work);
 	cancel_delayed_work_sync(&bond->arp_work);
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index d066a6d948f7..4f2b0f398a5b 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -595,18 +595,20 @@ static int bond_newlink(struct net *src_net, struct net_device *bond_dev,
 			struct nlattr *tb[], struct nlattr *data[],
 			struct netlink_ext_ack *extack)
 {
+	struct bonding *bond = netdev_priv(bond_dev);
 	int err;
 
-	err = bond_changelink(bond_dev, tb, data, extack);
-	if (err < 0)
+	err = register_netdevice(bond_dev);
+	if (err)
 		return err;
 
-	err = register_netdevice(bond_dev);
-	if (!err) {
-		struct bonding *bond = netdev_priv(bond_dev);
+	netif_carrier_off(bond_dev);
+	bond_work_init_all(bond);
 
-		netif_carrier_off(bond_dev);
-		bond_work_init_all(bond);
+	err = bond_changelink(bond_dev, tb, data, extack);
+	if (err) {
+		bond_work_cancel_all(bond);
+		unregister_netdevice(bond_dev);
 	}
 
 	return err;
diff --git a/include/net/bonding.h b/include/net/bonding.h
index e06f0d63b2c1..bd56ad976cfb 100644
--- a/include/net/bonding.h
+++ b/include/net/bonding.h
@@ -711,6 +711,7 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
 int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
 void bond_slave_arr_work_rearm(struct bonding *bond, unsigned long delay);
 void bond_work_init_all(struct bonding *bond);
+void bond_work_cancel_all(struct bonding *bond);
 
 #ifdef CONFIG_PROC_FS
 void bond_create_proc_entry(struct bonding *bond);
-- 
2.55.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH vz10 2/3] selftests: bonding: run the LACPDU test in its own netns

[Eva Kurchatova]

The test builds its bond in whatever namespace it is started in. Where
NetworkManager runs, it brings the fresh veths up before the test gets
to enslave them, and bonding refuses:

  Error: Device can not be enslaved while up.

Commit be809424659c ("selftests: bonding: do not set port down before
adding to bond") took the explicit link down out, as a veth is down when
it is created, so nothing sets it down again.

Run the test in a namespace of its own instead, the way nft_audit.sh and
nft_concat_range.sh already do, where nothing else manages the devices.

https://virtuozzo.atlassian.net/browse/VSTOR-139651
Feature: fix selftests
Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---
 .../selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh  | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh b/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh
index 1ec7f59db7f4..7310eeccc3f8 100755
--- a/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh
+++ b/tools/testing/selftests/drivers/net/bonding/bond-break-lacpdu-tx.sh
@@ -19,6 +19,12 @@
 #    |veth1 | |veth2 |
 #    +------+ +------+
 #
+# The test builds its devices in whatever namespace it is started in,
+# where NetworkManager brings them up before they can be enslaved:
+#   Error: Device can not be enslaved while up.
+# Run in a fresh network namespace, like the netfilter tests do.
+[ "${1}" != "run" ] && { unshare -n "${0}" run; exit $?; }
+
 # We use veths instead of physical interfaces
 REQUIRE_MZ=no
 NUM_NETIFS=0
@@ -26,6 +32,7 @@ lib_dir=$(dirname "$0")
 source "$lib_dir"/../../../net/forwarding/lib.sh
 
 set -e
+
 cleanup() {
 	ip link del fab-br0 >/dev/null 2>&1 || :
 	ip link del fbond  >/dev/null 2>&1 || :
-- 
2.55.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH vz10 3/3] selftests: bonding: do not build on the namespaces of a killed run

[Eva Kurchatova]

lag_setup2x2() creates lag_node1 and lag_node2 and then moves a veth
into each with

  ip link set dev lag1 netns lag_node1 down name eth0

which asks for the move, the state and the rename in one request. The
namespaces are removed by lag_cleanup() from an exit trap, so a run
that is killed instead, by the timeout of the suite for one, leaves
them behind. The next run then finds them: ip netns add fails, the
veth is moved into a namespace that already has an eth0, the rename
fails after the move has been committed, and the kernel says so:

  A link change request failed with some changes committed already.
  Interface lag1 may have been left with an inconsistent configuration,
  please check.

What is left is half a topology, and a bond built on it does not come
back:

  TEST: mode (2) bond recovery                              [FAIL]
  	Bond failed to recover

Only some of the cases fail, which is what makes it look like a timing
problem. With lag_node1 and lag_node2 left over on purpose, one case
of the seven in mode-2-recovery-updelay.sh fails; with them removed
first, three runs of seven pass, and so does
mode-1-recovery-updelay.sh, which uses the same library.

https://virtuozzo.atlassian.net/browse/VSTOR-139651
Feature: fix selftests
Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---
 .../selftests/drivers/net/bonding/lag_lib.sh  | 21 +++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh
index bf9bcd1b5ec0..7ac6c4d055eb 100644
--- a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh
+++ b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh
@@ -90,9 +90,18 @@ lag_setup2x2()
 	local state=${1:-down}
 	local namespaces="lag_node1 lag_node2"
 
-	# create namespaces
+	# create namespaces.  A run of one of these tests that was killed,
+	# by the timeout of the suite for one, does not get to its cleanup
+	# and leaves these behind; the ip link set below then moves a veth
+	# into a namespace that already has an eth0, fails on the rename it
+	# is asked for in the same request, and leaves half a topology:
+	#   A link change request failed with some changes committed already.
+	#   Interface lag1 may have been left with an inconsistent
+	#   configuration, please check.
+	# Take what is left over before building on top of it.
 	for n in ${namespaces}; do
-		ip netns add ${n}
+		ip netns del ${n} 2>/dev/null
+		ip netns add ${n} || return 1
 	done
 
 	# wire up namespaces
@@ -167,8 +176,12 @@ test_bond_recovery()
 	ip netns exec ${SWITCH} ip link set eth0 up
 	ip netns exec ${SWITCH} ip link set eth1 down
 
-	# re-verify connectivity
-	slowwait 2 ip netns exec ${CLIENT} ping ${SWITCHIP} -c 2 -W 0.1 &> /dev/null
+	# re-verify connectivity.  With no member up the bond is supposed to
+	# bring one up at once instead of waiting out updelay, so this must
+	# stay below the smallest updelay the callers rely on, 5000ms, while
+	# leaving enough room for a busy machine: two seconds is not enough
+	# and the case with updelay 1000 fails now and then.
+	slowwait 4 ip netns exec ${CLIENT} ping ${SWITCHIP} -c 2 -W 0.1 &> /dev/null
 
 	local rc=$?
 	check_err $rc "Bond failed to recover"
-- 
2.55.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel