* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-09 18:01 [PATCH net] bonding: stop the device in bond_setup_by_slave() Eric Dumazet
@ 2023-11-09 20:26 ` Jay Vosburgh
2023-11-10 3:44 ` Hangbin Liu
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Jay Vosburgh @ 2023-11-09 20:26 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Andy Gospodarek,
netdev, eric.dumazet, syzbot
Eric Dumazet <edumazet@google.com> wrote:
>Commit 9eed321cde22 ("net: lapbether: only support ethernet devices")
>has been able to keep syzbot away from net/lapb, until today.
>
>In the following splat [1], the issue is that a lapbether device has
>been created on a bonding device without members. Then adding a non
>ARPHRD_ETHER member forced the bonding master to change its type.
>
>The fix is to make sure we call dev_close() in bond_setup_by_slave()
>so that the potential linked lapbether devices (or any other devices
>having assumptions on the physical device) are removed.
>
>A similar bug has been addressed in commit 40baec225765
>("bonding: fix panic on non-ARPHRD_ETHER enslave failure")
>
>[1]
>skbuff: skb_under_panic: text:ffff800089508810 len:44 put:40 head:ffff0000c78e7c00 data:ffff0000c78e7bea tail:0x16 end:0x140 dev:bond0
>kernel BUG at net/core/skbuff.c:192 !
>Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>Modules linked in:
>CPU: 0 PID: 6007 Comm: syz-executor383 Not tainted 6.6.0-rc3-syzkaller-gbf6547d8715b #0
>Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/04/2023
>pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>pc : skb_panic net/core/skbuff.c:188 [inline]
>pc : skb_under_panic+0x13c/0x140 net/core/skbuff.c:202
>lr : skb_panic net/core/skbuff.c:188 [inline]
>lr : skb_under_panic+0x13c/0x140 net/core/skbuff.c:202
>sp : ffff800096a06aa0
>x29: ffff800096a06ab0 x28: ffff800096a06ba0 x27: dfff800000000000
>x26: ffff0000ce9b9b50 x25: 0000000000000016 x24: ffff0000c78e7bea
>x23: ffff0000c78e7c00 x22: 000000000000002c x21: 0000000000000140
>x20: 0000000000000028 x19: ffff800089508810 x18: ffff800096a06100
>x17: 0000000000000000 x16: ffff80008a629a3c x15: 0000000000000001
>x14: 1fffe00036837a32 x13: 0000000000000000 x12: 0000000000000000
>x11: 0000000000000201 x10: 0000000000000000 x9 : cb50b496c519aa00
>x8 : cb50b496c519aa00 x7 : 0000000000000001 x6 : 0000000000000001
>x5 : ffff800096a063b8 x4 : ffff80008e280f80 x3 : ffff8000805ad11c
>x2 : 0000000000000001 x1 : 0000000100000201 x0 : 0000000000000086
>Call trace:
>skb_panic net/core/skbuff.c:188 [inline]
>skb_under_panic+0x13c/0x140 net/core/skbuff.c:202
>skb_push+0xf0/0x108 net/core/skbuff.c:2446
>ip6gre_header+0xbc/0x738 net/ipv6/ip6_gre.c:1384
>dev_hard_header include/linux/netdevice.h:3136 [inline]
>lapbeth_data_transmit+0x1c4/0x298 drivers/net/wan/lapbether.c:257
>lapb_data_transmit+0x8c/0xb0 net/lapb/lapb_iface.c:447
>lapb_transmit_buffer+0x178/0x204 net/lapb/lapb_out.c:149
>lapb_send_control+0x220/0x320 net/lapb/lapb_subr.c:251
>__lapb_disconnect_request+0x9c/0x17c net/lapb/lapb_iface.c:326
>lapb_device_event+0x288/0x4e0 net/lapb/lapb_iface.c:492
>notifier_call_chain+0x1a4/0x510 kernel/notifier.c:93
>raw_notifier_call_chain+0x3c/0x50 kernel/notifier.c:461
>call_netdevice_notifiers_info net/core/dev.c:1970 [inline]
>call_netdevice_notifiers_extack net/core/dev.c:2008 [inline]
>call_netdevice_notifiers net/core/dev.c:2022 [inline]
>__dev_close_many+0x1b8/0x3c4 net/core/dev.c:1508
>dev_close_many+0x1e0/0x470 net/core/dev.c:1559
>dev_close+0x174/0x250 net/core/dev.c:1585
>lapbeth_device_event+0x2e4/0x958 drivers/net/wan/lapbether.c:466
>notifier_call_chain+0x1a4/0x510 kernel/notifier.c:93
>raw_notifier_call_chain+0x3c/0x50 kernel/notifier.c:461
>call_netdevice_notifiers_info net/core/dev.c:1970 [inline]
>call_netdevice_notifiers_extack net/core/dev.c:2008 [inline]
>call_netdevice_notifiers net/core/dev.c:2022 [inline]
>__dev_close_many+0x1b8/0x3c4 net/core/dev.c:1508
>dev_close_many+0x1e0/0x470 net/core/dev.c:1559
>dev_close+0x174/0x250 net/core/dev.c:1585
>bond_enslave+0x2298/0x30cc drivers/net/bonding/bond_main.c:2332
>bond_do_ioctl+0x268/0xc64 drivers/net/bonding/bond_main.c:4539
>dev_ifsioc+0x754/0x9ac
>dev_ioctl+0x4d8/0xd34 net/core/dev_ioctl.c:786
>sock_do_ioctl+0x1d4/0x2d0 net/socket.c:1217
>sock_ioctl+0x4e8/0x834 net/socket.c:1322
>vfs_ioctl fs/ioctl.c:51 [inline]
>__do_sys_ioctl fs/ioctl.c:871 [inline]
>__se_sys_ioctl fs/ioctl.c:857 [inline]
>__arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:857
>__invoke_syscall arch/arm64/kernel/syscall.c:37 [inline]
>invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:51
>el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:136
>do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:155
>el0_svc+0x58/0x16c arch/arm64/kernel/entry-common.c:678
>el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:696
>el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591
>Code: aa1803e6 aa1903e7 a90023f5 94785b8b (d4210000)
>
>Fixes: 872254dd6b1f ("net/bonding: Enable bonding to enslave non ARPHRD_ETHER")
>Reported-by: syzbot <syzkaller@googlegroups.com>
>Signed-off-by: Eric Dumazet <edumazet@google.com>
I was initially worred that the close / open dance was on the
regular path, but it's only for the non-ARPHRD_ETHER case. That's
really for Infiniband IPoIB, and I'm not sure that there is anything
that can be stacked atop an IPoIB bond.
Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>
-J
>---
> drivers/net/bonding/bond_main.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 51d47eda1c873debda6da094377bcb3367a78f6e..8e6cc0e133b7f19afccd3ecf44bea5ceacb393b1 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1500,6 +1500,10 @@ static void bond_compute_features(struct bonding *bond)
> static void bond_setup_by_slave(struct net_device *bond_dev,
> struct net_device *slave_dev)
> {
>+ bool was_up = !!(bond_dev->flags & IFF_UP);
>+
>+ dev_close(bond_dev);
>+
> bond_dev->header_ops = slave_dev->header_ops;
>
> bond_dev->type = slave_dev->type;
>@@ -1514,6 +1518,8 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
> bond_dev->flags &= ~(IFF_BROADCAST | IFF_MULTICAST);
> bond_dev->flags |= (IFF_POINTOPOINT | IFF_NOARP);
> }
>+ if (was_up)
>+ dev_open(bond_dev, NULL);
> }
>
> /* On bonding slaves other than the currently active slave, suppress
>--
>2.42.0.869.gea05f2083d-goog
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-09 18:01 [PATCH net] bonding: stop the device in bond_setup_by_slave() Eric Dumazet
2023-11-09 20:26 ` Jay Vosburgh
@ 2023-11-10 3:44 ` Hangbin Liu
2023-11-10 8:38 ` Eric Dumazet
2023-11-10 9:19 ` Jay Vosburgh
2023-11-14 1:30 ` Hangbin Liu
2023-11-14 5:00 ` patchwork-bot+netdevbpf
3 siblings, 2 replies; 10+ messages in thread
From: Hangbin Liu @ 2023-11-10 3:44 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Jay Vosburgh,
Andy Gospodarek, netdev, eric.dumazet, syzbot
On Thu, Nov 09, 2023 at 06:01:02PM +0000, Eric Dumazet wrote:
> Commit 9eed321cde22 ("net: lapbether: only support ethernet devices")
> has been able to keep syzbot away from net/lapb, until today.
>
> In the following splat [1], the issue is that a lapbether device has
> been created on a bonding device without members. Then adding a non
> ARPHRD_ETHER member forced the bonding master to change its type.
>
> The fix is to make sure we call dev_close() in bond_setup_by_slave()
> so that the potential linked lapbether devices (or any other devices
> having assumptions on the physical device) are removed.
>
> A similar bug has been addressed in commit 40baec225765
> ("bonding: fix panic on non-ARPHRD_ETHER enslave failure")
>
Do we need also do this if the bond changed to ether device from other dev
type? e.g.
if (slave_dev->type != ARPHRD_ETHER)
bond_setup_by_slave(bond_dev, slave_dev);
else
bond_ether_setup(bond_dev);
Thanks
Hangbin
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-10 3:44 ` Hangbin Liu
@ 2023-11-10 8:38 ` Eric Dumazet
2023-11-11 6:28 ` Hangbin Liu
2023-11-10 9:19 ` Jay Vosburgh
1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2023-11-10 8:38 UTC (permalink / raw)
To: Hangbin Liu
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Jay Vosburgh,
Andy Gospodarek, netdev, eric.dumazet, syzbot
On Fri, Nov 10, 2023 at 4:44 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Thu, Nov 09, 2023 at 06:01:02PM +0000, Eric Dumazet wrote:
> > Commit 9eed321cde22 ("net: lapbether: only support ethernet devices")
> > has been able to keep syzbot away from net/lapb, until today.
> >
> > In the following splat [1], the issue is that a lapbether device has
> > been created on a bonding device without members. Then adding a non
> > ARPHRD_ETHER member forced the bonding master to change its type.
> >
> > The fix is to make sure we call dev_close() in bond_setup_by_slave()
> > so that the potential linked lapbether devices (or any other devices
> > having assumptions on the physical device) are removed.
> >
> > A similar bug has been addressed in commit 40baec225765
> > ("bonding: fix panic on non-ARPHRD_ETHER enslave failure")
> >
>
> Do we need also do this if the bond changed to ether device from other dev
> type? e.g.
>
> if (slave_dev->type != ARPHRD_ETHER)
> bond_setup_by_slave(bond_dev, slave_dev);
> else
> bond_ether_setup(bond_dev);
Hmmm... possibly, but as far as I know, nothing can be stacked on top of IPoIB
Note that another way to deal with this in a fine grained way is to
return NOTIFY_BAD
from NETDEV_PRE_TYPE_CHANGE event (vlan, macvlan, ipvlan ...)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-10 8:38 ` Eric Dumazet
@ 2023-11-11 6:28 ` Hangbin Liu
0 siblings, 0 replies; 10+ messages in thread
From: Hangbin Liu @ 2023-11-11 6:28 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Jay Vosburgh,
Andy Gospodarek, netdev, eric.dumazet, syzbot
On Fri, Nov 10, 2023 at 09:38:18AM +0100, Eric Dumazet wrote:
> On Fri, Nov 10, 2023 at 4:44 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
> >
> > On Thu, Nov 09, 2023 at 06:01:02PM +0000, Eric Dumazet wrote:
> > > Commit 9eed321cde22 ("net: lapbether: only support ethernet devices")
> > > has been able to keep syzbot away from net/lapb, until today.
> > >
> > > In the following splat [1], the issue is that a lapbether device has
> > > been created on a bonding device without members. Then adding a non
> > > ARPHRD_ETHER member forced the bonding master to change its type.
> > >
> > > The fix is to make sure we call dev_close() in bond_setup_by_slave()
> > > so that the potential linked lapbether devices (or any other devices
> > > having assumptions on the physical device) are removed.
> > >
> > > A similar bug has been addressed in commit 40baec225765
> > > ("bonding: fix panic on non-ARPHRD_ETHER enslave failure")
> > >
> >
> > Do we need also do this if the bond changed to ether device from other dev
> > type? e.g.
> >
> > if (slave_dev->type != ARPHRD_ETHER)
> > bond_setup_by_slave(bond_dev, slave_dev);
> > else
> > bond_ether_setup(bond_dev);
>
> Hmmm... possibly, but as far as I know, nothing can be stacked on top of IPoIB
The "stacked on top of IPoIB", do you mean IPoIB as an up layer or down layer
device?
BTW, not only IPoIB can be enslaved to bond, but also other types like gre could be
enslaved to bond.
Thanks
Hangbin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-10 3:44 ` Hangbin Liu
2023-11-10 8:38 ` Eric Dumazet
@ 2023-11-10 9:19 ` Jay Vosburgh
2023-11-11 6:34 ` Hangbin Liu
1 sibling, 1 reply; 10+ messages in thread
From: Jay Vosburgh @ 2023-11-10 9:19 UTC (permalink / raw)
To: Hangbin Liu
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni,
Andy Gospodarek, netdev, eric.dumazet, syzbot
Hangbin Liu <liuhangbin@gmail.com> wrote:
>On Thu, Nov 09, 2023 at 06:01:02PM +0000, Eric Dumazet wrote:
>> Commit 9eed321cde22 ("net: lapbether: only support ethernet devices")
>> has been able to keep syzbot away from net/lapb, until today.
>>
>> In the following splat [1], the issue is that a lapbether device has
>> been created on a bonding device without members. Then adding a non
>> ARPHRD_ETHER member forced the bonding master to change its type.
>>
>> The fix is to make sure we call dev_close() in bond_setup_by_slave()
>> so that the potential linked lapbether devices (or any other devices
>> having assumptions on the physical device) are removed.
>>
>> A similar bug has been addressed in commit 40baec225765
>> ("bonding: fix panic on non-ARPHRD_ETHER enslave failure")
>>
>
>Do we need also do this if the bond changed to ether device from other dev
>type? e.g.
>
> if (slave_dev->type != ARPHRD_ETHER)
> bond_setup_by_slave(bond_dev, slave_dev);
> else
> bond_ether_setup(bond_dev);
I'm not sure I follow your comment; bond_enslave() already has
the above logic. If the bond is not ARPHRD_ETHER and an ARPHRD_ETHER
device is added to the bond, the above will take the bond_ether_setup()
path, which will call ether_setup() which will set the device to
ARPHRD_ETHER.
However, my recollection is that the bond device itself should
be unregistered if the last interface of a non-ARPHRD_ETHER bond is
removed. This dates back to d90a162a4ee2 ("net/bonding: Destroy bonding
master when last slave is gone"), but I don't know if the logic still
works correctly (I've not heard much about IPoIB with bonding in a
while). The bond cannot be initially created as non-ARPHRD_ETHER; the
type changes when the first such interface is added to the bond.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-10 9:19 ` Jay Vosburgh
@ 2023-11-11 6:34 ` Hangbin Liu
2023-11-14 0:43 ` Jay Vosburgh
0 siblings, 1 reply; 10+ messages in thread
From: Hangbin Liu @ 2023-11-11 6:34 UTC (permalink / raw)
To: Jay Vosburgh
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni,
Andy Gospodarek, netdev, eric.dumazet, syzbot
On Fri, Nov 10, 2023 at 11:19:22AM +0200, Jay Vosburgh wrote:
> >Do we need also do this if the bond changed to ether device from other dev
> >type? e.g.
> >
> > if (slave_dev->type != ARPHRD_ETHER)
> > bond_setup_by_slave(bond_dev, slave_dev);
> > else
> > bond_ether_setup(bond_dev);
>
> I'm not sure I follow your comment; bond_enslave() already has
> the above logic. If the bond is not ARPHRD_ETHER and an ARPHRD_ETHER
> device is added to the bond, the above will take the bond_ether_setup()
> path, which will call ether_setup() which will set the device to
> ARPHRD_ETHER.
>
> However, my recollection is that the bond device itself should
> be unregistered if the last interface of a non-ARPHRD_ETHER bond is
> removed. This dates back to d90a162a4ee2 ("net/bonding: Destroy bonding
> master when last slave is gone"), but I don't know if the logic still
> works correctly (I've not heard much about IPoIB with bonding in a
> while). The bond cannot be initially created as non-ARPHRD_ETHER; the
> type changes when the first such interface is added to the bond.
Ah, thanks for this info. I just tried and it still works. Which looks
there is no need to close bond dev before bond_ether_setup().
BTW, I tried to set gre0's master to bond0 and change the types. After that,
`ip link del gre0` will return 0 but gre0 is actually not deleted. I have to
remove the gre mode to delete the link. Is that expected?
```
# ip link add bond0 type bond mode 1 miimon 100
# ip link add gre0 type gre
# ip link set gre0 master bond0
# ip link show bond0
21: bond0: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
link/gre 0.0.0.0 brd 0.0.0.0
# ip link del gre0
# echo $?
0
# ip link show gre0
18: gre0@NONE: <NOARP,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UNKNOWN mode DEFAULT group default qlen 1000
link/gre 0.0.0.0 brd 0.0.0.0
```
Thanks
Hangbin
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-11 6:34 ` Hangbin Liu
@ 2023-11-14 0:43 ` Jay Vosburgh
0 siblings, 0 replies; 10+ messages in thread
From: Jay Vosburgh @ 2023-11-14 0:43 UTC (permalink / raw)
To: Hangbin Liu
Cc: Eric Dumazet, David S . Miller, Jakub Kicinski, Paolo Abeni,
Andy Gospodarek, netdev, eric.dumazet, syzbot
Hangbin Liu <liuhangbin@gmail.com> wrote:
>On Fri, Nov 10, 2023 at 11:19:22AM +0200, Jay Vosburgh wrote:
>> >Do we need also do this if the bond changed to ether device from other dev
>> >type? e.g.
>> >
>> > if (slave_dev->type != ARPHRD_ETHER)
>> > bond_setup_by_slave(bond_dev, slave_dev);
>> > else
>> > bond_ether_setup(bond_dev);
>>
>> I'm not sure I follow your comment; bond_enslave() already has
>> the above logic. If the bond is not ARPHRD_ETHER and an ARPHRD_ETHER
>> device is added to the bond, the above will take the bond_ether_setup()
>> path, which will call ether_setup() which will set the device to
>> ARPHRD_ETHER.
>>
>> However, my recollection is that the bond device itself should
>> be unregistered if the last interface of a non-ARPHRD_ETHER bond is
>> removed. This dates back to d90a162a4ee2 ("net/bonding: Destroy bonding
>> master when last slave is gone"), but I don't know if the logic still
>> works correctly (I've not heard much about IPoIB with bonding in a
>> while). The bond cannot be initially created as non-ARPHRD_ETHER; the
>> type changes when the first such interface is added to the bond.
>
>Ah, thanks for this info. I just tried and it still works. Which looks
>there is no need to close bond dev before bond_ether_setup().
>
>BTW, I tried to set gre0's master to bond0 and change the types. After that,
>`ip link del gre0` will return 0 but gre0 is actually not deleted. I have to
>remove the gre mode to delete the link. Is that expected?
I don't think that's expected; I'd expect "ip link del" to
delete the interface after removing it from the bond (via the
NETDEV_UNREGISTER case in bond_slave_netdev_event).
-J
>```
># ip link add bond0 type bond mode 1 miimon 100
># ip link add gre0 type gre
># ip link set gre0 master bond0
># ip link show bond0
>21: bond0: <BROADCAST,MULTICAST,MASTER> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
> link/gre 0.0.0.0 brd 0.0.0.0
># ip link del gre0
># echo $?
>0
># ip link show gre0
>18: gre0@NONE: <NOARP,SLAVE,UP,LOWER_UP> mtu 1500 qdisc noqueue master bond0 state UNKNOWN mode DEFAULT group default qlen 1000
> link/gre 0.0.0.0 brd 0.0.0.0
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-09 18:01 [PATCH net] bonding: stop the device in bond_setup_by_slave() Eric Dumazet
2023-11-09 20:26 ` Jay Vosburgh
2023-11-10 3:44 ` Hangbin Liu
@ 2023-11-14 1:30 ` Hangbin Liu
2023-11-14 5:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 10+ messages in thread
From: Hangbin Liu @ 2023-11-14 1:30 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Jay Vosburgh,
Andy Gospodarek, netdev, eric.dumazet, syzbot
On Thu, Nov 09, 2023 at 06:01:02PM +0000, Eric Dumazet wrote:
> Commit 9eed321cde22 ("net: lapbether: only support ethernet devices")
> has been able to keep syzbot away from net/lapb, until today.
>
> In the following splat [1], the issue is that a lapbether device has
> been created on a bonding device without members. Then adding a non
> ARPHRD_ETHER member forced the bonding master to change its type.
>
> The fix is to make sure we call dev_close() in bond_setup_by_slave()
> so that the potential linked lapbether devices (or any other devices
> having assumptions on the physical device) are removed.
>
> A similar bug has been addressed in commit 40baec225765
> ("bonding: fix panic on non-ARPHRD_ETHER enslave failure")
>
> [1]
> skbuff: skb_under_panic: text:ffff800089508810 len:44 put:40 head:ffff0000c78e7c00 data:ffff0000c78e7bea tail:0x16 end:0x140 dev:bond0
> kernel BUG at net/core/skbuff.c:192 !
> Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 0 PID: 6007 Comm: syz-executor383 Not tainted 6.6.0-rc3-syzkaller-gbf6547d8715b #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/04/2023
> pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : skb_panic net/core/skbuff.c:188 [inline]
> pc : skb_under_panic+0x13c/0x140 net/core/skbuff.c:202
> lr : skb_panic net/core/skbuff.c:188 [inline]
> lr : skb_under_panic+0x13c/0x140 net/core/skbuff.c:202
> sp : ffff800096a06aa0
> x29: ffff800096a06ab0 x28: ffff800096a06ba0 x27: dfff800000000000
> x26: ffff0000ce9b9b50 x25: 0000000000000016 x24: ffff0000c78e7bea
> x23: ffff0000c78e7c00 x22: 000000000000002c x21: 0000000000000140
> x20: 0000000000000028 x19: ffff800089508810 x18: ffff800096a06100
> x17: 0000000000000000 x16: ffff80008a629a3c x15: 0000000000000001
> x14: 1fffe00036837a32 x13: 0000000000000000 x12: 0000000000000000
> x11: 0000000000000201 x10: 0000000000000000 x9 : cb50b496c519aa00
> x8 : cb50b496c519aa00 x7 : 0000000000000001 x6 : 0000000000000001
> x5 : ffff800096a063b8 x4 : ffff80008e280f80 x3 : ffff8000805ad11c
> x2 : 0000000000000001 x1 : 0000000100000201 x0 : 0000000000000086
> Call trace:
> skb_panic net/core/skbuff.c:188 [inline]
> skb_under_panic+0x13c/0x140 net/core/skbuff.c:202
> skb_push+0xf0/0x108 net/core/skbuff.c:2446
> ip6gre_header+0xbc/0x738 net/ipv6/ip6_gre.c:1384
> dev_hard_header include/linux/netdevice.h:3136 [inline]
> lapbeth_data_transmit+0x1c4/0x298 drivers/net/wan/lapbether.c:257
> lapb_data_transmit+0x8c/0xb0 net/lapb/lapb_iface.c:447
> lapb_transmit_buffer+0x178/0x204 net/lapb/lapb_out.c:149
> lapb_send_control+0x220/0x320 net/lapb/lapb_subr.c:251
> __lapb_disconnect_request+0x9c/0x17c net/lapb/lapb_iface.c:326
> lapb_device_event+0x288/0x4e0 net/lapb/lapb_iface.c:492
> notifier_call_chain+0x1a4/0x510 kernel/notifier.c:93
> raw_notifier_call_chain+0x3c/0x50 kernel/notifier.c:461
> call_netdevice_notifiers_info net/core/dev.c:1970 [inline]
> call_netdevice_notifiers_extack net/core/dev.c:2008 [inline]
> call_netdevice_notifiers net/core/dev.c:2022 [inline]
> __dev_close_many+0x1b8/0x3c4 net/core/dev.c:1508
> dev_close_many+0x1e0/0x470 net/core/dev.c:1559
> dev_close+0x174/0x250 net/core/dev.c:1585
> lapbeth_device_event+0x2e4/0x958 drivers/net/wan/lapbether.c:466
> notifier_call_chain+0x1a4/0x510 kernel/notifier.c:93
> raw_notifier_call_chain+0x3c/0x50 kernel/notifier.c:461
> call_netdevice_notifiers_info net/core/dev.c:1970 [inline]
> call_netdevice_notifiers_extack net/core/dev.c:2008 [inline]
> call_netdevice_notifiers net/core/dev.c:2022 [inline]
> __dev_close_many+0x1b8/0x3c4 net/core/dev.c:1508
> dev_close_many+0x1e0/0x470 net/core/dev.c:1559
> dev_close+0x174/0x250 net/core/dev.c:1585
> bond_enslave+0x2298/0x30cc drivers/net/bonding/bond_main.c:2332
> bond_do_ioctl+0x268/0xc64 drivers/net/bonding/bond_main.c:4539
> dev_ifsioc+0x754/0x9ac
> dev_ioctl+0x4d8/0xd34 net/core/dev_ioctl.c:786
> sock_do_ioctl+0x1d4/0x2d0 net/socket.c:1217
> sock_ioctl+0x4e8/0x834 net/socket.c:1322
> vfs_ioctl fs/ioctl.c:51 [inline]
> __do_sys_ioctl fs/ioctl.c:871 [inline]
> __se_sys_ioctl fs/ioctl.c:857 [inline]
> __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:857
> __invoke_syscall arch/arm64/kernel/syscall.c:37 [inline]
> invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:51
> el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:136
> do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:155
> el0_svc+0x58/0x16c arch/arm64/kernel/entry-common.c:678
> el0t_64_sync_handler+0x84/0xfc arch/arm64/kernel/entry-common.c:696
> el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:591
> Code: aa1803e6 aa1903e7 a90023f5 94785b8b (d4210000)
>
> Fixes: 872254dd6b1f ("net/bonding: Enable bonding to enslave non ARPHRD_ETHER")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> drivers/net/bonding/bond_main.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 51d47eda1c873debda6da094377bcb3367a78f6e..8e6cc0e133b7f19afccd3ecf44bea5ceacb393b1 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1500,6 +1500,10 @@ static void bond_compute_features(struct bonding *bond)
> static void bond_setup_by_slave(struct net_device *bond_dev,
> struct net_device *slave_dev)
> {
> + bool was_up = !!(bond_dev->flags & IFF_UP);
> +
> + dev_close(bond_dev);
> +
> bond_dev->header_ops = slave_dev->header_ops;
>
> bond_dev->type = slave_dev->type;
> @@ -1514,6 +1518,8 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
> bond_dev->flags &= ~(IFF_BROADCAST | IFF_MULTICAST);
> bond_dev->flags |= (IFF_POINTOPOINT | IFF_NOARP);
> }
> + if (was_up)
> + dev_open(bond_dev, NULL);
> }
>
> /* On bonding slaves other than the currently active slave, suppress
> --
> 2.42.0.869.gea05f2083d-goog
>
As Jay's comments, the bond device itself should be unregistered if the
last interface of a non-ARPHRD_ETHER bond is removed. There should no need
to close/open bond dev on the non-ARPHRD_ETHER -> ARPHRD_ETHER path. So
Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] bonding: stop the device in bond_setup_by_slave()
2023-11-09 18:01 [PATCH net] bonding: stop the device in bond_setup_by_slave() Eric Dumazet
` (2 preceding siblings ...)
2023-11-14 1:30 ` Hangbin Liu
@ 2023-11-14 5:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-11-14 5:00 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, kuba, pabeni, j.vosburgh, andy, netdev, eric.dumazet,
syzkaller
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Thu, 9 Nov 2023 18:01:02 +0000 you wrote:
> Commit 9eed321cde22 ("net: lapbether: only support ethernet devices")
> has been able to keep syzbot away from net/lapb, until today.
>
> In the following splat [1], the issue is that a lapbether device has
> been created on a bonding device without members. Then adding a non
> ARPHRD_ETHER member forced the bonding master to change its type.
>
> [...]
Here is the summary with links:
- [net] bonding: stop the device in bond_setup_by_slave()
https://git.kernel.org/netdev/net/c/3cffa2ddc4d3
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 10+ messages in thread