[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

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

[Konstantin Khorenko]

On 9/1/26 01:39, Eva Kurchatova wrote:
> 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)

While backporting this patch you have adapted it, in particular here - dropped 2 hunks.
Please ALWAYS mention any changes you do when backporting a patch from other repo/upstream.

> 
> 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);

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

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

[Konstantin Khorenko]

> 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.

Measured numbers do not belong in a commit message, they are true only
on the machine they came from. "one case of the seven", "three runs of
seven" go to the ticket. Here it is enough to say that the failure is
intermittent and goes away once the leftover namespaces are removed.

Also the message describes one change, the namespace cleanup, while
the patch does two, see the slowwait hunk below.

> 
> https://virtuozzo.atlassian.net/browse/VSTOR-139651
> Feature: fix selftests
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
>
> diff --git a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh
> index bf9bcd1b5ec04..7ac6c4d055eb2 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.

This repeats the commit message. In a test one line is enough, e.g.

      # a killed run leaves the namespaces behind, start clean

Also double space after the period ("namespaces.  A run"), single space
please, here and below.

>  	for n in ${namespaces}; do
> -		ip netns add ${n}
> +		ip netns del ${n} 2>/dev/null
> +		ip netns add ${n} || return 1

The del before add is fine: the old veths go away with the namespaces
and the new pair is created in the root namespace and moved right away,
nothing collides.

The "|| return 1" does nothing though: lag_setup_network() does not look
at the return value and these scripts do not run under set -e. Either
drop it or make the caller act on it.

>  	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

This is a separate change and it is not mentioned in the subject or the
body, only this comment explains it. Please split it into its own patch
with its own justification, or at least describe it in the message.

And the justification is what I am missing. bond_miimon_inspect()
ignores updelay as soon as there is no active member
(usable_slaves->count == 0 for xor, curr_active_slave == NULL for
active-backup), so once eth1 goes down eth0 is brought up on the next
miimon tick. With miimon 100 that is a few hundred milliseconds, not
seconds. If the updelay 1000 case fails at 2 seconds now and then,
either the box is heavily loaded or the kernel really waits somewhere,
and a longer timeout would hide that. How long did the recovery take in
the failed runs? Was that with the leftover namespaces already removed?

Side effect to keep in mind: at 4 seconds the updelay 2000 case stops
catching a bond that sits out its updelay, it would recover in ~2.1s
and pass. Only the 5000 and 10000 cases remain a real check after this.

The comment itself is again the commit message pasted into the test,
one line at most, e.g.

      # must stay below the smallest updelay the callers use

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

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

[Konstantin Khorenko]

Committed to branch-rh10-6.12.0-211.52.1.22.x.vz10-ovz, will be released in rh10-6.12.0-211.52.1.22.3.vz10

ecbcb0520bdf



ms/bonding: fix xfrm offload feature setup on active-backup mode
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)
Conflict: bond_newlink() here still takes tb and data as arguments,
upstream reads them from struct rtnl_newlink_params. The reorder is
applied as is on top of the old signature.
https://virtuozzo.atlassian.net/browse/VSTOR-139651
Feature: fix ms/net
Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
Link: http://lore.virtuozzo.com/20260831234036.1649006-1-eva.kurchatova@virtuozzo.com







--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 9/1/26 01:39, Eva Kurchatova wrote:
> 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);

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

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

[Konstantin Khorenko]

Committed to branch-rh10-6.12.0-211.52.1.22.x.vz10-ovz, will be released in rh10-6.12.0-211.52.1.22.3.vz10

ca28e6fa4bad



selftests: bonding: run the LACPDU test in its own netns
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>
Link: http://lore.virtuozzo.com/20260831234036.1649006-2-eva.kurchatova@virtuozzo.com

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 9/1/26 01:39, Eva Kurchatova wrote:
> 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 || :

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

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

[Konstantin Khorenko]

i have NOT applied it, please rework.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 9/1/26 01:39, Eva Kurchatova wrote:
> 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"

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