[PATCH net v2 0/4] bonding: properly restore flags when bond changes ether type

[PATCH net v2 0/4] bonding: properly restore flags when bond changes ether type

[Nikolay Aleksandrov]

Hi,
A bug was reported by syzbot[1] that causes a warning and a myriad of
other potential issues if a bond, that is also a slave, fails to enslave a
non-eth device. While fixing that bug I found that we have the same
issues when such enslave passes and after that the bond changes back to
ARPHRD_ETHER (again due to ether_setup). This set fixes all issues by
extracting the ether_setup() sequence in a helper which does the right
thing about bond flags when it needs to change back to ARPHRD_ETHER. It
also adds selftests for these cases.

Patch 01 adds the new bond_ether_setup helper that is used in the
following patches to fix the bond dev flag issues. Patch 02 fixes the
issues when a bond device changes its ether type due to successful
enslave. Patch 03 fixes the issues when it changes its ether type due to
an unsuccessful enslave. Note we need two patches because the bugs were
introduced by different commits. Patch 04 adds the new selftests.

v2: new set, all patches are new due to new approach of fixing these bugs

Thanks,
 Nik

[1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef

Nikolay Aleksandrov (4):
  bonding: add bond_ether_setup helper
  bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type
    change
  bonding: restore bond's IFF_SLAVE flag if a non-eth dev enslave fails
  selftests: bonding: add tests for ether type changes

 drivers/net/bonding/bond_main.c               | 22 +++--
 .../selftests/drivers/net/bonding/Makefile    |  3 +-
 .../net/bonding/bond-eth-type-change.sh       | 85 +++++++++++++++++++
 3 files changed, 102 insertions(+), 8 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-eth-type-change.sh

-- 
2.39.2

[PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Nikolay Aleksandrov]

Add bond_ether_setup helper which will be used in the following patches
to fix all ether_setup() calls in the bonding driver. It takes care of both
IFF_MASTER and IFF_SLAVE flags, the former is always restored and the
latter only if it was set.

Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type")
Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 drivers/net/bonding/bond_main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 00646aa315c3..d41024ad2c18 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave)
 		slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);	\
 } while (0)
 
+/* ether_setup() resets bond_dev's flags so we always have to restore
+ * IFF_MASTER, and only restore IFF_SLAVE if it was set
+ */
+static void bond_ether_setup(struct net_device *bond_dev)
+{
+	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
+
+	ether_setup(bond_dev);
+	bond_dev->flags |= IFF_MASTER | slave_flag;
+	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+}
+
 /* enslave device <slave> to bond device <master> */
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 		 struct netlink_ext_ack *extack)
-- 
2.39.2

[PATCH net v2 2/4] bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type change

[Nikolay Aleksandrov]

If the bond enslaves non-ARPHRD_ETHER device (changes its type), then
releases it and enslaves ARPHRD_ETHER device (changes back) then we
use ether_setup() to restore the bond device type but it also resets its
flags and removes IFF_MASTER and IFF_SLAVE[1]. Use the bond_ether_setup
helper to restore both after such transition.

[1] reproduce (nlmon is non-ARPHRD_ETHER):
 $ ip l add nlmon0 type nlmon
 $ ip l add bond2 type bond mode active-backup
 $ ip l set nlmon0 master bond2
 $ ip l set nlmon0 nomaster
 $ ip l add bond1 type bond
 (we use bond1 as ARPHRD_ETHER device to restore bond2's mode)
 $ ip l set bond1 master bond2
 $ ip l sh dev bond2
 37: bond2: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether be:d7:c5:40:5b:cc brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 1500
 (notice bond2's IFF_MASTER is missing)

Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 drivers/net/bonding/bond_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d41024ad2c18..cd94baccdac5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1878,10 +1878,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 
 			if (slave_dev->type != ARPHRD_ETHER)
 				bond_setup_by_slave(bond_dev, slave_dev);
-			else {
-				ether_setup(bond_dev);
-				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
-			}
+			else
+				bond_ether_setup(bond_dev);
 
 			call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
 						 bond_dev);
-- 
2.39.2

[PATCH net v2 3/4] bonding: restore bond's IFF_SLAVE flag if a non-eth dev enslave fails

[Nikolay Aleksandrov]

syzbot reported a warning[1] where the bond device itself is a slave and
we try to enslave a non-ethernet device as the first slave which fails
but then in the error path when ether_setup() restores the bond device
it also clears all flags. In my previous fix[2] I restored the
IFF_MASTER flag, but I didn't consider the case that the bond device
itself might also be a slave with IFF_SLAVE set, so we need to restore
that flag as well. Use the new bond_ether_setup helper which does the
right thing and restores the bond's flags properly.

Steps to reproduce using a nlmon dev:
 $ ip l add nlmon0 type nlmon
 $ ip l add bond1 type bond
 $ ip l add bond2 type bond
 $ ip l set bond1 master bond2
 $ ip l set dev nlmon0 master bond1
 $ ip -d l sh dev bond1
 22: bond1: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noqueue master bond2 state DOWN mode DEFAULT group default qlen 1000
 (now bond1's IFF_SLAVE flag is gone and we'll hit a warning[3] if we
  try to delete it)

[1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
[2] commit 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
[3] example warning:
 [   27.008664] bond1: (slave nlmon0): The slave device specified does not support setting the MAC address
 [   27.008692] bond1: (slave nlmon0): Error -95 calling set_mac_address
 [   32.464639] bond1 (unregistering): Released all slaves
 [   32.464685] ------------[ cut here ]------------
 [   32.464686] WARNING: CPU: 1 PID: 2004 at net/core/dev.c:10829 unregister_netdevice_many+0x72a/0x780
 [   32.464694] Modules linked in: br_netfilter bridge bonding virtio_net
 [   32.464699] CPU: 1 PID: 2004 Comm: ip Kdump: loaded Not tainted 5.18.0-rc3+ #47
 [   32.464703] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc37 04/01/2014
 [   32.464704] RIP: 0010:unregister_netdevice_many+0x72a/0x780
 [   32.464707] Code: 99 fd ff ff ba 90 1a 00 00 48 c7 c6 f4 02 66 96 48 c7 c7 20 4d 35 96 c6 05 fa c7 2b 02 01 e8 be 6f 4a 00 0f 0b e9 73 fd ff ff <0f> 0b e9 5f fd ff ff 80 3d e3 c7 2b 02 00 0f 85 3b fd ff ff ba 59
 [   32.464710] RSP: 0018:ffffa006422d7820 EFLAGS: 00010206
 [   32.464712] RAX: ffff8f6e077140a0 RBX: ffffa006422d7888 RCX: 0000000000000000
 [   32.464714] RDX: ffff8f6e12edbe58 RSI: 0000000000000296 RDI: ffffffff96d4a520
 [   32.464716] RBP: ffff8f6e07714000 R08: ffffffff96d63600 R09: ffffa006422d7728
 [   32.464717] R10: 0000000000000ec0 R11: ffffffff9698c988 R12: ffff8f6e12edb140
 [   32.464719] R13: dead000000000122 R14: dead000000000100 R15: ffff8f6e12edb140
 [   32.464723] FS:  00007f297c2f1740(0000) GS:ffff8f6e5d900000(0000) knlGS:0000000000000000
 [   32.464725] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 [   32.464726] CR2: 00007f297bf1c800 CR3: 00000000115e8000 CR4: 0000000000350ee0
 [   32.464730] Call Trace:
 [   32.464763]  <TASK>
 [   32.464767]  rtnl_dellink+0x13e/0x380
 [   32.464776]  ? cred_has_capability.isra.0+0x68/0x100
 [   32.464780]  ? __rtnl_unlock+0x33/0x60
 [   32.464783]  ? bpf_lsm_capset+0x10/0x10
 [   32.464786]  ? security_capable+0x36/0x50
 [   32.464790]  rtnetlink_rcv_msg+0x14e/0x3b0
 [   32.464792]  ? _copy_to_iter+0xb1/0x790
 [   32.464796]  ? post_alloc_hook+0xa0/0x160
 [   32.464799]  ? rtnl_calcit.isra.0+0x110/0x110
 [   32.464802]  netlink_rcv_skb+0x50/0xf0
 [   32.464806]  netlink_unicast+0x216/0x340
 [   32.464809]  netlink_sendmsg+0x23f/0x480
 [   32.464812]  sock_sendmsg+0x5e/0x60
 [   32.464815]  ____sys_sendmsg+0x22c/0x270
 [   32.464818]  ? import_iovec+0x17/0x20
 [   32.464821]  ? sendmsg_copy_msghdr+0x59/0x90
 [   32.464823]  ? do_set_pte+0xa0/0xe0
 [   32.464828]  ___sys_sendmsg+0x81/0xc0
 [   32.464832]  ? mod_objcg_state+0xc6/0x300
 [   32.464835]  ? refill_obj_stock+0xa9/0x160
 [   32.464838]  ? memcg_slab_free_hook+0x1a5/0x1f0
 [   32.464842]  __sys_sendmsg+0x49/0x80
 [   32.464847]  do_syscall_64+0x3b/0x90
 [   32.464851]  entry_SYSCALL_64_after_hwframe+0x44/0xae
 [   32.464865] RIP: 0033:0x7f297bf2e5e7
 [   32.464868] Code: 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
 [   32.464869] RSP: 002b:00007ffd96c824c8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
 [   32.464872] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f297bf2e5e7
 [   32.464874] RDX: 0000000000000000 RSI: 00007ffd96c82540 RDI: 0000000000000003
 [   32.464875] RBP: 00000000640f19de R08: 0000000000000001 R09: 000000000000007c
 [   32.464876] R10: 00007f297bffabe0 R11: 0000000000000246 R12: 0000000000000001
 [   32.464877] R13: 00007ffd96c82d20 R14: 00007ffd96c82610 R15: 000055bfe38a7020
 [   32.464881]  </TASK>
 [   32.464882] ---[ end trace 0000000000000000 ]---

Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
Reported-by: syzbot+9dfc3f3348729cc82277@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 drivers/net/bonding/bond_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index cd94baccdac5..16691016da1c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2299,9 +2299,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 			eth_hw_addr_random(bond_dev);
 		if (bond_dev->type != ARPHRD_ETHER) {
 			dev_close(bond_dev);
-			ether_setup(bond_dev);
-			bond_dev->flags |= IFF_MASTER;
-			bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
+			bond_ether_setup(bond_dev);
 		}
 	}
 
-- 
2.39.2

[PATCH net v2 4/4] selftests: bonding: add tests for ether type changes

[Nikolay Aleksandrov]

Add new network selftests for the bonding device which exercise the ether
type changing call paths. They also test for the recent syzbot bug[1] which
causes a warning and results in wrong device flags (IFF_SLAVE missing).
The test adds three bond devices and a nlmon device, enslaves one of the
bond devices to the other and then uses the nlmon device for successful
and unsuccesful enslaves both of which change the bond ether type. Thus
we can test for both MASTER and SLAVE flags at the same time.

If the flags are properly restored we get:
TEST: Change ether type of an enslaved bond device with unsuccessful enslave   [ OK ]
TEST: Change ether type of an enslaved bond device with successful enslave   [ OK ]

[1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef

Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
 .../selftests/drivers/net/bonding/Makefile    |  3 +-
 .../net/bonding/bond-eth-type-change.sh       | 85 +++++++++++++++++++
 2 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-eth-type-change.sh

diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile
index 8e3b786a748f..a39bb2560d9b 100644
--- a/tools/testing/selftests/drivers/net/bonding/Makefile
+++ b/tools/testing/selftests/drivers/net/bonding/Makefile
@@ -8,7 +8,8 @@ TEST_PROGS := \
 	dev_addr_lists.sh \
 	mode-1-recovery-updelay.sh \
 	mode-2-recovery-updelay.sh \
-	option_prio.sh
+	option_prio.sh \
+	bond-eth-type-change.sh
 
 TEST_FILES := \
 	lag_lib.sh \
diff --git a/tools/testing/selftests/drivers/net/bonding/bond-eth-type-change.sh b/tools/testing/selftests/drivers/net/bonding/bond-eth-type-change.sh
new file mode 100755
index 000000000000..5cdd22048ba7
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/bonding/bond-eth-type-change.sh
@@ -0,0 +1,85 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test bond device ether type changing
+#
+
+ALL_TESTS="
+	bond_test_unsuccessful_enslave_type_change
+	bond_test_successful_enslave_type_change
+"
+REQUIRE_MZ=no
+NUM_NETIFS=0
+lib_dir=$(dirname "$0")
+source "$lib_dir"/net_forwarding_lib.sh
+
+bond_check_flags()
+{
+	local bonddev=$1
+
+	ip -d l sh dev "$bonddev" | grep -q "MASTER"
+	check_err $? "MASTER flag is missing from the bond device"
+
+	ip -d l sh dev "$bonddev" | grep -q "SLAVE"
+	check_err $? "SLAVE flag is missing from the bond device"
+}
+
+# test enslaved bond dev type change from ARPHRD_ETHER and back
+# this allows us to test both MASTER and SLAVE flags at once
+bond_test_enslave_type_change()
+{
+	local test_success=$1
+	local devbond0="test-bond0"
+	local devbond1="test-bond1"
+	local devbond2="test-bond2"
+	local nonethdev="test-noneth0"
+
+	# create a non-ARPHRD_ETHER device for testing (e.g. nlmon type)
+	ip link add name "$nonethdev" type nlmon
+	check_err $? "could not create a non-ARPHRD_ETHER device (nlmon)"
+	ip link add name "$devbond0" type bond
+	if [ $test_success -eq 1 ]; then
+		# we need devbond0 in active-backup mode to successfully enslave nonethdev
+		ip link set dev "$devbond0" type bond mode active-backup
+		check_err $? "could not change bond mode to active-backup"
+	fi
+	ip link add name "$devbond1" type bond
+	ip link add name "$devbond2" type bond
+	ip link set dev "$devbond0" master "$devbond1"
+	check_err $? "could not enslave $devbond0 to $devbond1"
+	# change bond type to non-ARPHRD_ETHER
+	ip link set dev "$nonethdev" master "$devbond0" 1>/dev/null 2>/dev/null
+	ip link set dev "$nonethdev" nomaster 1>/dev/null 2>/dev/null
+	# restore ARPHRD_ETHER type by enslaving such device
+	ip link set dev "$devbond2" master "$devbond0"
+	check_err $? "could not enslave $devbond2 to $devbond0"
+	ip link set dev "$devbond1" nomaster
+
+	bond_check_flags "$devbond0"
+
+	# clean up
+	ip link del dev "$devbond0"
+	ip link del dev "$devbond1"
+	ip link del dev "$devbond2"
+	ip link del dev "$nonethdev"
+}
+
+bond_test_unsuccessful_enslave_type_change()
+{
+	RET=0
+
+	bond_test_enslave_type_change 0
+	log_test "Change ether type of an enslaved bond device with unsuccessful enslave"
+}
+
+bond_test_successful_enslave_type_change()
+{
+	RET=0
+
+	bond_test_enslave_type_change 1
+	log_test "Change ether type of an enslaved bond device with successful enslave"
+}
+
+tests_run
+
+exit "$EXIT_STATUS"
-- 
2.39.2

Re: [PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Michal Kubiak]

On Tue, Mar 14, 2023 at 01:14:23PM +0200, Nikolay Aleksandrov wrote:
> Add bond_ether_setup helper which will be used in the following patches
> to fix all ether_setup() calls in the bonding driver. It takes care of both
> IFF_MASTER and IFF_SLAVE flags, the former is always restored and the
> latter only if it was set.
> 
> Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type")
> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>  drivers/net/bonding/bond_main.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 00646aa315c3..d41024ad2c18 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave)
>  		slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);	\
>  } while (0)
>  
> +/* ether_setup() resets bond_dev's flags so we always have to restore
> + * IFF_MASTER, and only restore IFF_SLAVE if it was set
> + */

I would suggest using the kernel pattern for function documentation.
At first glance, the name "ether_setup" at the beginning is easy to be
confused with the function name (bond_ether_setup).

> +static void bond_ether_setup(struct net_device *bond_dev)
> +{
> +	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
> +
> +	ether_setup(bond_dev);
> +	bond_dev->flags |= IFF_MASTER | slave_flag;
> +	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> +}
> +
>  /* enslave device <slave> to bond device <master> */
>  int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  		 struct netlink_ext_ack *extack)

It seems you never call this newly added helper in the current patch. I
think it creates a compilation warning ("defined but not used").
Please add your function in the patch where you actually use it.

> -- 
> 2.39.2
> 


Thanks,
Michal

Re: [PATCH net v2 4/4] selftests: bonding: add tests for ether type changes

[Michal Kubiak]

On Tue, Mar 14, 2023 at 01:14:26PM +0200, Nikolay Aleksandrov wrote:
> Add new network selftests for the bonding device which exercise the ether
> type changing call paths. They also test for the recent syzbot bug[1] which
> causes a warning and results in wrong device flags (IFF_SLAVE missing).
> The test adds three bond devices and a nlmon device, enslaves one of the
> bond devices to the other and then uses the nlmon device for successful
> and unsuccesful enslaves both of which change the bond ether type. Thus
> we can test for both MASTER and SLAVE flags at the same time.
> 
> If the flags are properly restored we get:
> TEST: Change ether type of an enslaved bond device with unsuccessful enslave   [ OK ]
> TEST: Change ether type of an enslaved bond device with successful enslave   [ OK ]
> 
> [1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
> 
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>


Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Thanks,
Michal

Re: [PATCH net v2 3/4] bonding: restore bond's IFF_SLAVE flag if a non-eth dev enslave fails

[Michal Kubiak]

On Tue, Mar 14, 2023 at 01:14:25PM +0200, Nikolay Aleksandrov wrote:
> syzbot reported a warning[1] where the bond device itself is a slave and
> we try to enslave a non-ethernet device as the first slave which fails
> but then in the error path when ether_setup() restores the bond device
> it also clears all flags. In my previous fix[2] I restored the
> IFF_MASTER flag, but I didn't consider the case that the bond device
> itself might also be a slave with IFF_SLAVE set, so we need to restore
> that flag as well. Use the new bond_ether_setup helper which does the
> right thing and restores the bond's flags properly.
> 
> Steps to reproduce using a nlmon dev:
>  $ ip l add nlmon0 type nlmon
>  $ ip l add bond1 type bond
>  $ ip l add bond2 type bond
>  $ ip l set bond1 master bond2
>  $ ip l set dev nlmon0 master bond1
>  $ ip -d l sh dev bond1
>  22: bond1: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noqueue master bond2 state DOWN mode DEFAULT group default qlen 1000
>  (now bond1's IFF_SLAVE flag is gone and we'll hit a warning[3] if we
>   try to delete it)
> 
> [1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
> [2] commit 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
> [3] example warning:
>  [   27.008664] bond1: (slave nlmon0): The slave device specified does not support setting the MAC address
>  [   27.008692] bond1: (slave nlmon0): Error -95 calling set_mac_address
>  [   32.464639] bond1 (unregistering): Released all slaves
>  [   32.464685] ------------[ cut here ]------------
>  [   32.464686] WARNING: CPU: 1 PID: 2004 at net/core/dev.c:10829 unregister_netdevice_many+0x72a/0x780
>  [   32.464694] Modules linked in: br_netfilter bridge bonding virtio_net
>  [   32.464699] CPU: 1 PID: 2004 Comm: ip Kdump: loaded Not tainted 5.18.0-rc3+ #47
>  [   32.464703] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.1-2.fc37 04/01/2014
>  [   32.464704] RIP: 0010:unregister_netdevice_many+0x72a/0x780
>  [   32.464707] Code: 99 fd ff ff ba 90 1a 00 00 48 c7 c6 f4 02 66 96 48 c7 c7 20 4d 35 96 c6 05 fa c7 2b 02 01 e8 be 6f 4a 00 0f 0b e9 73 fd ff ff <0f> 0b e9 5f fd ff ff 80 3d e3 c7 2b 02 00 0f 85 3b fd ff ff ba 59
>  [   32.464710] RSP: 0018:ffffa006422d7820 EFLAGS: 00010206
>  [   32.464712] RAX: ffff8f6e077140a0 RBX: ffffa006422d7888 RCX: 0000000000000000
>  [   32.464714] RDX: ffff8f6e12edbe58 RSI: 0000000000000296 RDI: ffffffff96d4a520
>  [   32.464716] RBP: ffff8f6e07714000 R08: ffffffff96d63600 R09: ffffa006422d7728
>  [   32.464717] R10: 0000000000000ec0 R11: ffffffff9698c988 R12: ffff8f6e12edb140
>  [   32.464719] R13: dead000000000122 R14: dead000000000100 R15: ffff8f6e12edb140
>  [   32.464723] FS:  00007f297c2f1740(0000) GS:ffff8f6e5d900000(0000) knlGS:0000000000000000
>  [   32.464725] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  [   32.464726] CR2: 00007f297bf1c800 CR3: 00000000115e8000 CR4: 0000000000350ee0
>  [   32.464730] Call Trace:
>  [   32.464763]  <TASK>
>  [   32.464767]  rtnl_dellink+0x13e/0x380
>  [   32.464776]  ? cred_has_capability.isra.0+0x68/0x100
>  [   32.464780]  ? __rtnl_unlock+0x33/0x60
>  [   32.464783]  ? bpf_lsm_capset+0x10/0x10
>  [   32.464786]  ? security_capable+0x36/0x50
>  [   32.464790]  rtnetlink_rcv_msg+0x14e/0x3b0
>  [   32.464792]  ? _copy_to_iter+0xb1/0x790
>  [   32.464796]  ? post_alloc_hook+0xa0/0x160
>  [   32.464799]  ? rtnl_calcit.isra.0+0x110/0x110
>  [   32.464802]  netlink_rcv_skb+0x50/0xf0
>  [   32.464806]  netlink_unicast+0x216/0x340
>  [   32.464809]  netlink_sendmsg+0x23f/0x480
>  [   32.464812]  sock_sendmsg+0x5e/0x60
>  [   32.464815]  ____sys_sendmsg+0x22c/0x270
>  [   32.464818]  ? import_iovec+0x17/0x20
>  [   32.464821]  ? sendmsg_copy_msghdr+0x59/0x90
>  [   32.464823]  ? do_set_pte+0xa0/0xe0
>  [   32.464828]  ___sys_sendmsg+0x81/0xc0
>  [   32.464832]  ? mod_objcg_state+0xc6/0x300
>  [   32.464835]  ? refill_obj_stock+0xa9/0x160
>  [   32.464838]  ? memcg_slab_free_hook+0x1a5/0x1f0
>  [   32.464842]  __sys_sendmsg+0x49/0x80
>  [   32.464847]  do_syscall_64+0x3b/0x90
>  [   32.464851]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>  [   32.464865] RIP: 0033:0x7f297bf2e5e7
>  [   32.464868] Code: 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
>  [   32.464869] RSP: 002b:00007ffd96c824c8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>  [   32.464872] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f297bf2e5e7
>  [   32.464874] RDX: 0000000000000000 RSI: 00007ffd96c82540 RDI: 0000000000000003
>  [   32.464875] RBP: 00000000640f19de R08: 0000000000000001 R09: 000000000000007c
>  [   32.464876] R10: 00007f297bffabe0 R11: 0000000000000246 R12: 0000000000000001
>  [   32.464877] R13: 00007ffd96c82d20 R14: 00007ffd96c82610 R15: 000055bfe38a7020
>  [   32.464881]  </TASK>
>  [   32.464882] ---[ end trace 0000000000000000 ]---
> 
> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
> Reported-by: syzbot+9dfc3f3348729cc82277@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>

Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Re: [PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Nikolay Aleksandrov]

On 14/03/2023 16:58, Michal Kubiak wrote:
> On Tue, Mar 14, 2023 at 01:14:23PM +0200, Nikolay Aleksandrov wrote:
>> Add bond_ether_setup helper which will be used in the following patches
>> to fix all ether_setup() calls in the bonding driver. It takes care of both
>> IFF_MASTER and IFF_SLAVE flags, the former is always restored and the
>> latter only if it was set.
>>
>> Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type")
>> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>>  drivers/net/bonding/bond_main.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 00646aa315c3..d41024ad2c18 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave)
>>  		slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);	\
>>  } while (0)
>>  
>> +/* ether_setup() resets bond_dev's flags so we always have to restore
>> + * IFF_MASTER, and only restore IFF_SLAVE if it was set
>> + */
> 
> I would suggest using the kernel pattern for function documentation.
> At first glance, the name "ether_setup" at the beginning is easy to be
> confused with the function name (bond_ether_setup).
> 

This is an internal helper, I don't think it needs a full kernel doc.

>> +static void bond_ether_setup(struct net_device *bond_dev)
>> +{
>> +	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
>> +
>> +	ether_setup(bond_dev);
>> +	bond_dev->flags |= IFF_MASTER | slave_flag;
>> +	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>> +}
>> +
>>  /* enslave device <slave> to bond device <master> */
>>  int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>>  		 struct netlink_ext_ack *extack)
> 
> It seems you never call this newly added helper in the current patch. I
> think it creates a compilation warning ("defined but not used").
> Please add your function in the patch where you actually use it.
> 

I'm adding the helper in a separate patch to emphasize it and focus the review.
I have written in the commit message that the next two fixes will be using it.
IMO, this should be ok.

>> -- 
>> 2.39.2
>>
> 
> 
> Thanks,
> Michal

Cheers,
 Nik

Re: [PATCH net v2 2/4] bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type change

[Michal Kubiak]

On Tue, Mar 14, 2023 at 01:14:24PM +0200, Nikolay Aleksandrov wrote:
> If the bond enslaves non-ARPHRD_ETHER device (changes its type), then
> releases it and enslaves ARPHRD_ETHER device (changes back) then we
> use ether_setup() to restore the bond device type but it also resets its
> flags and removes IFF_MASTER and IFF_SLAVE[1]. Use the bond_ether_setup
> helper to restore both after such transition.
> 
> [1] reproduce (nlmon is non-ARPHRD_ETHER):
>  $ ip l add nlmon0 type nlmon
>  $ ip l add bond2 type bond mode active-backup
>  $ ip l set nlmon0 master bond2
>  $ ip l set nlmon0 nomaster
>  $ ip l add bond1 type bond
>  (we use bond1 as ARPHRD_ETHER device to restore bond2's mode)
>  $ ip l set bond1 master bond2
>  $ ip l sh dev bond2
>  37: bond2: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether be:d7:c5:40:5b:cc brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 1500
>  (notice bond2's IFF_MASTER is missing)
> 
> Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>  drivers/net/bonding/bond_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index d41024ad2c18..cd94baccdac5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1878,10 +1878,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>  
>  			if (slave_dev->type != ARPHRD_ETHER)
>  				bond_setup_by_slave(bond_dev, slave_dev);
> -			else {
> -				ether_setup(bond_dev);
> -				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> -			}
> +			else
> +				bond_ether_setup(bond_dev);

As I already commented on your previous patch: there is the first call
of "bond_ether_setup()".
Please think about merging this patch with the previous one to avoid the
compilation warning.

Thanks,
Michal


>  
>  			call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
>  						 bond_dev);
> -- 
> 2.39.2
> 

Re: [PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Nikolay Aleksandrov]

On 14/03/2023 17:08, Nikolay Aleksandrov wrote:
> On 14/03/2023 16:58, Michal Kubiak wrote:
>> On Tue, Mar 14, 2023 at 01:14:23PM +0200, Nikolay Aleksandrov wrote:
>>> Add bond_ether_setup helper which will be used in the following patches
>>> to fix all ether_setup() calls in the bonding driver. It takes care of both
>>> IFF_MASTER and IFF_SLAVE flags, the former is always restored and the
>>> latter only if it was set.
>>>
>>> Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type")
>>> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
>>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>>> ---
>>>  drivers/net/bonding/bond_main.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
[snip]
>> Please add your function in the patch where you actually use it.
>>
> 
> I'm adding the helper in a separate patch to emphasize it and focus the review.
> I have written in the commit message that the next two fixes will be using it.
> IMO, this should be ok.
> 

Also since both fixes are for different commits this should be backportable
for both, that makes it easier to pick if only one of the two would be ported to a
specific version (i.e. the first fix is for a commit from 2009, second 2015).

Cheers,
 Nik

Re: [PATCH net v2 2/4] bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type change

[Nikolay Aleksandrov]

On 14/03/2023 17:09, Michal Kubiak wrote:
> On Tue, Mar 14, 2023 at 01:14:24PM +0200, Nikolay Aleksandrov wrote:
>> If the bond enslaves non-ARPHRD_ETHER device (changes its type), then
>> releases it and enslaves ARPHRD_ETHER device (changes back) then we
>> use ether_setup() to restore the bond device type but it also resets its
>> flags and removes IFF_MASTER and IFF_SLAVE[1]. Use the bond_ether_setup
>> helper to restore both after such transition.
>>
>> [1] reproduce (nlmon is non-ARPHRD_ETHER):
>>  $ ip l add nlmon0 type nlmon
>>  $ ip l add bond2 type bond mode active-backup
>>  $ ip l set nlmon0 master bond2
>>  $ ip l set nlmon0 nomaster
>>  $ ip l add bond1 type bond
>>  (we use bond1 as ARPHRD_ETHER device to restore bond2's mode)
>>  $ ip l set bond1 master bond2
>>  $ ip l sh dev bond2
>>  37: bond2: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>>     link/ether be:d7:c5:40:5b:cc brd ff:ff:ff:ff:ff:ff promiscuity 0 minmtu 68 maxmtu 1500
>>  (notice bond2's IFF_MASTER is missing)
>>
>> Fixes: e36b9d16c6a6 ("bonding: clean muticast addresses when device changes type")
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>>  drivers/net/bonding/bond_main.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index d41024ad2c18..cd94baccdac5 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1878,10 +1878,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>>  
>>  			if (slave_dev->type != ARPHRD_ETHER)
>>  				bond_setup_by_slave(bond_dev, slave_dev);
>> -			else {
>> -				ether_setup(bond_dev);
>> -				bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>> -			}
>> +			else
>> +				bond_ether_setup(bond_dev);
> 
> As I already commented on your previous patch: there is the first call
> of "bond_ether_setup()".
> Please think about merging this patch with the previous one to avoid the
> compilation warning.
> 
> Thanks,
> Michal
> 

Please check my replies to your comment of the first patch.

Thanks

Re: [PATCH net v2 0/4] bonding: properly restore flags when bond changes ether type

[Jonathan Toppins]

On 3/14/23 07:14, Nikolay Aleksandrov wrote:
> Hi,
> A bug was reported by syzbot[1] that causes a warning and a myriad of
> other potential issues if a bond, that is also a slave, fails to enslave a
> non-eth device. While fixing that bug I found that we have the same
> issues when such enslave passes and after that the bond changes back to
> ARPHRD_ETHER (again due to ether_setup). This set fixes all issues by
> extracting the ether_setup() sequence in a helper which does the right
> thing about bond flags when it needs to change back to ARPHRD_ETHER. It
> also adds selftests for these cases.
> 
> Patch 01 adds the new bond_ether_setup helper that is used in the
> following patches to fix the bond dev flag issues. Patch 02 fixes the
> issues when a bond device changes its ether type due to successful
> enslave. Patch 03 fixes the issues when it changes its ether type due to
> an unsuccessful enslave. Note we need two patches because the bugs were
> introduced by different commits. Patch 04 adds the new selftests.
> 
> v2: new set, all patches are new due to new approach of fixing these bugs
> 
> Thanks,
>   Nik
> 
> [1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
> 
> Nikolay Aleksandrov (4):
>    bonding: add bond_ether_setup helper
>    bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type
>      change
>    bonding: restore bond's IFF_SLAVE flag if a non-eth dev enslave fails
>    selftests: bonding: add tests for ether type changes
> 
>   drivers/net/bonding/bond_main.c               | 22 +++--
>   .../selftests/drivers/net/bonding/Makefile    |  3 +-
>   .../net/bonding/bond-eth-type-change.sh       | 85 +++++++++++++++++++
>   3 files changed, 102 insertions(+), 8 deletions(-)
>   create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-eth-type-change.sh
> 

For the series.
Acked-by: Jonathan Toppins <jtoppins@redhat.com>

Re: [PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Jay Vosburgh]

Nikolay Aleksandrov <razor@blackwall.org> wrote:

>Add bond_ether_setup helper which will be used in the following patches
>to fix all ether_setup() calls in the bonding driver. It takes care of both
>IFF_MASTER and IFF_SLAVE flags, the former is always restored and the
>latter only if it was set.
>
>Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type")
>Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
>Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>---
> drivers/net/bonding/bond_main.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 00646aa315c3..d41024ad2c18 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave)
> 		slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);	\
> } while (0)
> 
>+/* ether_setup() resets bond_dev's flags so we always have to restore
>+ * IFF_MASTER, and only restore IFF_SLAVE if it was set
>+ */
>+static void bond_ether_setup(struct net_device *bond_dev)
>+{
>+	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
>+
>+	ether_setup(bond_dev);
>+	bond_dev->flags |= IFF_MASTER | slave_flag;
>+	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>+}

	Is setting IFF_MASTER always correct here?  I note that patch #2
is replacing code that does not set IFF_MASTER, whereas patch #3 is
replacing code that does set IFF_MASTER.

	Presuming that this is the desired behavior, perhaps mention
explicitly in the commentary that bond_ether_setup() is only for use on
a bond master device.  The nomenclature "bond_dev" does imply that, but
it's not explicit.

	Also, why is the call to ether_setup() from bond_setup() not
also being converted to bond_ether_setup()?

	-J

>+
> /* enslave device <slave> to bond device <master> */
> int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> 		 struct netlink_ext_ack *extack)
>-- 
>2.39.2
>

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

Re: [PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Nikolay Aleksandrov]

On 14/03/2023 17:34, Jay Vosburgh wrote:
> Nikolay Aleksandrov <razor@blackwall.org> wrote:
> 
>> Add bond_ether_setup helper which will be used in the following patches
>> to fix all ether_setup() calls in the bonding driver. It takes care of both
>> IFF_MASTER and IFF_SLAVE flags, the former is always restored and the
>> latter only if it was set.
>>
>> Fixes: e36b9d16c6a6d ("bonding: clean muticast addresses when device changes type")
>> Fixes: 7d5cd2ce5292 ("bonding: correctly handle bonding type change on enslave failure")
>> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
>> ---
>> drivers/net/bonding/bond_main.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 00646aa315c3..d41024ad2c18 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1775,6 +1775,18 @@ void bond_lower_state_changed(struct slave *slave)
>> 		slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);	\
>> } while (0)
>>
>> +/* ether_setup() resets bond_dev's flags so we always have to restore
>> + * IFF_MASTER, and only restore IFF_SLAVE if it was set
>> + */
>> +static void bond_ether_setup(struct net_device *bond_dev)
>> +{
>> +	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
>> +
>> +	ether_setup(bond_dev);
>> +	bond_dev->flags |= IFF_MASTER | slave_flag;
>> +	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>> +}
> 
> 	Is setting IFF_MASTER always correct here?  I note that patch #2
> is replacing code that does not set IFF_MASTER, whereas patch #3 is
> replacing code that does set IFF_MASTER.
> 
> 	Presuming that this is the desired behavior, perhaps mention
> explicitly in the commentary that bond_ether_setup() is only for use on
> a bond master device.  The nomenclature "bond_dev" does imply that, but
> it's not explicit.
> 

Setting IFF_MASTER is always correct because we're talking about a bond master device.
I.e. we're restoring the flags to a bond device itself. The bugs are different because
previously I had fixed the error path (partly, missed the IFF_SLAVE), but I just noticed
the normal enslave path while fixing the IFF_SLAVE one now. :)
So yes, both paths need the same treatment for both flags.

> 	Also, why is the call to ether_setup() from bond_setup() not
> also being converted to bond_ether_setup()?

That is more of a cleanup, the one there is correct because flags are set after that.
In that case only IFF_MASTER is needed, IFF_SLAVE cannot be set. Once these are
merged in net-next I'll send a followup to use it there, too.

> 
> 	-J
> 

Thanks,
 Nik

>> +
>> /* enslave device <slave> to bond device <master> */
>> int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
>> 		 struct netlink_ext_ack *extack)
>> -- 
>> 2.39.2
>>
> 
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net v2 0/4] bonding: properly restore flags when bond changes ether type

[Jay Vosburgh]

Nikolay Aleksandrov <razor@blackwall.org> wrote:

>Hi,
>A bug was reported by syzbot[1] that causes a warning and a myriad of
>other potential issues if a bond, that is also a slave, fails to enslave a
>non-eth device. While fixing that bug I found that we have the same
>issues when such enslave passes and after that the bond changes back to
>ARPHRD_ETHER (again due to ether_setup). This set fixes all issues by
>extracting the ether_setup() sequence in a helper which does the right
>thing about bond flags when it needs to change back to ARPHRD_ETHER. It
>also adds selftests for these cases.
>
>Patch 01 adds the new bond_ether_setup helper that is used in the
>following patches to fix the bond dev flag issues. Patch 02 fixes the
>issues when a bond device changes its ether type due to successful
>enslave. Patch 03 fixes the issues when it changes its ether type due to
>an unsuccessful enslave. Note we need two patches because the bugs were
>introduced by different commits. Patch 04 adds the new selftests.
>
>v2: new set, all patches are new due to new approach of fixing these bugs

	For the series:

Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>


>Thanks,
> Nik
>
>[1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
>
>Nikolay Aleksandrov (4):
>  bonding: add bond_ether_setup helper
>  bonding: restore IFF_MASTER/SLAVE flags on bond enslave ether type
>    change
>  bonding: restore bond's IFF_SLAVE flag if a non-eth dev enslave fails
>  selftests: bonding: add tests for ether type changes
>
> drivers/net/bonding/bond_main.c               | 22 +++--
> .../selftests/drivers/net/bonding/Makefile    |  3 +-
> .../net/bonding/bond-eth-type-change.sh       | 85 +++++++++++++++++++
> 3 files changed, 102 insertions(+), 8 deletions(-)
> create mode 100755 tools/testing/selftests/drivers/net/bonding/bond-eth-type-change.sh
>
>-- 
>2.39.2
>

Re: [PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Jakub Kicinski]

On Tue, 14 Mar 2023 13:14:23 +0200 Nikolay Aleksandrov wrote:
> +/* ether_setup() resets bond_dev's flags so we always have to restore
> + * IFF_MASTER, and only restore IFF_SLAVE if it was set
> + */
> +static void bond_ether_setup(struct net_device *bond_dev)
> +{
> +	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
> +
> +	ether_setup(bond_dev);
> +	bond_dev->flags |= IFF_MASTER | slave_flag;
> +	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
> +}
> +

We can't split this from patch 2, it's going to generate a warning
under normal build flags, people may have WERROR set these days..

Re: [PATCH net v2 1/4] bonding: add bond_ether_setup helper

[Nikolay Aleksandrov]

On 15/03/2023 09:55, Jakub Kicinski wrote:
> On Tue, 14 Mar 2023 13:14:23 +0200 Nikolay Aleksandrov wrote:
>> +/* ether_setup() resets bond_dev's flags so we always have to restore
>> + * IFF_MASTER, and only restore IFF_SLAVE if it was set
>> + */
>> +static void bond_ether_setup(struct net_device *bond_dev)
>> +{
>> +	unsigned int slave_flag = bond_dev->flags & IFF_SLAVE;
>> +
>> +	ether_setup(bond_dev);
>> +	bond_dev->flags |= IFF_MASTER | slave_flag;
>> +	bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
>> +}
>> +
> 
> We can't split this from patch 2, it's going to generate a warning
> under normal build flags, people may have WERROR set these days..

Oh well, that's unfortunate. I'll leave a note in patch 03 that it depends
on the helper added in the other fix.

Thanks,
 Nik