Re: [PATCH net] bonding: fix bond dev flags after convert to arphrd_ether

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
[...]
>Restarting this thread because there’s actually a bug here, what you described with
>the bonding destruction is true when the slaves are all destroyed but it isn’t true if they’re
>just released, if you take a look at bond_slave_netdev_event() the bond destruction happens
>only on NETDEV_UNREGISTER and I just hit this bug by enslaving a non-ARPHRD_ETHER
>device, releasing it and enslaving a ARPHRD_ETHER device so ether_setup() path in bond_enslave
>is hit and IFF_MASTER gets dropped:
>17: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>    link/fddi 9a:33:c5:30:ff:a6 brd ff:ff:ff:ff:ff:ff
>(release non-ARPHRD_ETHER slave)
>(enslave ARPHRD_ETHER device)
>17: bond0: <BROADCAST,MULTICAST,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
>    link/ether 08:00:27:3c:13:57 brd ff:ff:ff:ff:ff:ff
>
>Notice the master flag is gone and of course on unload we get:
>[57981.545547] ------------[ cut here ]------------
>[57981.545567] WARNING: CPU: 0 PID: 13792 at fs/proc/generic.c:575 remove_proc_entry+0x17e/0x190()
>[57981.545572] remove_proc_entry: removing non-empty directory 'net/bonding', leaking at least 'bond0'
[...]
>We need to convert it back to ARPHRD_ETHER if releasing the last slave, because
>we can’t destroy it (in some paths bond->dev is used after bond_release()).
>Basically we should make the case that if the bonding doesn’t have any slaves then it’s
>always an ARPHRD_ETHER device.
>
>Thoughts ?

	I agree that it would be cleaner for bond_dev->type to switch
back on release of last slave.  The options code (caller of
bond_option_slaves_set) and bond_uninit() both reference the bond or dev
after calling bond_release(), and would need changing if any release
could destroy the bond itself.

	However, for the type change, there's the potentially tricky
case of a nested non-ARPHRD_ETHER bond, e.g., bond0 -> bond1 -> ib0.
This isn't a typical use case that I'm aware of, but I believe it's
supported by the code.

	If ib0, the last slave, is released, bond1 will want to change
to ARPHRD_ETHER, but bond0 is ARPHRD_INFINIBAND.  I suspect bonding will
have to notice the NETDEV_PRE_TYPE_CHANGE and _POST_ notifiers and take
appropriate action (i.e., cascade the type change upwards).

	There might be similar issues with other devices stacked on top
of the IB -> Ether type-changing bond; I'm not sure how many of those
there may be, though, since many things won't stack over IB devices (or
an IB-flavor bond).

	If the type change works, then I don't think we would still need
the "release and destroy" logic.

	-J

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