* [PATCH net 1/2] bonding: restore bond's IFF_SLAVE flag if a non-eth dev enslave fails
2023-03-13 13:28 [PATCH net 0/2] bonding: properly restore flags when non-eth dev enslave fails Nikolay Aleksandrov
@ 2023-03-13 13:28 ` Nikolay Aleksandrov
2023-03-13 13:28 ` [PATCH net 2/2] selftests: rtnetlink: add a bond test trying to enslave non-eth dev Nikolay Aleksandrov
2023-03-13 13:42 ` [PATCH net 0/2] bonding: properly restore flags when non-eth dev enslave fails Nikolay Aleksandrov
2 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2023-03-13 13:28 UTC (permalink / raw)
To: netdev
Cc: syoshida, j.vosburgh, andy, kuba, davem, pabeni, edumazet,
syzbot+9dfc3f3348729cc82277, 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.
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 | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 00646aa315c3..0d12b5ba4bf3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2288,9 +2288,15 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
slave_dev->dev_addr))
eth_hw_addr_random(bond_dev);
if (bond_dev->type != ARPHRD_ETHER) {
+ unsigned int restore_flags = bond_dev->flags &
+ (IFF_MASTER | IFF_SLAVE);
+
dev_close(bond_dev);
+ /* ether_setup() will reset bond_dev's flags, we need to
+ * restore IFF_MASTER, and IFF_SLAVE if it was set
+ */
ether_setup(bond_dev);
- bond_dev->flags |= IFF_MASTER;
+ bond_dev->flags |= restore_flags;
bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
}
}
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net 2/2] selftests: rtnetlink: add a bond test trying to enslave non-eth dev
2023-03-13 13:28 [PATCH net 0/2] bonding: properly restore flags when non-eth dev enslave fails Nikolay Aleksandrov
2023-03-13 13:28 ` [PATCH net 1/2] bonding: restore bond's IFF_SLAVE flag if a " Nikolay Aleksandrov
@ 2023-03-13 13:28 ` Nikolay Aleksandrov
2023-03-13 13:32 ` Nikolay Aleksandrov
2023-03-13 13:42 ` [PATCH net 0/2] bonding: properly restore flags when non-eth dev enslave fails Nikolay Aleksandrov
2 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2023-03-13 13:28 UTC (permalink / raw)
To: netdev
Cc: syoshida, j.vosburgh, andy, kuba, davem, pabeni, edumazet,
syzbot+9dfc3f3348729cc82277, Nikolay Aleksandrov
Add a new selftest for the recent bonding bug hit by syzbot[1] which
causes a warning and results in wrong device flags (IFF_SLAVE missing).
The test adds two bond devices and a nlmon device, enslaves one of the
bond devices to the other and then tries to enslave the nlmon device to
the enslaved bond testing the bond_enslave() error path when trying to
enslave a non-eth device. It checks that both MASTER and SLAVE flags are
properly restored.
If the flags are properly restored we get:
PASS: enslaved bond device has flags restored properly
[1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
Cc: Shigeru Yoshida <syoshida@redhat.com>
Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
---
tools/testing/selftests/net/rtnetlink.sh | 36 ++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index 275491be3da2..02964b2afd8d 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -1225,6 +1225,40 @@ kci_test_bridge_parent_id()
echo "PASS: bridge_parent_id"
}
+kci_test_enslaved_bond_non_eth()
+{
+ local ret=0
+
+ ip link add name test-nlmon0 type nlmon
+ ip link add name test-bond0 type bond
+ ip link add name test-bond1 type bond
+ ip link set dev test-bond0 master test-bond1
+ ip link set dev test-nlmon0 master test-bond0 1>/dev/null 2>/dev/null
+
+ ip -d l sh dev test-bond0 | grep -q "SLAVE"
+ if [ $? -ne 0 ]; then
+ echo "FAIL: IFF_SLAVE flag is missing from the bond device"
+ check_err 1
+ fi
+ ip -d l sh dev test-bond0 | grep -q "MASTER"
+ if [ $? -ne 0 ]; then
+ echo "FAIL: IFF_MASTER flag is missing from the bond device"
+ check_err 1
+ fi
+
+ # on error we return before cleaning up as that may hang the system
+ if [ $ret -ne 0 ]; then
+ return 1
+ fi
+
+ # clean up any leftovers
+ ip link del dev test-bond0
+ ip link del dev test-bond1
+ ip link del dev test-nlmon0
+
+ echo "PASS: enslaved bond device has flags restored properly"
+}
+
kci_test_rtnl()
{
local ret=0
@@ -1276,6 +1310,8 @@ kci_test_rtnl()
check_err $?
kci_test_bridge_parent_id
check_err $?
+ kci_test_enslaved_bond_non_eth
+ check_err $?
kci_del_dummy
return $ret
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 2/2] selftests: rtnetlink: add a bond test trying to enslave non-eth dev
2023-03-13 13:28 ` [PATCH net 2/2] selftests: rtnetlink: add a bond test trying to enslave non-eth dev Nikolay Aleksandrov
@ 2023-03-13 13:32 ` Nikolay Aleksandrov
2023-03-13 13:34 ` Nikolay Aleksandrov
0 siblings, 1 reply; 6+ messages in thread
From: Nikolay Aleksandrov @ 2023-03-13 13:32 UTC (permalink / raw)
To: netdev
Cc: syoshida, j.vosburgh, andy, kuba, davem, pabeni, edumazet,
syzbot+9dfc3f3348729cc82277
On 13/03/2023 15:28, Nikolay Aleksandrov wrote:
[snip]
> +kci_test_enslaved_bond_non_eth()
> +{
> + local ret=0
> +
> + ip link add name test-nlmon0 type nlmon
> + ip link add name test-bond0 type bond
> + ip link add name test-bond1 type bond
> + ip link set dev test-bond0 master test-bond1
> + ip link set dev test-nlmon0 master test-bond0 1>/dev/null 2>/dev/null
> +
> + ip -d l sh dev test-bond0 | grep -q "SLAVE"
> + if [ $? -ne 0 ]; then
> + echo "FAIL: IFF_SLAVE flag is missing from the bond device"
> + check_err 1
> + fi
> + ip -d l sh dev test-bond0 | grep -q "MASTER"
> + if [ $? -ne 0 ]; then
> + echo "FAIL: IFF_MASTER flag is missing from the bond device"
> + check_err 1
> + fi
> +
> + # on error we return before cleaning up as that may hang the system
I wasn't sure if this part was ok, let me know if you prefer to always attempt cleaning up
and I'll send v2 moving the return after the cleanup attempt.
> + if [ $ret -ne 0 ]; then
> + return 1
> + fi
> +
> + # clean up any leftovers
> + ip link del dev test-bond0
> + ip link del dev test-bond1
> + ip link del dev test-nlmon0
> +
> + echo "PASS: enslaved bond device has flags restored properly"
> +}
> +
> kci_test_rtnl()
> {
> local ret=0
> @@ -1276,6 +1310,8 @@ kci_test_rtnl()
> check_err $?
> kci_test_bridge_parent_id
> check_err $?
> + kci_test_enslaved_bond_non_eth
> + check_err $?
>
> kci_del_dummy
> return $ret
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net 2/2] selftests: rtnetlink: add a bond test trying to enslave non-eth dev
2023-03-13 13:32 ` Nikolay Aleksandrov
@ 2023-03-13 13:34 ` Nikolay Aleksandrov
0 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2023-03-13 13:34 UTC (permalink / raw)
To: netdev
Cc: syoshida, j.vosburgh, andy, kuba, davem, pabeni, edumazet,
syzbot+9dfc3f3348729cc82277
On 13/03/2023 15:32, Nikolay Aleksandrov wrote:
> On 13/03/2023 15:28, Nikolay Aleksandrov wrote:
> [snip]
>> +kci_test_enslaved_bond_non_eth()
>> +{
>> + local ret=0
>> +
>> + ip link add name test-nlmon0 type nlmon
>> + ip link add name test-bond0 type bond
>> + ip link add name test-bond1 type bond
>> + ip link set dev test-bond0 master test-bond1
>> + ip link set dev test-nlmon0 master test-bond0 1>/dev/null 2>/dev/null
>> +
>> + ip -d l sh dev test-bond0 | grep -q "SLAVE"
>> + if [ $? -ne 0 ]; then
>> + echo "FAIL: IFF_SLAVE flag is missing from the bond device"
>> + check_err 1
>> + fi
>> + ip -d l sh dev test-bond0 | grep -q "MASTER"
>> + if [ $? -ne 0 ]; then
>> + echo "FAIL: IFF_MASTER flag is missing from the bond device"
>> + check_err 1
>> + fi
>> +
>> + # on error we return before cleaning up as that may hang the system
>
> I wasn't sure if this part was ok, let me know if you prefer to always attempt cleaning up
> and I'll send v2 moving the return after the cleanup attempt.
>
Actually sorry for the noise, I'll just send v2 instead with that change.
I don't have a good argument why we shouldn't attempt a cleanup after printing
the error messages even if the system hangs.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 0/2] bonding: properly restore flags when non-eth dev enslave fails
2023-03-13 13:28 [PATCH net 0/2] bonding: properly restore flags when non-eth dev enslave fails Nikolay Aleksandrov
2023-03-13 13:28 ` [PATCH net 1/2] bonding: restore bond's IFF_SLAVE flag if a " Nikolay Aleksandrov
2023-03-13 13:28 ` [PATCH net 2/2] selftests: rtnetlink: add a bond test trying to enslave non-eth dev Nikolay Aleksandrov
@ 2023-03-13 13:42 ` Nikolay Aleksandrov
2 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2023-03-13 13:42 UTC (permalink / raw)
To: netdev
Cc: syoshida, j.vosburgh, andy, kuba, davem, pabeni, edumazet,
syzbot+9dfc3f3348729cc82277
On 13/03/2023 15:28, 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. Patch 01 fixes the bug and patch 02 adds a selftest for
> bond flags restoration in such cases. For more information please see
> commit descriptions.
>
> Thanks,
> Nik
>
> [1] https://syzkaller.appspot.com/bug?id=391c7b1f6522182899efba27d891f1743e8eb3ef
>
> Nikolay Aleksandrov (2):
> bonding: restore bond's IFF_SLAVE flag if a non-eth dev
> enslave fails
> selftests: rtnetlink: add a bond test trying to enslave non-eth dev
>
> drivers/net/bonding/bond_main.c | 8 +++++-
> tools/testing/selftests/net/rtnetlink.sh | 36 ++++++++++++++++++++++++
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
Self-NAK, apologies for the noise. I'll send a v2 with the selftest adjusted to
always to attempt a cleanup after printing the errors regardless if the system hangs.
^ permalink raw reply [flat|nested] 6+ messages in thread