> On Aug 27, 2015, at 8:39 PM, Jay Vosburgh <jay.vosbu...@canonical.com> wrote: > > Nikolay Aleksandrov <niko...@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). > Ugh right, this would be a problem. I’ll see if it can be handled well.
> If the type change works, then I don't think we would still need > the "release and destroy" logic. > Right, that was my intention. > -J > > --- > -Jay Vosburgh, jay.vosbu...@canonical.com I’ll look into this some more and if it works out I’ll post the patch. Thanks, Nik-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
